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

[Marvin Häuser]

Hey Jiewen,

Right, I meant to ask about this and forgot (sorry, I sent out a bit 
less than 30 patches yesterday :) ).
Why do we record and potentially defer the loading of images that are 
distrusted by dbx?
I would expect any image explicitly distrusted (not just untrusted) to 
be rejected and unloaded immediately.

Sorry if I got wrong what is happening!

Best regards,
Marvin

On 09/08/2021 04:48, Yao, Jiewen wrote:
> 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 at posteo.de>
>> Sent: Monday, August 9, 2021 3:40 AM
>> To: devel at edk2.groups.io
>> Cc: Yao, Jiewen <jiewen.yao at intel.com>; Wang, Jian J <jian.j.wang at intel.com>;
>> Xu, Min M <min.m.xu at intel.com>; Vitaly Cheptsov <vit9696 at 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 at intel.com>
>> Cc: Jian J Wang <jian.j.wang at intel.com>
>> Cc: Min Xu <min.m.xu at intel.com>
>> Cc: Vitaly Cheptsov <vit9696 at protonmail.com>
>> Signed-off-by: Marvin Häuser <mhaeuser at 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
>
>
> 
>
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78919): https://edk2.groups.io/g/devel/message/78919
Mute This Topic: https://groups.io/mt/84754063/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-