From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web10.18928.1677574033950441872 for ; Tue, 28 Feb 2023 00:47:14 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=NNgUJeBB; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 3A2DD2401C9 for ; Tue, 28 Feb 2023 09:47:12 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1677574032; bh=pIWYVCA9xJlP4+O2vGFeLpRJpFiHNQ7oRtIay3RWNRo=; h=From:Subject:Date:Cc:To:From; b=NNgUJeBBDnnfuenFvhzD7+cmfh5LJgH5hyrxQ4pY++8shsS//FjmlgaHadf3Bqu/h 1K2WhzrTdKjhzNXyvYUWYgXOLFQeWgiMfBav/zj1ShIX9VeyvF9bnOdcd3k48np9vQ UsMoaj6PlEil3I8Ell60dfupXoXzf8nBzoL7WCeX28MAkhsSMzHWGePYsRz3z5M4yA 4CLtBJyxnvj4g9YsQbPvPPGn2W5C/DifBs9jxeMWmUy28IcX2UHibtFB/4s/qGPnwL QFjIjHBYadQN6gS8cRCqPhrGuxcWg6rbCaUHr3v+QvAUyqJ3z/51aFMsPyYEuo8WKG FB/DeBiEtHkOg== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4PQrb25lyMz6tmV; Tue, 28 Feb 2023 09:47:10 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [PATCH 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2 Date: Tue, 28 Feb 2023 08:47:10 +0000 Message-Id: <50ACE54B-1E92-4469-BD8F-FBC02FFEF061@posteo.de> References: <20230228061222.duxm66xdrvsnlgxc@sirius.home.kraxel.org> Cc: edk2-devel-groups-io , Pawel Polawski , Jiewen Yao , jmaloy@redhat.com, Min Xu , Jian J Wang , Oliver Steffen In-Reply-To: <20230228061222.duxm66xdrvsnlgxc@sirius.home.kraxel.org> To: Gerd Hoffmann Content-Type: multipart/alternative; boundary=Apple-Mail-FE00270B-2915-4B60-9E36-C447665E2E8A Content-Transfer-Encoding: 7bit --Apple-Mail-FE00270B-2915-4B60-9E36-C447665E2E8A Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > On 28. Feb 2023, at 07:12, Gerd Hoffmann wrote: >=20 > =EF=BB=BF Hi, >=20 >>> (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to >>> the return value, or >>=20 >> @Maintainers Would there be any objection to drop this and skip the SB ch= ecks only when explicitly disabled? >> Please explicitly respond even if not, so we don't end up with everyone s= ilently agreeing, but forgetting about the patch after. Thanks! :) >=20 > I hold back v2, waiting for an answer here. >=20 >>> - if (*SecureBoot =3D=3D SECURE_BOOT_MODE_DISABLE) { >>> - FreePool (SecureBoot); >>> + if ((VarStatus =3D=3D EFI_SUCCESS) && (SecureBoot =3D=3D SECURE_BOOT_= MODE_DISABLE)) { >>=20 >> I would check the attributes here as well. They should be BS | RT, but >> explicitly not NV. This would force the SB checks in case a malicious >> actor somehow managed to store a persistent disable-value variable (be >> that a bug, physical access, or other means). >=20 > Like this (incremental fixup)? Sorry, I formulated it a bit vague - what I meant is that the attributes sho= uld be exactly BS | RT (i.e., equal to), but I see how adding it must not be= NV sounds like it should be just those three Bits checked. Otherwise, yes, t= hanks a lot! It=E2=80=99s a read-only status reporting variable, so even with future chan= ges, setting any of the other attributes wouldn=E2=80=99t make much sense. B= S | RT is what the spec currently dictates (there is a table here: https://u= efi.org/specs/UEFI/2.10/03_Boot_Manager.html). >=20 > Do we have macros for variable attribute checking? > Havn't seen anything while skimming variable-related headers ... Don=E2=80=99t think so. Not sure there is much attribute-checking done to be= gin with outside VarCheckLib. The only stack I=E2=80=99ve seen doing this ex= tensively is recent-years Mac EFI. Best regards, Marvin >=20 > take care, > Gerd >=20 > diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificat= ionLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL= ib.c > index f29a27e5a053..79c784f77ac8 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.= c > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.= c > @@ -1688,6 +1688,7 @@ DxeImageVerificationHandler ( > EFI_STATUS HashStatus; > EFI_STATUS DbStatus; > EFI_STATUS VarStatus; > + UINT32 VarAttr; > BOOLEAN IsFound; >=20 > SignatureList =3D NULL; > @@ -1745,7 +1746,7 @@ DxeImageVerificationHandler ( > } >=20 > SecureBootSize =3D sizeof (SecureBoot); > - VarStatus =3D gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGl= obalVariableGuid, NULL, &SecureBootSize, &SecureBoot); > + VarStatus =3D gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGl= obalVariableGuid, &VarAttr, &SecureBootSize, &SecureBoot); > // > // Skip verification if SecureBoot variable doesn't exist. > // > @@ -1756,7 +1757,12 @@ DxeImageVerificationHandler ( > // > // Skip verification if SecureBoot is disabled but not AuditMode > // > - if ((VarStatus =3D=3D EFI_SUCCESS) && (SecureBoot =3D=3D SECURE_BOOT_MO= DE_DISABLE)) { > + if ((VarStatus =3D=3D EFI_SUCCESS) && > + !(VarAttr & EFI_VARIABLE_NON_VOLATILE) && > + (VarAttr & EFI_VARIABLE_BOOTSERVICE_ACCESS) && > + (VarAttr & EFI_VARIABLE_RUNTIME_ACCESS) && > + (SecureBoot =3D=3D SECURE_BOOT_MODE_DISABLE)) > + { > return EFI_SUCCESS; > } >=20 >=20 --Apple-Mail-FE00270B-2915-4B60-9E36-C447665E2E8A Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
On 28. Feb 2023, at 07:12,= Gerd Hoffmann <kraxel@redhat.com> wrote:

