[edk2-devel] [PATCH] SecurityPkg: check return value of GetEfiGlobalVariable2() in DxeImageVerificationHandler()

["Jon Maloy" <jmaloy@redhat.com>]

Fixes: CVE-2019-14560

GetEfiGlobalVariable2() is used in some instances when looking up the
SecureBoot UEFI variable. The API can fail in certain circumstances,
for example, if AllocatePool() fails or if gRT->GetVariable() fails.
In the case of secure boot checks, it is critical that this return value
is checked. if an attacker can cause the API to fail, it would currently
constitute a secure boot bypass.

This return value check is missing in the function DxeImageVerificationHandler(),
so we add it here.

This commit is almost identical to one suggested by Jian J Wang <jian.j.wang@intel.com>
on 2019-09-09, but that one was for some reason never posted to the edk2-devel
list. We now make a new attempt to get it reviewed and applied.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: devel@edk2.groups.io

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

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 66e2f5eaa3c0..4ae0bd8b20db 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1686,6 +1686,7 @@ DxeImageVerificationHandler (
   RETURN_STATUS                 PeCoffStatus;
   EFI_STATUS                    HashStatus;
   EFI_STATUS                    DbStatus;
+  EFI_STATUS                    SecBootStatus;
   BOOLEAN                       IsFound;
 
   SignatureList     = NULL;
@@ -1742,23 +1743,29 @@ DxeImageVerificationHandler (
     CpuDeadLoop ();
   }
 
-  GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID **)&SecureBoot, NULL);
-  //
-  // Skip verification if SecureBoot variable doesn't exist.
-  //
-  if (SecureBoot == NULL) {
-    return EFI_SUCCESS;
-  }
+  SecBootStatus = GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID **)&SecureBoot, NULL);
+  if (!EFI_ERROR (SecBootStatus)) {
+    if (SecureBoot == NULL) {
+      //
+      // Skip verification if SecureBoot variable doesn't exist.
+      //
+      return EFI_SUCCESS;
+    } else {
+      //
+      // Skip verification if SecureBoot is disabled but not AuditMode
+      //
+      if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
+        FreePool (SecureBoot);
+        return EFI_SUCCESS;
+      }
+      FreePool (SecureBoot);
+    }
+  } else {
+    //
+    // Assume SecureBoot enabled in the case of error.
+    //
+   }
 
-  //
-  // Skip verification if SecureBoot is disabled but not AuditMode
-  //
-  if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
-    FreePool (SecureBoot);
-    return EFI_SUCCESS;
-  }
-
-  FreePool (SecureBoot);
 
   //
   // Read the Dos header.
-- 
2.35.3