Skip to content

Commit

Permalink
SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code (C…
Browse files Browse the repository at this point in the history 
…VE-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 <[email protected]>
Cc: Chao Zhang <[email protected]>
Signed-off-by: Jian J Wang <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
  • Loading branch information
@mergify
Jian J Wang authored and mergify[bot] committed Feb 19, 2020
1 parent 929d1a2 commit adc6898
Showing 1 changed file with 83 additions and 61 deletions.
144 changes: 83 additions & 61 deletions SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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:
Expand Down

0 comments on commit adc6898

Please sign in to comment.