=EF=BB=BF  Hi,
= (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to
   the return value, or
=

@Maintainers Would there be any objection to drop t= his and skip the SB checks only when explicitly disabled?
Please explicitly respond even if not, s= o we don't end up with everyone silently agreeing, but forgetting about the p= atch after. Thanks! :)

I hold b= ack v2, waiting for an answer here.

-  if (*SecureBoot =3D=3D S= ECURE_BOOT_MODE_DISABLE) {
-    FreePool (S= ecureBoot);
+  if ((VarStatus =3D=3D EFI_SUCCESS) &am= p;& (SecureBoot =3D=3D SECURE_BOOT_MODE_DISABLE)) {

I would check the attributes here as well. They s= hould be BS | RT, but
explicitly not NV. This would force the SB checks in case a malicious
actor somehow managed to s= tore a persistent disable-value variable (be
that a bug, physical access, or other means).=

Like this (incremental fixup)?

Sorry, I formulated it a bit vag= ue - what I meant is that the attributes should be exactly BS | RT (i.e= ., equal to), but I see how adding it must not be NV sounds like it should b= e just those three Bits checked. Otherwise, yes, thanks a lot!
It=E2=80=99s a read-only status reporting variable, so even with= future changes, setting any of the other attributes wouldn=E2=80=99t make m= uch sense. BS | RT is what the spec currently dictates (there is a table her= e: ht= tps://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html).


Do we have macros f= or variable attribute checking?
Havn't seen anything while s= kimming variable-related headers ...

<= /div>
Don=E2=80=99t think so. Not sure there is much attribute-checking d= one to begin with outside VarCheckLib. The only stack I=E2=80=99ve seen doin= g this extensively is recent-years Mac EFI.

Best re= gards,
Marvin

=
take care,
 Gerd

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/= DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeI= mageVerificationLib.c
index f29a27e5a053..79c784f77ac8 10064= 4
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImage= VerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerifica= tionLib/DxeImageVerificationLib.c
@@ -1688,6 +1688,7 @@ DxeI= mageVerificationHandler (
  EFI_STATUS  &nbs= p;            &n= bsp;    HashStatus;
  EFI_STA= TUS             =        DbStatus;
 &= nbsp;EFI_STATUS           =          VarStatus;
<= span>+  UINT32          &n= bsp;            =  VarAttr;
  BOOLEAN     =             &nbs= p;     IsFound;

&= nbsp; SignatureList     =3D NULL;
@= @ -1745,7 +1746,7 @@ DxeImageVerificationHandler (
 &n= bsp;}

  SecureBootSize =3D sizeo= f (SecureBoot);
-  VarStatus     &n= bsp;=3D gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVaria= bleGuid, NULL, &SecureBootSize, &SecureBoot);
+ &nbs= p;VarStatus      =3D gRT->GetVariable (EFI_SECUR= E_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, &VarAttr, &SecureBoot= Size, &SecureBoot);
  //
&nb= sp; // Skip verification if SecureBoot variable doesn't exist.   //
@@ -1756,7 +1757,12 @@ DxeImageVerif= icationHandler (
  //
 &nbs= p;// Skip verification if SecureBoot is disabled but not AuditMode   //
-  if ((VarStatus =3D=3D EFI_SUC= CESS) && (SecureBoot =3D=3D SECURE_BOOT_MODE_DISABLE)) {
<= span>+  if ((VarStatus =3D=3D EFI_SUCCESS) &&
+=      !(VarAttr & EFI_VARIABLE_NON_VOLATILE) &a= mp;&
+      (VarAttr & EFI_= VARIABLE_BOOTSERVICE_ACCESS) &&
+    =   (VarAttr & EFI_VARIABLE_RUNTIME_ACCESS) &&+      (SecureBoot =3D=3D SECURE_BOOT_MODE_D= ISABLE))
+  {
    = return EFI_SUCCESS;
  }

= --Apple-Mail-FE00270B-2915-4B60-9E36-C447665E2E8A--