Re: [PATCH] SecurityPkg: check return value of GetEfiGlobalVariable2() in DxeImageVerificationHandler()

[Jon Maloy]

Please ignore this one.  Sent to the wrong list.
///jon

On 11/24/22 21:32, Jon Maloy wrote:
> 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.
>
> 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 66e2f5eaa3..6c58b71d37 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;
> -  }
> -
> -  //
> -  // Skip verification if SecureBoot is disabled but not AuditMode
> -  //
> -  if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
> -    FreePool (SecureBoot);
> -    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.
> +    //
> +   }
>   
> -  FreePool (SecureBoot);
>   
>     //
>     // Read the Dos header.