1. Patchew
  2. EDK2
  3. View series

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

Jon Maloy posted 1 patch 1 year, 4 months ago
Failed in applying to current master (apply log)
.../DxeImageVerificationLib.c                 | 39 +++++++++++--------
1 file changed, 23 insertions(+), 16 deletions(-)
[edk2-devel] [PATCH] SecurityPkg: check return value of GetEfiGlobalVariable2() in DxeImageVerificationHandler()
Posted by Jon Maloy 1 year, 4 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96881): https://edk2.groups.io/g/devel/message/96881
Mute This Topic: https://groups.io/mt/95399277/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] SecurityPkg: check return value of GetEfiGlobalVariable2() in DxeImageVerificationHandler()
Posted by Yao, Jiewen 1 year, 4 months ago 
Thanks for the patch.

Would you please share the info that, how this patch is tested?

Have you created a situation to cause GetEfiGlobalVariable2() returns failure here?


> -----Original Message-----
> From: Jon Maloy <jmaloy@redhat.com>
> Sent: Friday, December 2, 2022 11:05 AM
> To: devel@edk2.groups.io
> Cc: kraxel@redhat.com; lersek@redhat.com; jmaloy@redhat.com; Yao,
> Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Xu,
> Min M <min.m.xu@intel.com>
> Subject: [edk2-devel] [PATCH] SecurityPkg: check return value of
> GetEfiGlobalVariable2() in DxeImageVerificationHandler()
> 
> 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97169): https://edk2.groups.io/g/devel/message/97169
Mute This Topic: https://groups.io/mt/95399277/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.