Agreed. Will fixed it in the v3 patch. Thanks, Cinnamon Shia
-----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Tuesday, April 26, 2016 11:54 PM To: Shia, Cinnamon <[email protected]>; [email protected] Cc: El-Haj-Mahmoud, Samer <[email protected]>; [email protected] Subject: Re: [PATCH v2] SecurityPkg/DxeImageVerificationLib: Add DEBUG messages for image verification failures 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/DxeImageVerification > +++ Lib.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
