From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web10.2683.1580439575560071175 for ; Thu, 30 Jan 2020 18:59:35 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: michael.d.kinney@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jan 2020 18:59:35 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,384,1574150400"; d="scan'208";a="253213494" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by fmsmga004.fm.intel.com with ESMTP; 30 Jan 2020 18:59:35 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.57]) by ORSMSX109.amr.corp.intel.com ([169.254.11.6]) with mapi id 14.03.0439.000; Thu, 30 Jan 2020 18:59:34 -0800 From: "Michael D Kinney" To: "devel@edk2.groups.io" , "lersek@redhat.com" , "Kinney, Michael D" CC: "Zhang, Chao B" , "Wang, Jian J" , "Yao, Jiewen" Subject: Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Thread-Topic: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Thread-Index: AQHVzKAr/+ElsmeVKUm+2G2+2jSfcqgEKWCQ Date: Fri, 31 Jan 2020 02:59:34 +0000 Message-ID: References: <20200116190705.18816-1-lersek@redhat.com> In-Reply-To: <20200116190705.18816-1-lersek@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, I have reviewed this patch series. All the patches look good. The detailed description of each change made it=20 easy to review. Series Reviewed-by: Michael D Kinney I have one question about that is not directly related to this logic change. I see this logic that checks for invalid policy settings and ASSERT()s and halts in a deadloop if either of 2 specific values are used that have been removed from the UEFI Spec. // // The policy QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECU= RITY_VIOLATION // violates the UEFI spec and has been removed. // ASSERT (Policy !=3D QUERY_USER_ON_SECURITY_VIOLATION && Policy !=3D ALLO= W_EXECUTE_ON_SECURITY_VIOLATION); if (Policy =3D=3D QUERY_USER_ON_SECURITY_VIOLATION || Policy =3D=3D ALLO= W_EXECUTE_ON_SECURITY_VIOLATION) { CpuDeadLoop (); } There are 3 conditions where the Policy comes from a PCD. // // Check the image type and get policy setting. // switch (GetImageType (File)) { case IMAGE_FROM_FV: Policy =3D ALWAYS_EXECUTE; break; case IMAGE_FROM_OPTION_ROM: Policy =3D PcdGet32 (PcdOptionRomImageVerificationPolicy); break; case IMAGE_FROM_REMOVABLE_MEDIA: Policy =3D PcdGet32 (PcdRemovableMediaImageVerificationPolicy); break; case IMAGE_FROM_FIXED_MEDIA: Policy =3D PcdGet32 (PcdFixedMediaImageVerificationPolicy); break; default: Policy =3D DENY_EXECUTE_ON_SECURITY_VIOLATION; break; } However, there are no checks in this function to verify that=20 Policy is set to one of the allowed values. This means an invalid PCD value will fall through to the EFI_ACCESS_DENIED case. Is this the expected behavior for an invalid PCD setting? If so, then why is there a check for the retired values from the UEFI Spec and a deadloop performed. That seems inconsistent and not a good idea to deadloop if we do not have to. Would it be better to return EFI_ACCESS_DENIED for these 2 retired Policy values as well? I am ok if you consider this to be a different issue.=20 Thanks, Mike > -----Original Message----- > From: devel@edk2.groups.io On > Behalf Of Laszlo Ersek > Sent: Thursday, January 16, 2020 11:07 AM > To: edk2-devel-groups-io > Cc: Zhang, Chao B ; Wang, Jian > J ; Yao, Jiewen > > Subject: [edk2-devel] [PATCH 00/11] > SecurityPkg/DxeImageVerificationHandler: fix retval for > "deny" policy >=20 > Ref: > https://bugzilla.tianocore.org/show_bug.cgi?id=3D2129 > Repo: https://github.com/lersek/edk2.git > Branch: deny_execute_2129 >=20 > The DxeImageVerificationHandler() function does not > handle the > DENY_EXECUTE_ON_SECURITY_VIOLATION policy correctly. > When an image is > rejected, and the platform sets this policy for the > corresponding image > source, the function should return EFI_ACCESS_DENIED. > Instead, the > function currently returns EFI_SECURITY_VIOLATION. The > consequence is > that gBS->LoadImage() will keep the image loaded (in > untrusted state), > rather than unloading it immediately. If the platform > sets the > DENY_EXECUTE_ON_SECURITY_VIOLATION policy for all image > sources, then > the platform may not expect EFI_SECURITY_VIOLATION at > all. Then, > rejected images may linger in RAM, in untrusted state, > and may be leaked > forever. >=20 > This series refactors the DxeImageVerificationHandler() > function, > simplifying the control flow. The series also improves > the conformance > of the return values to the > SECURITY2_FILE_AUTHENTICATION_HANDLER > prototype. The last two patches are actual bugfixes, > with the last one > fixing the problem laid out above. >=20 > The patches in this posting have been formatted with > "--function-context", for easier review. >=20 > Cc: Chao Zhang > Cc: Jian J Wang > Cc: Jiewen Yao >=20 > Thanks, > Laszlo >=20 > Laszlo Ersek (11): > SecurityPkg/DxeImageVerificationHandler: simplify > "VerifyStatus" > SecurityPkg/DxeImageVerificationHandler: remove > "else" after > return/break > SecurityPkg/DxeImageVerificationHandler: keep PE/COFF > info status > internal > SecurityPkg/DxeImageVerificationHandler: narrow down > PE/COFF hash > status > SecurityPkg/DxeImageVerificationHandler: fix retval > on memalloc > failure > SecurityPkg/DxeImageVerificationHandler: remove > superfluous Status > setting > SecurityPkg/DxeImageVerificationHandler: unnest > AddImageExeInfo() call > SecurityPkg/DxeImageVerificationHandler: eliminate > "Status" variable > SecurityPkg/DxeImageVerificationHandler: fix retval > for > (FileBuffer=3D=3DNULL) > SecurityPkg/DxeImageVerificationHandler: fix imgexec > info on memalloc > fail > SecurityPkg/DxeImageVerificationHandler: fix "defer" > vs. "deny" > policies >=20 >=20 > SecurityPkg/Library/DxeImageVerificationLib/DxeImageVer > ificationLib.c | 118 ++++++++++---------- > 1 file changed, 59 insertions(+), 59 deletions(-) >=20 > -- > 2.19.1.3.g30247aa5d201 >=20 >=20 >=20