On 04/26/16 17:22, Cinnamon Shia wrote:
> Add DEBUG messages in DxeImageerificationLib to help debug Secure Boot image
> verification failures
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Cinnamon Shia <[email protected]>
> Reviewed-by: Samer EL-Haj-Mahmoud <[email protected]>
> ---
> .../DxeImageVerificationLib.c | 37
> +++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
... Sorry, I feel quite dumb. :(
I have now actually checked what IsAllowedByDb does, and it says:
"Check whether the image signature can be verified by the trusted
certificates in DB database."
In other words, it is not a direct hash match, but a certificate chain
match. So the variable should not be called "SigFoundInDb" after all,
but something like "SigAcceptedByCertsInDb". I feel very bad that I
realized this just now. :(
Another comment:
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 4b4d3bf..95c0007 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -13,6 +13,7 @@
> untrusted PE/COFF image and validate its data structure within this image
> buffer before use.
>
> Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.<BR>
> +(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -71,6 +72,8 @@ HASH_TABLE mHash[] = {
> { L"SHA512", 64, &mHashOidValue[32], 9, Sha512GetContextSize, Sha512Init,
> Sha512Update, Sha512Final}
> };
>
> +EFI_STRING mHashTypeStr;
> +
> /**
> SecureBoot Hook for processing image verification.
>
> @@ -340,6 +343,7 @@ HashPeImage (
> return FALSE;
> }
>
> + mHashTypeStr = mHash[HashAlg].Name;
> CtxSize = mHash[HashAlg].GetContextSize();
>
> HashCtx = AllocatePool (CtxSize);
> @@ -1782,6 +1786,7 @@ ImageVerificationInAuditMode (
> UINT32 OffSet;
> CHAR16 *FilePathStr;
> UINTN SignatureListSize;
> + BOOLEAN SigFoundInDb;
>
> SignatureList = NULL;
> WinCertificate = NULL;
> @@ -1790,6 +1795,7 @@ ImageVerificationInAuditMode (
> FilePathStr = NULL;
> Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED |
> EFI_IMAGE_EXECUTION_INITIALIZED;
> Status = EFI_ACCESS_DENIED;
> + SigFoundInDb = FALSE;
>
>
> //
> @@ -1873,6 +1879,7 @@ ImageVerificationInAuditMode (
> //
> // The information can't be got from the invalid PeImage
> //
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: PeImage invalid. Cannot
> retrieve image information.\n"));
> goto END;
> }
>
> @@ -1896,6 +1903,7 @@ ImageVerificationInAuditMode (
> //
> // It is not a valid Pe/Coff file.
> //
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Not a valid PE/COFF
> image.\n"));
> Status = EFI_ACCESS_DENIED;
> goto END;
> }
> @@ -1942,6 +1950,7 @@ ImageVerificationInAuditMode (
> // and not be reflected in the security data base "dbx".
> //
> if (!HashPeImage (HASHALG_SHA256)) {
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Failed to hash this
> image using %s.\n", mHashTypeStr));
> Status = EFI_ACCESS_DENIED;
> goto END;
> }
> @@ -1955,7 +1964,14 @@ ImageVerificationInAuditMode (
> //
> if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE,
> mImageDigest, &mCertType, mImageDigestSize)) {
> Action = EFI_IMAGE_EXECUTION_AUTH_SIG_PASSED |
> EFI_IMAGE_EXECUTION_INITIALIZED;
> + } else {
> + //
> + // Image Hash is not in DB/DBX
> + //
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is not signed
> and %s hash of image is not found in DB/DBX.\n", mHashTypeStr));
> }
> + } else {
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is not signed and
> %s hash of image is rejected by DBX.\n", mHashTypeStr));
> }
>
> //
> @@ -2029,7 +2045,9 @@ ImageVerificationInAuditMode (
> // Check the digital signature against the valid certificate in allowed
> database (db).
> //
> if (!IsForbiddenByDbx (AuthData, AuthDataSize, TRUE, FilePathStr, File))
> {
> - IsAllowedByDb (AuthData, AuthDataSize, TRUE, FilePathStr, File);
> + SigFoundInDb = IsAllowedByDb (AuthData, AuthDataSize, TRUE,
> FilePathStr, File);
> + } else {
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is signed but
> signature is rejected by DBX.\n"));
> }
>
> //
> @@ -2038,7 +2056,13 @@ ImageVerificationInAuditMode (
> if (!IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1,
> mImageDigest, &mCertType, mImageDigestSize)) {
> if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE,
> mImageDigest, &mCertType, mImageDigestSize)) {
> Action = EFI_IMAGE_EXECUTION_AUTH_SIG_PASSED |
> EFI_IMAGE_EXECUTION_INITIALIZED;
> + } else {
Okay, so at this point we know that neither DB, nor DBX contains the
direct hash of the binary. But then:
> + if (SigFoundInDb) {
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is signed but
> signature and %s hash of image are not found in DB/DBX.\n", mHashTypeStr));
this message says that *not even* the certificate chain validation
succeeded.
For that message, shouldn't the condition be the exact opposite? In
other words, (!SigFoundInDb)?
Because in DxeImageVerificationHandler():
> + }
> }
> + } else {
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is signed but %s
> hash of image is rejected by DBX.\n", mHashTypeStr));
> }
>
> //
> @@ -2259,6 +2283,7 @@ DxeImageVerificationHandler (
> //
> // The information can't be got from the invalid PeImage
> //
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: PeImage invalid. Cannot
> retrieve image information.\n"));
> goto Done;
> }
>
> @@ -2282,6 +2307,7 @@ DxeImageVerificationHandler (
> //
> // It is not a valid Pe/Coff file.
> //
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Not a valid PE/COFF
> image.\n"));
> goto Done;
> }
>
> @@ -2327,6 +2353,7 @@ DxeImageVerificationHandler (
> // and not be reflected in the security data base "dbx".
> //
> if (!HashPeImage (HASHALG_SHA256)) {
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Failed to hash this
> image using %s.\n", mHashTypeStr));
> goto Done;
> }
>
> @@ -2334,6 +2361,7 @@ DxeImageVerificationHandler (
> //
> // Image Hash is in forbidden database (DBX).
> //
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is not signed and
> %s hash of image is rejected by DBX.\n", mHashTypeStr));
> goto Done;
> }
>
> @@ -2347,6 +2375,7 @@ DxeImageVerificationHandler (
> //
> // Image Hash is not found in both forbidden and allowed database.
> //
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is not signed and
> %s hash of image is not found in DB/DBX.\n", mHashTypeStr));
> goto Done;
> }
>
> @@ -2409,6 +2438,7 @@ DxeImageVerificationHandler (
> if (IsForbiddenByDbx (AuthData, AuthDataSize, FALSE, NULL, NULL)) {
> Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED;
> VerifyStatus = EFI_ACCESS_DENIED;
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is signed but
> signature is rejected by DBX.\n"));
> break;
> }
>
> @@ -2426,11 +2456,16 @@ DxeImageVerificationHandler (
> //
> if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1,
> mImageDigest, &mCertType, mImageDigestSize)) {
> Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is signed but %s
> hash of image is rejected by DBX.\n", mHashTypeStr));
> VerifyStatus = EFI_ACCESS_DENIED;
> break;
> } else if (EFI_ERROR (VerifyStatus)) {
> if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE,
> mImageDigest, &mCertType, mImageDigestSize)) {
> VerifyStatus = EFI_SUCCESS;
> + } else {
at this point we have again established that the image digest is neither
in DBX nor in DB -- same as above, all fine.
> + if (VerifyStatus != EFI_SUCCESS) {
(This condition is guaranteed to succeed (it will evaluate to constant
TRUE), given that we are on an EFI_ERROR (VerifyStatus) branch, from above.)
> + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is signed but
> signature and %s hash of image are not found in DB/DBX.\n", mHashTypeStr));
So at this point, the message corresponds to:
- image digest is not in DBX -- okay
- image digest is not in DB -- okay
- image was *not* accepted via cert chain in DB -- also okay here. But,
this means that IsAllowedByDb() was either never reached, or it failed;
and the check in ImageVerificationInAuditMode() expresses the exact
opposite (with the same debug message).
Summary:
- I think the variable should be renamed to "SigAcceptedByCertsInDb"
(sorry, again!)
- The assignment to SigFoundInDb (in the future:
SigAcceptedByCertsInDb) is correct.
- The debug message that depends on SigFoundInDb (in the future:
SigAcceptedByCertsInDb) is itself correct, but the condition should be
negated.
- The (VerifyStatus != EFI_SUCCESS) check added to
DxeImageVerificationHandler() is tautological (constant TRUE), it
should be removed.
Do you agree?
Thanks
Laszlo
> + }
> }
> }
> }
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
