From: "Zhang, Chao B" <chao.b.zhang@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"Wang, Jian J" <jian.j.wang@intel.com>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [edk2-devel] [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575)
Date: Thu, 13 Feb 2020 09:02:37 +0000 [thread overview]
Message-ID: <713ae40e107f48409602006ff45e1b4a@intel.com> (raw)
In-Reply-To: <20200206141933.356-10-jian.j.wang@intel.com>
Ack-by : Chao Zhang <chao.b.zhang@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wang, Jian J
Sent: Thursday, February 6, 2020 10:20 PM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: [edk2-devel] [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
To avoid false-negative issue in check hash against dbx, both error condition (as return value) and check result (as out parameter) of
IsSignatureFoundInDatabase() are added. So the caller of this function will know exactly if a failure is caused by a black list hit or other error happening, and enforce a more secure operation to prevent secure boot from being bypassed. For a white list check (db), there's no such necessity.
All intermediate results inside this function will be checked and returned immediately upon any failure or error, like out-of-resource, hash calculation error or certificate retrieval failure.
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>
---
.../DxeImageVerificationLib.c | 77 ++++++++++++++-----
1 file changed, 58 insertions(+), 19 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 5b7a67f811..8e599ca0be 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLi
+++ b.c
@@ -955,17 +955,19 @@ Done:
@param[in] Signature Pointer to signature that is searched for. @param[in] CertType Pointer to hash algorithm. @param[in] SignatureSize Size of Signature.+ @param[out] IsFound Search result. Only valid if EFI_SUCCESS returned - @return TRUE Found the signature in the variable database.- @return FALSE Not found the signature in the variable database.+ @retval EFI_SUCCESS Finished the search without any error.+ @retval Others Error occurred in the search of database. **/-BOOLEAN+EFI_STATUS IsSignatureFoundInDatabase (- IN CHAR16 *VariableName,- IN UINT8 *Signature,- IN EFI_GUID *CertType,- IN UINTN SignatureSize+ IN CHAR16 *VariableName,+ IN UINT8 *Signature,+ IN EFI_GUID *CertType,+ IN UINTN SignatureSize,+ OUT BOOLEAN *IsFound ) { EFI_STATUS Status;@@ -975,22 +977,28 @@ IsSignatureFoundInDatabase (
UINT8 *Data; UINTN Index; UINTN CertCount;- BOOLEAN IsFound; // // Read signature database variable. //- IsFound = FALSE;+ *IsFound = FALSE; Data = NULL; DataSize = 0; Status = gRT->GetVariable (VariableName, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL); if (Status != EFI_BUFFER_TOO_SMALL) {- return FALSE;+ if (Status == EFI_NOT_FOUND) {+ //+ // No database, no need to search.+ //+ Status = EFI_SUCCESS;+ }++ return Status; } Data = (UINT8 *) AllocateZeroPool (DataSize); if (Data == NULL) {- return FALSE;+ return EFI_OUT_OF_RESOURCES; } Status = gRT->GetVariable (VariableName, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, Data);@@ -1010,7 +1018,7 @@ IsSignatureFoundInDatabase (
// // Find the signature in database. //- IsFound = TRUE;+ *IsFound = TRUE; // // Entries in UEFI_IMAGE_SECURITY_DATABASE that are used to validate image should be measured //@@ -1023,7 +1031,7 @@ IsSignatureFoundInDatabase (
Cert = (EFI_SIGNATURE_DATA *) ((UINT8 *) Cert + CertList->SignatureSize); } - if (IsFound) {+ if (*IsFound) { break; } }@@ -1037,7 +1045,7 @@ Done:
FreePool (Data); } - return IsFound;+ return Status; } /**@@ -1642,6 +1650,8 @@ DxeImageVerificationHandler (
CHAR16 *NameStr; RETURN_STATUS PeCoffStatus; EFI_STATUS HashStatus;+ EFI_STATUS DbStatus;+ BOOLEAN IsFound; SignatureList = NULL; SignatureListSize = 0;@@ -1650,7 +1660,7 @@ DxeImageVerificationHandler (
PkcsCertData = NULL; Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED; IsVerified = FALSE;-+ IsFound = FALSE; // // Check the image type and get policy setting.@@ -1792,7 +1802,14 @@ DxeImageVerificationHandler (
goto Failed; } - if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {+ DbStatus = IsSignatureFoundInDatabase (+ EFI_IMAGE_SECURITY_DATABASE1,+ mImageDigest,+ &mCertType,+ mImageDigestSize,+ &IsFound+ );+ if (EFI_ERROR (DbStatus) || IsFound) { // // Image Hash is in forbidden database (DBX). //@@ -1800,7 +1817,14 @@ DxeImageVerificationHandler (
goto Failed; } - if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {+ DbStatus = IsSignatureFoundInDatabase (+ EFI_IMAGE_SECURITY_DATABASE,+ mImageDigest,+ &mCertType,+ mImageDigestSize,+ &IsFound+ );+ if (!EFI_ERROR (DbStatus) && IsFound) { // // Image Hash is in allowed database (DB). //@@ -1888,14 +1912,29 @@ DxeImageVerificationHandler (
// // Check the image's hash value. //- if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {+ DbStatus = IsSignatureFoundInDatabase (+ EFI_IMAGE_SECURITY_DATABASE1,+ mImageDigest,+ &mCertType,+ mImageDigestSize,+ &IsFound+ );+ if (EFI_ERROR (DbStatus) || IsFound) { Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND; DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s hash of image is found in DBX.\n", mHashTypeStr)); IsVerified = FALSE; break; }+ if (!IsVerified) {- if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {+ DbStatus = IsSignatureFoundInDatabase (+ EFI_IMAGE_SECURITY_DATABASE,+ mImageDigest,+ &mCertType,+ mImageDigestSize,+ &IsFound+ );+ if (!EFI_ERROR (DbStatus) && IsFound) { IsVerified = TRUE; } else { DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature is not allowed by DB and %s hash of image is not found in DB/DBX.\n", mHashTypeStr));--
2.24.0.windows.2
-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53875): https://edk2.groups.io/g/devel/message/53875
Mute This Topic: https://groups.io/mt/71023427/1790519
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [chao.b.zhang@intel.com] -=-=-=-=-=-=
next prev parent reply other threads:[~2020-02-13 9:02 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
2020-02-06 14:19 ` [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575) Wang, Jian J
2020-02-13 9:34 ` Yao, Jiewen
2020-02-13 16:43 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-02-06 14:19 ` [PATCH 2/9] SecurityPkg/DxeImageVerificationLib: reject CertStack.CertNumber==0 per DBX(CVE-2019-14575) Wang, Jian J
2020-02-13 9:36 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575) Wang, Jian J
2020-02-13 9:38 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 4/9] SecurityPkg/DxeImageVerificationLib: avoid bypass in " Wang, Jian J
2020-02-13 9:39 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code " Wang, Jian J
2020-02-13 9:44 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575) Wang, Jian J
2020-02-13 10:11 ` Yao, Jiewen
2020-02-13 15:07 ` Wang, Jian J
2020-02-14 0:54 ` Yao, Jiewen
2020-02-14 3:31 ` Wang, Jian J
2020-02-14 3:33 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 7/9] SecurityPkg/DxeImageVerificationLib: tighten default result of IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
2020-02-13 10:13 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
2020-02-13 10:14 ` Yao, Jiewen
2020-02-13 16:56 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-02-06 14:19 ` [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575) Wang, Jian J
2020-02-13 9:02 ` Zhang, Chao B [this message]
2020-02-13 10:20 ` Yao, Jiewen
2020-02-13 1:53 ` [edk2-devel] [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Liming Gao
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=713ae40e107f48409602006ff45e1b4a@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