From bb0f29580825e60a5dc5c67e260dd20258eb71b0 Mon Sep 17 00:00:00 2001
From: Jon Maloy <jmaloy@redhat.com>
Date: Wed, 29 Mar 2023 11:52:52 -0400
Subject: [PATCH] SecurityPkg/DxeImageVerificationLib: Check result of
 GetEfiGlobalVariable2
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

RH-Author: Jon Maloy <jmaloy@redhat.com>
RH-MergeRequest: 22: SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2
RH-Bugzilla: 1861743
RH-Acked-by: Gerd Hoffmann <kraxel@redhat.com>
RH-Commit: [1/1] 70e1ae5e2c7c148fc23160acdd360c044df5f4ff

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1861743
Upstream: Merged
CVE: CVE-2019-14560

commit 494127613b36e870250649b02cd4ce5f1969d9bd
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Fri Mar 3 18:35:53 2023 +0800

    SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

    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
    Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
    Suggested-by: Marvin Häuser <mhaeuser@posteo.de>
    Reviewed-by: Min Xu <min.m.xu@intel.com>
    Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

Signed-off-by: Jon Maloy <jmaloy@redhat.com>
---
 .../DxeImageVerificationLib.c                  | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index c48861cd64..1252927664 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1650,7 +1650,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;
@@ -1665,6 +1666,8 @@ DxeImageVerificationHandler (
   RETURN_STATUS                        PeCoffStatus;
   EFI_STATUS                           HashStatus;
   EFI_STATUS                           DbStatus;
+  EFI_STATUS                           VarStatus;
+  UINT32                               VarAttr;
   BOOLEAN                              IsFound;
 
   SignatureList     = NULL;
@@ -1720,22 +1723,25 @@ 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.1