Good enhancement. Reviewed-by: Jiewen Yao <jiewen....@intel.com>
> -----Original Message----- > From: Wang, Jian J <jian.j.w...@intel.com> > Sent: Thursday, February 6, 2020 10:19 PM > To: devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen....@intel.com>; Zhang, Chao B > <chao.b.zh...@intel.com> > Subject: [PATCH 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx > fetching code in IsAllowedByDb(CVE-2019-14575) > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608 > > The dbx fetching code inside the while/for-loop causes code hard to > understand. Since there's no need to get dbx more than once, this patch > simplify the code logic by moving related code to be outside the while- > loop. db fetching code is also refined accordingly to reduce the indent > level of code. > > More comments are also added or refined to explain more details. > > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Chao Zhang <chao.b.zh...@intel.com> > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > --- > .../DxeImageVerificationLib.c | 144 ++++++++++-------- > 1 file changed, 83 insertions(+), 61 deletions(-) > > diff --git > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index ed5dbf26b0..8739d1fa29 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > @@ -1412,76 +1412,92 @@ IsAllowedByDb ( > RootCertSize = 0; > > VerifyStatus = FALSE; > > > > + // > > + // Fetch 'db' content. If 'db' doesn't exist or encounters problem to get > the > > + // data, return not-allowed-by-db (FALSE). > > + // > > DataSize = 0; > > Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, > &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL); > > - if (Status == EFI_BUFFER_TOO_SMALL) { > > - Data = (UINT8 *) AllocateZeroPool (DataSize); > > - if (Data == NULL) { > > - return VerifyStatus; > > + ASSERT (EFI_ERROR (Status)); > > + if (Status != EFI_BUFFER_TOO_SMALL) { > > + return VerifyStatus; > > + } > > + > > + Data = (UINT8 *) AllocateZeroPool (DataSize); > > + if (Data == NULL) { > > + return VerifyStatus; > > + } > > + > > + Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, > &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data); > > + if (EFI_ERROR (Status)) { > > + goto Done; > > + } > > + > > + // > > + // Fetch 'dbx' content. If 'dbx' doesn't exist, continue to check 'db'. > > + // If any other errors occured, no need to check 'db' but just return > > + // not-allowed-by-db (FALSE) to avoid bypass. > > + // > > + DbxDataSize = 0; > > + Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, > &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL); > > + ASSERT (EFI_ERROR (Status)); > > + if (Status != EFI_BUFFER_TOO_SMALL) { > > + if (Status != EFI_NOT_FOUND) { > > + goto Done; > > + } > > + // > > + // 'dbx' does not exist. Continue to check 'db'. > > + // > > + } else { > > + // > > + // 'dbx' exists. Get its content. > > + // > > + DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize); > > + if (DbxData == NULL) { > > + goto Done; > > } > > > > - Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, > &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data); > > + Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, > &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData); > > if (EFI_ERROR (Status)) { > > goto Done; > > } > > + } > > > > - // > > - // Find X509 certificate in Signature List to verify the signature in > pkcs7 signed > data. > > - // > > - CertList = (EFI_SIGNATURE_LIST *) Data; > > - while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) { > > - if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) { > > - CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof > (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize); > > - CertCount = (CertList->SignatureListSize - sizeof > (EFI_SIGNATURE_LIST) - > CertList->SignatureHeaderSize) / CertList->SignatureSize; > > + // > > + // Find X509 certificate in Signature List to verify the signature in > pkcs7 signed > data. > > + // > > + CertList = (EFI_SIGNATURE_LIST *) Data; > > + while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) { > > + if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) { > > + CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof > (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize); > > + CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) > - > CertList->SignatureHeaderSize) / CertList->SignatureSize; > > > > - for (Index = 0; Index < CertCount; Index++) { > > - // > > - // Iterate each Signature Data Node within this CertList for > verify. > > - // > > - RootCert = CertData->SignatureData; > > - RootCertSize = CertList->SignatureSize - sizeof (EFI_GUID); > > + for (Index = 0; Index < CertCount; Index++) { > > + // > > + // Iterate each Signature Data Node within this CertList for verify. > > + // > > + RootCert = CertData->SignatureData; > > + RootCertSize = CertList->SignatureSize - sizeof (EFI_GUID); > > > > + // > > + // Call AuthenticodeVerify library to Verify Authenticode struct. > > + // > > + VerifyStatus = AuthenticodeVerify ( > > + AuthData, > > + AuthDataSize, > > + RootCert, > > + RootCertSize, > > + mImageDigest, > > + mImageDigestSize > > + ); > > + if (VerifyStatus) { > > // > > - // Call AuthenticodeVerify library to Verify Authenticode struct. > > + // The image is signed and its signature is found in 'db'. > > // > > - VerifyStatus = AuthenticodeVerify ( > > - AuthData, > > - AuthDataSize, > > - RootCert, > > - RootCertSize, > > - mImageDigest, > > - mImageDigestSize > > - ); > > - if (VerifyStatus) { > > + if (DbxData != NULL) { > > // > > // Here We still need to check if this RootCert's Hash is revoked > > // > > - DbxDataSize = 0; > > - Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, > &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL); > > - if (Status != EFI_BUFFER_TOO_SMALL) { > > - if (Status != EFI_NOT_FOUND) { > > - VerifyStatus = FALSE; > > - } > > - goto Done; > > - } > > - DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize); > > - if (DbxData == NULL) { > > - // > > - // Force not-allowed-by-db to avoid bypass > > - // > > - VerifyStatus = FALSE; > > - goto Done; > > - } > > - > > - Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, > &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData); > > - if (EFI_ERROR (Status)) { > > - // > > - // Force not-allowed-by-db to avoid bypass > > - // > > - VerifyStatus = FALSE; > > - goto Done; > > - } > > - > > if (IsCertHashFoundInDatabase (RootCert, RootCertSize, > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) { > > // > > // Check the timestamp signature and signing time to determine > if the > RootCert can be trusted. > > @@ -1491,17 +1507,23 @@ IsAllowedByDb ( > DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is > signed and > signature is accepted by DB, but its root cert failed the timestamp > check.\n")); > > } > > } > > - > > - goto Done; > > } > > > > - CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList- > >SignatureSize); > > + // > > + // There's no 'dbx' to check revocation time against (must-be > pass), > > + // or, there's revocation time found in 'dbx' and checked againt > 'dbt' > > + // (maybe pass or fail, depending on timestamp compare result). > Either > > + // way the verification job has been completed at this point. > > + // > > + goto Done; > > } > > - } > > > > - DataSize -= CertList->SignatureListSize; > > - CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList- > >SignatureListSize); > > + CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList- > >SignatureSize); > > + } > > } > > + > > + DataSize -= CertList->SignatureListSize; > > + CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList- > >SignatureListSize); > > } > > > > Done: > > -- > 2.24.0.windows.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54350): https://edk2.groups.io/g/devel/message/54350 Mute This Topic: https://groups.io/mt/71023422/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
