From: "Wang, Jian J" <jian.j.wang@intel.com>
To: devel@edk2.groups.io
Cc: Jiewen Yao <jiewen.yao@intel.com>, Chao Zhang <chao.b.zhang@intel.com>
Subject: [PATCH v2 05/10] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code(CVE-2019-14575)
Date: Fri, 14 Feb 2020 15:27:40 +0800 [thread overview]
Message-ID: <20200214072745.1570-6-jian.j.wang@intel.com> (raw)
In-Reply-To: <20200214072745.1570-1-jian.j.wang@intel.com>
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.yao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@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
next prev parent reply other threads:[~2020-02-14 7:27 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-14 7:27 [PATCH v2 00/10] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
2020-02-14 7:27 ` [PATCH v2 01/10] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575) Wang, Jian J
2020-02-14 7:27 ` [PATCH v2 02/10] SecurityPkg/DxeImageVerificationLib: reject CertStack.CertNumber==0 per DBX(CVE-2019-14575) Wang, Jian J
2020-02-14 7:27 ` [PATCH v2 03/10] SecurityPkg/DxeImageVerificationLib: fix wrong fetch dbx in IsAllowedByDb(CVE-2019-14575) Wang, Jian J
2020-02-14 7:27 ` [PATCH v2 04/10] SecurityPkg/DxeImageVerificationLib: avoid bypass in fetching dbx(CVE-2019-14575) Wang, Jian J
2020-02-14 7:27 ` Wang, Jian J [this message]
2020-02-14 7:27 ` [PATCH v2 06/10] SecurityPkg/DxeImageVerificationLib: Differentiate error/search result (1)(CVE-2019-14575) Wang, Jian J
2020-02-14 7:47 ` Yao, Jiewen
2020-02-14 7:27 ` [PATCH v2 07/10] SecurityPkg/DxeImageVerificationLib: tighten default result(CVE-2019-14575) Wang, Jian J
2020-02-14 7:27 ` [PATCH v2 08/10] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
2020-02-14 7:27 ` [PATCH v2 09/10] SecurityPkg/DxeImageVerificationLib: Differentiate error/search result (2)(CVE-2019-14575) Wang, Jian J
2020-02-14 7:27 ` [PATCH v2 10/10] SecurityPkg/DxeImageVerificationLib: change IsCertHashFoundInDatabase name(CVE-2019-14575) Wang, Jian J
2020-02-14 7:46 ` Yao, Jiewen
2020-02-17 7:48 ` [edk2-devel] [PATCH v2 00/10] Fix false negative issue in DxeImageVerificationHandler Laszlo Ersek
2020-02-17 7:51 ` Wang, Jian J
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200214072745.1570-6-jian.j.wang@intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox