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

[PATCH 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                      | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 66e2f5eaa3c0..f29a27e5a053 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,7 @@ DxeImageVerificationHandler (
   RETURN_STATUS                 PeCoffStatus;
   EFI_STATUS                    HashStatus;
   EFI_STATUS                    DbStatus;
+  EFI_STATUS                    VarStatus;
   BOOLEAN                       IsFound;
 
   SignatureList     = NULL;
@@ -1742,24 +1744,22 @@ DxeImageVerificationHandler (
     CpuDeadLoop ();
   }
 
-  GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID **)&SecureBoot, NULL);
+  SecureBootSize = sizeof (SecureBoot);
+  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, NULL, &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) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {
     return EFI_SUCCESS;
   }
 
-  FreePool (SecureBoot);
-
   //
   // Read the Dos header.
   //
-- 
2.39.2

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

[Marvin Häuser]

*Thanks*, Gerd!

Two comments inline.

> On 27. Feb 2023, at 13:55, Gerd Hoffmann <kraxel@redhat.com> 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

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

> (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                      | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 66e2f5eaa3c0..f29a27e5a053 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,7 @@ DxeImageVerificationHandler (
>   RETURN_STATUS                 PeCoffStatus;
>   EFI_STATUS                    HashStatus;
>   EFI_STATUS                    DbStatus;
> +  EFI_STATUS                    VarStatus;
>   BOOLEAN                       IsFound;
> 
>   SignatureList     = NULL;
> @@ -1742,24 +1744,22 @@ DxeImageVerificationHandler (
>     CpuDeadLoop ();
>   }
> 
> -  GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID **)&SecureBoot, NULL);
> +  SecureBootSize = sizeof (SecureBoot);
> +  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, NULL, &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) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {

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).

Best regards,
Marvin

>     return EFI_SUCCESS;
>   }
> 
> -  FreePool (SecureBoot);
> -
>   //
>   // Read the Dos header.
>   //
> -- 
> 2.39.2
> 

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

[Gerd Hoffmann]

  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 this and skip the SB checks only when explicitly disabled?
> Please explicitly respond even if not, so we don't end up with everyone silently agreeing, but forgetting about the patch after. Thanks! :)

I hold back v2, waiting for an answer here.

> > -  if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
> > -    FreePool (SecureBoot);
> > +  if ((VarStatus == EFI_SUCCESS) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {
> 
> 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).

Like this (incremental fixup)?

Do we have macros for variable attribute checking?
Havn't seen anything while skimming variable-related headers ...

take care,
  Gerd

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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;
 
   SignatureList     = NULL;
@@ -1745,7 +1746,7 @@ DxeImageVerificationHandler (
   }
 
   SecureBootSize = sizeof (SecureBoot);
-  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, NULL, &SecureBootSize, &SecureBoot);
+  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, &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 == EFI_SUCCESS) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {
+  if ((VarStatus == EFI_SUCCESS) &&
+      !(VarAttr & EFI_VARIABLE_NON_VOLATILE) &&
+      (VarAttr & EFI_VARIABLE_BOOTSERVICE_ACCESS) &&
+      (VarAttr & EFI_VARIABLE_RUNTIME_ACCESS) &&
+      (SecureBoot == SECURE_BOOT_MODE_DISABLE))
+  {
     return EFI_SUCCESS;
   }
 

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

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 3541 bytes --]


> On 28. Feb 2023, at 07:12, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>   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 this and skip the SB checks only when explicitly disabled?
>> Please explicitly respond even if not, so we don't end up with everyone silently agreeing, but forgetting about the patch after. Thanks! :)
> 
> I hold back v2, waiting for an answer here.
> 
>>> -  if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
>>> -    FreePool (SecureBoot);
>>> +  if ((VarStatus == EFI_SUCCESS) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {
>> 
>> 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).
> 
> Like this (incremental fixup)?

Sorry, I formulated it a bit vague - 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 be just those three Bits checked. Otherwise, yes, thanks a lot!

It’s a read-only status reporting variable, so even with future changes, setting any of the other attributes wouldn’t make much sense. BS | RT is what the spec currently dictates (there is a table here: https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html).

> 
> Do we have macros for variable attribute checking?
> Havn't seen anything while skimming variable-related headers ...

Don’t think so. Not sure there is much attribute-checking done to begin with outside VarCheckLib. The only stack I’ve seen doing this extensively is recent-years Mac EFI.

Best regards,
Marvin

> 
> take care,
>  Gerd
> 
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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;
> 
>   SignatureList     = NULL;
> @@ -1745,7 +1746,7 @@ DxeImageVerificationHandler (
>   }
> 
>   SecureBootSize = sizeof (SecureBoot);
> -  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, NULL, &SecureBootSize, &SecureBoot);
> +  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, &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 == EFI_SUCCESS) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {
> +  if ((VarStatus == EFI_SUCCESS) &&
> +      !(VarAttr & EFI_VARIABLE_NON_VOLATILE) &&
> +      (VarAttr & EFI_VARIABLE_BOOTSERVICE_ACCESS) &&
> +      (VarAttr & EFI_VARIABLE_RUNTIME_ACCESS) &&
> +      (SecureBoot == SECURE_BOOT_MODE_DISABLE))
> +  {
>     return EFI_SUCCESS;
>   }
> 
> 

[-- Attachment #2: Type: text/html, Size: 6691 bytes --]