Reviewed-by: Samer El-Haj-Mahmoud <el...@hpe.com> Chao or Laszlo, will one of you commit this please?
Thanks, --Samer -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Tuesday, April 26, 2016 12:17 PM To: Shia, Cinnamon <cinnamon.s...@hpe.com>; edk2-de...@ml01.01.org Cc: El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com>; chao.b.zh...@intel.com Subject: Re: [PATCH v3] SecurityPkg/DxeImageVerificationLib: Add DEBUG messages for image verification failures On 04/26/16 18:51, 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 <cinnamon.s...@hpe.com> > Reviewed-by: Samer EL-Haj-Mahmoud <el...@hpe.com> > --- > .../DxeImageVerificationLib.c | 49 > ++++++++++++++++++---- > 1 file changed, 41 insertions(+), 8 deletions(-) Looks good to me, thank you. I'm not a crypto expert, so I'll leave Reviewed-by tags to others. But, I'm willing to give an: Acked-by: Laszlo Ersek <ler...@redhat.com> Also, I think Samer should re-review this patch (v3). Thanks! Laszlo > diff --git > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib. > c > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib. > c > index 4b4d3bf..ee94dce 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,14 +1786,16 @@ ImageVerificationInAuditMode ( > UINT32 OffSet; > CHAR16 *FilePathStr; > UINTN SignatureListSize; > + BOOLEAN SigAcceptedByCertsInDb; > > - SignatureList = NULL; > - WinCertificate = NULL; > - SecDataDir = NULL; > - PkcsCertData = NULL; > - FilePathStr = NULL; > - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED | > EFI_IMAGE_EXECUTION_INITIALIZED; > - Status = EFI_ACCESS_DENIED; > + SignatureList = NULL; > + WinCertificate = NULL; > + SecDataDir = NULL; > + PkcsCertData = NULL; > + FilePathStr = NULL; > + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED | > EFI_IMAGE_EXECUTION_INITIALIZED; > + Status = EFI_ACCESS_DENIED; > + SigAcceptedByCertsInDb = 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); > + SigAcceptedByCertsInDb = 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 { > + if (!SigAcceptedByCertsInDb) { > + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is signed but > signature and %s hash of image are not found in DB/DBX.\n", mHashTypeStr)); > + } > } > + } 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,14 @@ 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 { > + DEBUG ((DEBUG_ERROR, "DxeImageVerificationLib: Image is > + signed but signature and %s hash of image are not found in > + DB/DBX.\n", mHashTypeStr)); > } > } > } > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
