[PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

[PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

[Gerd Hoffmann]

Call gRT->GetVariable() directly to read the SecureBoot variable.  It is
one byte in size so we can easily place it on the stack instead of
having GetEfiGlobalVariable2() allocate it for us, which avoids a few
possible error cases.

Skip secure boot checks if (and only if):

 (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to
     the return value, or
 (b) the SecureBoot variable was read successfully and is set to
     SECURE_BOOT_MODE_DISABLE.

Previously the code skipped the secure boot checks on *any*
gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable
value to NULL in that case) and also on memory allocation failures.

Fixes: CVE-2019-14560
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2167
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 .../DxeImageVerificationLib.c                 | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 66e2f5eaa3c0..b3d40c21e975 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1671,7 +1671,8 @@ DxeImageVerificationHandler (
   EFI_IMAGE_EXECUTION_ACTION    Action;
   WIN_CERTIFICATE               *WinCertificate;
   UINT32                        Policy;
-  UINT8                         *SecureBoot;
+  UINT8                         SecureBoot;
+  UINTN                         SecureBootSize;
   PE_COFF_LOADER_IMAGE_CONTEXT  ImageContext;
   UINT32                        NumberOfRvaAndSizes;
   WIN_CERTIFICATE_EFI_PKCS      *PkcsCertData;
@@ -1686,6 +1687,8 @@ DxeImageVerificationHandler (
   RETURN_STATUS                 PeCoffStatus;
   EFI_STATUS                    HashStatus;
   EFI_STATUS                    DbStatus;
+  EFI_STATUS                    VarStatus;
+  UINT32                        VarAttr;
   BOOLEAN                       IsFound;
 
   SignatureList     = NULL;
@@ -1742,24 +1745,26 @@ DxeImageVerificationHandler (
     CpuDeadLoop ();
   }
 
-  GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID **)&SecureBoot, NULL);
+  SecureBootSize = sizeof (SecureBoot);
+  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, &VarAttr, &SecureBootSize, &SecureBoot);
   //
   // Skip verification if SecureBoot variable doesn't exist.
   //
-  if (SecureBoot == NULL) {
+  if (VarStatus == EFI_NOT_FOUND) {
     return EFI_SUCCESS;
   }
 
   //
   // Skip verification if SecureBoot is disabled but not AuditMode
   //
-  if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
-    FreePool (SecureBoot);
+  if ((VarStatus == EFI_SUCCESS) &&
+      (VarAttr == (EFI_VARIABLE_BOOTSERVICE_ACCESS |
+                   EFI_VARIABLE_RUNTIME_ACCESS)) &&
+      (SecureBoot == SECURE_BOOT_MODE_DISABLE))
+  {
     return EFI_SUCCESS;
   }
 
-  FreePool (SecureBoot);
-
   //
   // Read the Dos header.
   //
-- 
2.39.2

Re: [PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

[Gerd Hoffmann]

On Fri, Mar 03, 2023 at 11:35:53AM +0100, Gerd Hoffmann wrote:
> Call gRT->GetVariable() directly to read the SecureBoot variable.  It is
> one byte in size so we can easily place it on the stack instead of
> having GetEfiGlobalVariable2() allocate it for us, which avoids a few
> possible error cases.
> 
> Skip secure boot checks if (and only if):
> 
>  (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to
>      the return value, or
>  (b) the SecureBoot variable was read successfully and is set to
>      SECURE_BOOT_MODE_DISABLE.
> 
> Previously the code skipped the secure boot checks on *any*
> gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable
> value to NULL in that case) and also on memory allocation failures.
> 
> Fixes: CVE-2019-14560
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2167
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Ping.  Any comments on this patch?

take care,
  Gerd

Re: [PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

[Min Xu]

It's good to me.
Reviewed-by: Min Xu <min.m.xu@intel.com>

Thanks

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Monday, March 20, 2023 6:02 PM
> To: devel@edk2.groups.io
> Cc: Pawel Polawski <ppolawsk@redhat.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Oliver Steffen <osteffen@redhat.com>; Xu, Min M
> <min.m.xu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>; Yao, Jiewen
> <jiewen.yao@intel.com>; jmaloy@redhat.com
> Subject: Re: [PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check
> result of GetEfiGlobalVariable2
> 
> On Fri, Mar 03, 2023 at 11:35:53AM +0100, Gerd Hoffmann wrote:
> > Call gRT->GetVariable() directly to read the SecureBoot variable.  It
> > is one byte in size so we can easily place it on the stack instead of
> > having GetEfiGlobalVariable2() allocate it for us, which avoids a few
> > possible error cases.
> >
> > Skip secure boot checks if (and only if):
> >
> >  (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to
> >      the return value, or
> >  (b) the SecureBoot variable was read successfully and is set to
> >      SECURE_BOOT_MODE_DISABLE.
> >
> > Previously the code skipped the secure boot checks on *any*
> > gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable
> > value to NULL in that case) and also on memory allocation failures.
> >
> > Fixes: CVE-2019-14560
> > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2167
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Ping.  Any comments on this patch?
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

[Yao, Jiewen]

Would you please share with us what test has been done for this patch?

Thank you
Yao, Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Monday, March 20, 2023 6:02 PM
> To: devel@edk2.groups.io
> Cc: Pawel Polawski <ppolawsk@redhat.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Oliver Steffen <osteffen@redhat.com>; Xu, Min M
> <min.m.xu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>; Yao,
> Jiewen <jiewen.yao@intel.com>; jmaloy@redhat.com
> Subject: Re: [edk2-devel] [PATCH v2 1/1]
> SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2
> 
> On Fri, Mar 03, 2023 at 11:35:53AM +0100, Gerd Hoffmann wrote:
> > Call gRT->GetVariable() directly to read the SecureBoot variable.  It is
> > one byte in size so we can easily place it on the stack instead of
> > having GetEfiGlobalVariable2() allocate it for us, which avoids a few
> > possible error cases.
> >
> > Skip secure boot checks if (and only if):
> >
> >  (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to
> >      the return value, or
> >  (b) the SecureBoot variable was read successfully and is set to
> >      SECURE_BOOT_MODE_DISABLE.
> >
> > Previously the code skipped the secure boot checks on *any*
> > gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable
> > value to NULL in that case) and also on memory allocation failures.
> >
> > Fixes: CVE-2019-14560
> > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2167
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Ping.  Any comments on this patch?
> 
> take care,
>   Gerd
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

[Gerd Hoffmann]

On Mon, Mar 20, 2023 at 01:20:29PM +0000, Yao, Jiewen wrote:
> Would you please share with us what test has been done for this patch?

Usual regression testing, including booting images with and without
secure boot.  Additionally checked images with the wrong signature
are rejected (try boot grub.efi directly instead of using the
shim.efi -> grub.efi chain).

take care,
  Gerd

> 
> Thank you
> Yao, Jiewen
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> > Hoffmann
> > Sent: Monday, March 20, 2023 6:02 PM
> > To: devel@edk2.groups.io
> > Cc: Pawel Polawski <ppolawsk@redhat.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Oliver Steffen <osteffen@redhat.com>; Xu, Min M
> > <min.m.xu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>; Yao,
> > Jiewen <jiewen.yao@intel.com>; jmaloy@redhat.com
> > Subject: Re: [edk2-devel] [PATCH v2 1/1]
> > SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2
> > 
> > On Fri, Mar 03, 2023 at 11:35:53AM +0100, Gerd Hoffmann wrote:
> > > Call gRT->GetVariable() directly to read the SecureBoot variable.  It is
> > > one byte in size so we can easily place it on the stack instead of
> > > having GetEfiGlobalVariable2() allocate it for us, which avoids a few
> > > possible error cases.
> > >
> > > Skip secure boot checks if (and only if):
> > >
> > >  (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to
> > >      the return value, or
> > >  (b) the SecureBoot variable was read successfully and is set to
> > >      SECURE_BOOT_MODE_DISABLE.
> > >
> > > Previously the code skipped the secure boot checks on *any*
> > > gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable
> > > value to NULL in that case) and also on memory allocation failures.
> > >
> > > Fixes: CVE-2019-14560
> > > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2167
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > Ping.  Any comments on this patch?
> > 
> > take care,
> >   Gerd
> > 
> > 
> > 
> > 
> > 
> 
> 
> 
> 
> 
> 

-- 

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

[Yao, Jiewen]

Sounds good. Thanks.

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Monday, March 20, 2023 11:00 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Pawel Polawski <ppolawsk@redhat.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Oliver Steffen <osteffen@redhat.com>; Xu, Min M
> <min.m.xu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>;
> jmaloy@redhat.com
> Subject: Re: [edk2-devel] [PATCH v2 1/1]
> SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2
> 
> On Mon, Mar 20, 2023 at 01:20:29PM +0000, Yao, Jiewen wrote:
> > Would you please share with us what test has been done for this patch?
> 
> Usual regression testing, including booting images with and without
> secure boot.  Additionally checked images with the wrong signature
> are rejected (try boot grub.efi directly instead of using the
> shim.efi -> grub.efi chain).
> 
> take care,
>   Gerd
> 
> >
> > Thank you
> > Yao, Jiewen
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> > > Hoffmann
> > > Sent: Monday, March 20, 2023 6:02 PM
> > > To: devel@edk2.groups.io
> > > Cc: Pawel Polawski <ppolawsk@redhat.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>; Oliver Steffen <osteffen@redhat.com>; Xu,
> Min M
> > > <min.m.xu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>; Yao,
> > > Jiewen <jiewen.yao@intel.com>; jmaloy@redhat.com
> > > Subject: Re: [edk2-devel] [PATCH v2 1/1]
> > > SecurityPkg/DxeImageVerificationLib: Check result of
> GetEfiGlobalVariable2
> > >
> > > On Fri, Mar 03, 2023 at 11:35:53AM +0100, Gerd Hoffmann wrote:
> > > > Call gRT->GetVariable() directly to read the SecureBoot variable.  It is
> > > > one byte in size so we can easily place it on the stack instead of
> > > > having GetEfiGlobalVariable2() allocate it for us, which avoids a few
> > > > possible error cases.
> > > >
> > > > Skip secure boot checks if (and only if):
> > > >
> > > >  (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according
> to
> > > >      the return value, or
> > > >  (b) the SecureBoot variable was read successfully and is set to
> > > >      SECURE_BOOT_MODE_DISABLE.
> > > >
> > > > Previously the code skipped the secure boot checks on *any*
> > > > gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable
> > > > value to NULL in that case) and also on memory allocation failures.
> > > >
> > > > Fixes: CVE-2019-14560
> > > > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2167
> > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > >
> > > Ping.  Any comments on this patch?
> > >
> > > take care,
> > >   Gerd
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> > 
> >
> >
> 
> --

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

[Yao, Jiewen]

Merged https://github.com/tianocore/edk2/pull/4155


> -----Original Message-----
> From: Yao, Jiewen
> Sent: Tuesday, March 21, 2023 10:29 AM
> To: kraxel@redhat.com; devel@edk2.groups.io
> Cc: Pawel Polawski <ppolawsk@redhat.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Oliver Steffen <osteffen@redhat.com>; Xu, Min M
> <min.m.xu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>;
> jmaloy@redhat.com
> Subject: RE: [edk2-devel] [PATCH v2 1/1]
> SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2
> 
> Sounds good. Thanks.
> 
> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> > -----Original Message-----
> > From: kraxel@redhat.com <kraxel@redhat.com>
> > Sent: Monday, March 20, 2023 11:00 PM
> > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: Pawel Polawski <ppolawsk@redhat.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Oliver Steffen <osteffen@redhat.com>; Xu, Min
> M
> > <min.m.xu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>;
> > jmaloy@redhat.com
> > Subject: Re: [edk2-devel] [PATCH v2 1/1]
> > SecurityPkg/DxeImageVerificationLib: Check result of
> GetEfiGlobalVariable2
> >
> > On Mon, Mar 20, 2023 at 01:20:29PM +0000, Yao, Jiewen wrote:
> > > Would you please share with us what test has been done for this patch?
> >
> > Usual regression testing, including booting images with and without
> > secure boot.  Additionally checked images with the wrong signature
> > are rejected (try boot grub.efi directly instead of using the
> > shim.efi -> grub.efi chain).
> >
> > take care,
> >   Gerd
> >
> > >
> > > Thank you
> > > Yao, Jiewen
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Gerd
> > > > Hoffmann
> > > > Sent: Monday, March 20, 2023 6:02 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Pawel Polawski <ppolawsk@redhat.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>; Oliver Steffen <osteffen@redhat.com>; Xu,
> > Min M
> > > > <min.m.xu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>; Yao,
> > > > Jiewen <jiewen.yao@intel.com>; jmaloy@redhat.com
> > > > Subject: Re: [edk2-devel] [PATCH v2 1/1]
> > > > SecurityPkg/DxeImageVerificationLib: Check result of
> > GetEfiGlobalVariable2
> > > >
> > > > On Fri, Mar 03, 2023 at 11:35:53AM +0100, Gerd Hoffmann wrote:
> > > > > Call gRT->GetVariable() directly to read the SecureBoot variable.  It is
> > > > > one byte in size so we can easily place it on the stack instead of
> > > > > having GetEfiGlobalVariable2() allocate it for us, which avoids a few
> > > > > possible error cases.
> > > > >
> > > > > Skip secure boot checks if (and only if):
> > > > >
> > > > >  (a) the SecureBoot variable is not present (EFI_NOT_FOUND)
> according
> > to
> > > > >      the return value, or
> > > > >  (b) the SecureBoot variable was read successfully and is set to
> > > > >      SECURE_BOOT_MODE_DISABLE.
> > > > >
> > > > > Previously the code skipped the secure boot checks on *any*
> > > > > gRT->GetVariable() error (GetEfiGlobalVariable2 sets the variable
> > > > > value to NULL in that case) and also on memory allocation failures.
> > > > >
> > > > > Fixes: CVE-2019-14560
> > > > > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2167
> > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > >
> > > > Ping.  Any comments on this patch?
> > > >
> > > > take care,
> > > >   Gerd
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > > 
> > >
> > >
> >
> > --