Re: [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256 hash in dbx

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hi Marvin
With this patch, the path "Action == EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND" no longer exists.

Do you think we should remove EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND as well?



Thank you
Yao Jiewen


> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Monday, August 9, 2021 3:40 AM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Xu, Min M <min.m.xu@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
> Subject: [PATCH] SecurityPkg/DxeImageVerificationLib: Always lookup SHA-256
> hash in dbx
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3461
> 
> The UEFI specification prohibits loading any UEFI image of which a
> matching SHA-256 hash is contained in "dbx" (UEFI 2.9, 32.5.3.3
> "Authorization Process", 3.A). Currently, this is only explicitly
> checked when the image is unsigned and otherwise the hash algorithms
> of the certificates are used.
> 
> Align with the UEFI specification by specifically looking up the
> SHA-256 hash of the image in "dbx".
> 
> 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: Vitaly Cheptsov <vit9696@protonmail.com>
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> ---
>  SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 60
> ++++++++------------
>  1 file changed, 24 insertions(+), 36 deletions(-)
> 
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index c48861cd6496..1f9bb33e86c3 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1803,34 +1803,36 @@ DxeImageVerificationHandler (
>      }
> 
>    }
> 
> 
> 
> +  //
> 
> +  // The SHA256 hash value of the image must not be reflected in the security
> data base "dbx".
> 
> +  //
> 
> +  if (!HashPeImage (HASHALG_SHA256)) {
> 
> +    DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image
> using %s.\n", mHashTypeStr));
> 
> +    goto Failed;
> 
> +  }
> 
> +
> 
> +  DbStatus = IsSignatureFoundInDatabase (
> 
> +               EFI_IMAGE_SECURITY_DATABASE1,
> 
> +               mImageDigest,
> 
> +               &mCertType,
> 
> +               mImageDigestSize,
> 
> +               &IsFound
> 
> +               );
> 
> +  if (EFI_ERROR (DbStatus) || IsFound) {
> 
> +    //
> 
> +    // Image Hash is in forbidden database (DBX).
> 
> +    //
> 
> +    DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed
> and %s hash of image is forbidden by DBX.\n", mHashTypeStr));
> 
> +    goto Failed;
> 
> +  }
> 
> +
> 
>    //
> 
>    // Start Image Validation.
> 
>    //
> 
>    if (SecDataDir == NULL || SecDataDir->Size == 0) {
> 
>      //
> 
> -    // This image is not signed. The SHA256 hash value of the image must match
> a record in the security database "db",
> 
> -    // and not be reflected in the security data base "dbx".
> 
> +    // This image is not signed. The SHA256 hash value of the image must match
> a record in the security database "db".
> 
>      //
> 
> -    if (!HashPeImage (HASHALG_SHA256)) {
> 
> -      DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this image
> using %s.\n", mHashTypeStr));
> 
> -      goto Failed;
> 
> -    }
> 
> -
> 
> -    DbStatus = IsSignatureFoundInDatabase (
> 
> -                 EFI_IMAGE_SECURITY_DATABASE1,
> 
> -                 mImageDigest,
> 
> -                 &mCertType,
> 
> -                 mImageDigestSize,
> 
> -                 &IsFound
> 
> -                 );
> 
> -    if (EFI_ERROR (DbStatus) || IsFound) {
> 
> -      //
> 
> -      // Image Hash is in forbidden database (DBX).
> 
> -      //
> 
> -      DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed
> and %s hash of image is forbidden by DBX.\n", mHashTypeStr));
> 
> -      goto Failed;
> 
> -    }
> 
> -
> 
>      DbStatus = IsSignatureFoundInDatabase (
> 
>                   EFI_IMAGE_SECURITY_DATABASE,
> 
>                   mImageDigest,
> 
> @@ -1932,20 +1934,6 @@ DxeImageVerificationHandler (
>      //
> 
>      // Check the image's hash value.
> 
>      //
> 
> -    DbStatus = IsSignatureFoundInDatabase (
> 
> -                 EFI_IMAGE_SECURITY_DATABASE1,
> 
> -                 mImageDigest,
> 
> -                 &mCertType,
> 
> -                 mImageDigestSize,
> 
> -                 &IsFound
> 
> -                 );
> 
> -    if (EFI_ERROR (DbStatus) || IsFound) {
> 
> -      Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
> 
> -      DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s
> hash of image is found in DBX.\n", mHashTypeStr));
> 
> -      IsVerified = FALSE;
> 
> -      break;
> 
> -    }
> 
> -
> 
>      if (!IsVerified) {
> 
>        DbStatus = IsSignatureFoundInDatabase (
> 
>                     EFI_IMAGE_SECURITY_DATABASE,
> 
> --
> 2.31.1