From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from huawei.com (huawei.com [45.249.212.191]) by mx.groups.io with SMTP id smtpd.web12.8328.1597319991865720294 for ; Thu, 13 Aug 2020 04:59:52 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.191, mailfrom: xiewenyi2@huawei.com) Received: from DGGEMS407-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id 764F7BC6A687291F4260; Thu, 13 Aug 2020 19:59:42 +0800 (CST) Received: from HGH1000039998.huawei.com (10.184.68.188) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.487.0; Thu, 13 Aug 2020 19:59:35 +0800 From: "wenyi,xie" To: , , , CC: , Subject: [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset Date: Thu, 13 Aug 2020 19:55:41 +0800 Message-ID: <1597319741-59646-2-git-send-email-xiewenyi2@huawei.com> X-Mailer: git-send-email 2.8.1 In-Reply-To: <1597319741-59646-1-git-send-email-xiewenyi2@huawei.com> References: <1597319741-59646-1-git-send-email-xiewenyi2@huawei.com> MIME-Version: 1.0 X-Originating-IP: [10.184.68.188] X-CFilter-Loop: Reflected Content-Type: text/plain REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215 There is an integer overflow vulnerability in DxeImageVerificationHandler function when parsing the PE files attribute certificate table. In cases where WinCertificate->dwLength is sufficiently large, it's possible to overflow Offset back to 0 causing an endless loop. Check offset inbetween VirtualAddress and VirtualAddress + Size. Using SafeintLib to do offset addition with result check. Cc: Jiewen Yao Cc: Jian J Wang Cc: Laszlo Ersek Signed-off-by: Wenyi Xie --- SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf | 1 + SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h | 1 + SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 111 +++++++++++--------- 3 files changed, 63 insertions(+), 50 deletions(-) diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf index 1e1a639857e0..a7ac4830b3d4 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf @@ -53,6 +53,7 @@ [LibraryClasses] SecurityManagementLib PeCoffLib TpmMeasurementLib + SafeIntLib [Protocols] gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h index 17955ff9774c..060273917d5d 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include #include #include +#include #include #include #include diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c index 36b87e16d53d..dbc03e28c05b 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c @@ -1658,6 +1658,10 @@ DxeImageVerificationHandler ( EFI_STATUS HashStatus; EFI_STATUS DbStatus; BOOLEAN IsFound; + UINT32 AlignedLength; + UINT32 Result; + EFI_STATUS AddStatus; + BOOLEAN IsAuthDataAssigned; SignatureList = NULL; SignatureListSize = 0; @@ -1667,6 +1671,7 @@ DxeImageVerificationHandler ( Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED; IsVerified = FALSE; IsFound = FALSE; + Result = 0; // // Check the image type and get policy setting. @@ -1850,9 +1855,10 @@ DxeImageVerificationHandler ( // The first certificate starts at offset (SecDataDir->VirtualAddress) from the start of the file. // for (OffSet = SecDataDir->VirtualAddress; - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); - OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) { + (OffSet >= SecDataDir->VirtualAddress) && (OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size));) { + IsAuthDataAssigned = FALSE; WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); + AlignedLength = WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength); if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) || (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) { break; @@ -1872,6 +1878,8 @@ DxeImageVerificationHandler ( } AuthData = PkcsCertData->CertData; AuthDataSize = PkcsCertData->Hdr.dwLength - sizeof(PkcsCertData->Hdr); + IsAuthDataAssigned = TRUE; + HashStatus = HashPeImageByType (AuthData, AuthDataSize); } else if (WinCertificate->wCertificateType == WIN_CERT_TYPE_EFI_GUID) { // // The certificate is formatted as WIN_CERTIFICATE_UEFI_GUID which is described in UEFI Spec. @@ -1880,72 +1888,75 @@ DxeImageVerificationHandler ( if (WinCertUefiGuid->Hdr.dwLength <= OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData)) { break; } - if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) { - continue; + if (CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) { + AuthData = WinCertUefiGuid->CertData; + AuthDataSize = WinCertUefiGuid->Hdr.dwLength - OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); + IsAuthDataAssigned = TRUE; + HashStatus = HashPeImageByType (AuthData, AuthDataSize); } - AuthData = WinCertUefiGuid->CertData; - AuthDataSize = WinCertUefiGuid->Hdr.dwLength - OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); } else { if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) { break; } - continue; } - HashStatus = HashPeImageByType (AuthData, AuthDataSize); - if (EFI_ERROR (HashStatus)) { - continue; - } - - // - // Check the digital signature against the revoked certificate in forbidden database (dbx). - // - if (IsForbiddenByDbx (AuthData, AuthDataSize)) { - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED; - IsVerified = FALSE; - break; - } - - // - // Check the digital signature against the valid certificate in allowed database (db). - // - if (!IsVerified) { - if (IsAllowedByDb (AuthData, AuthDataSize)) { - IsVerified = TRUE; + if (IsAuthDataAssigned && !EFI_ERROR (HashStatus)) { + // + // Check the digital signature against the revoked certificate in forbidden database (dbx). + // + if (IsForbiddenByDbx (AuthData, AuthDataSize)) { + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED; + IsVerified = FALSE; + break; } - } - // - // Check the image's hash value. - // - 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; - } + // + // Check the digital signature against the valid certificate in allowed database (db). + // + if (!IsVerified) { + if (IsAllowedByDb (AuthData, AuthDataSize)) { + IsVerified = TRUE; + } + } - if (!IsVerified) { + // + // Check the image's hash value. + // DbStatus = IsSignatureFoundInDatabase ( - EFI_IMAGE_SECURITY_DATABASE, + EFI_IMAGE_SECURITY_DATABASE1, 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)); + 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) { + 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)); + } + } + } + + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result); + if (EFI_ERROR (AddStatus)) { + break; } + OffSet = Result; } if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) { -- 2.20.1.windows.1