Re: [edk2-devel] [PATCH 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code in IsAllowedByDb(CVE-2019-14575)
Good enhancement.
Reviewed-by: Jiewen Yao
> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, February 6, 2020 10:19 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen ; Zhang, Chao B
>
> 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
> Cc: Chao Zhang
> Signed-off-by: Jian J Wang
> ---
> .../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,
> , NULL, , 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,
> , NULL, , (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,
> , NULL, , 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,
> , NULL, , (VOID *) Data);
>
> +Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> , NULL, , (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 (>SignatureType, )) {
>
> -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 (>SignatureType, )) {
>
> + 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
[edk2-devel] [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
Cc: Chao Zhang
Signed-off-by: Jian J Wang
---
.../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,
, NULL, , 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,
, NULL, , (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,
, NULL, , 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,
, NULL, , (VOID *) Data);
+Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
, NULL, , (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 (>SignatureType, )) {
-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 (>SignatureType, )) {
+ 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 =
