From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from huawei.com (huawei.com [45.249.212.35]) by mx.groups.io with SMTP id smtpd.web10.13851.1597391663344609996 for ; Fri, 14 Aug 2020 00:54:23 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.35, mailfrom: xiewenyi2@huawei.com) Received: from DGGEMS414-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id AF3357977A48EF3838C9; Fri, 14 Aug 2020 15:54:14 +0800 (CST) Received: from [127.0.0.1] (10.174.152.231) by DGGEMS414-HUB.china.huawei.com (10.3.19.214) with Microsoft SMTP Server id 14.3.487.0; Fri, 14 Aug 2020 15:54:06 +0800 From: "wenyi,xie" Subject: Re: [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset To: Laszlo Ersek , , , CC: , References: <1597319741-59646-1-git-send-email-xiewenyi2@huawei.com> <1597319741-59646-2-git-send-email-xiewenyi2@huawei.com> Message-ID: <024b1279-609d-fefa-8535-5af072815bf8@huawei.com> Date: Fri, 14 Aug 2020 15:53:43 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.0.1 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.174.152.231] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit To Laszlo, Thank you for your detailed description, I agree with what you analyzed and I'm OK with your patches, it's correct and much simpler. To Jiewen, Sorry, I don't have environment to reproduce the issue. Thanks Wenyi On 2020/8/14 2:50, Laszlo Ersek wrote: > On 08/13/20 13:55, Wenyi Xie wrote: >> 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); > > I disagree with this patch. > > The primary reason for my disagreement is that the bug report > is inexact, and > so this patch tries to fix the wrong thing. > > With edk2 master at commit 65904cdbb33c, it is *not* possible to > overflow the OffSet variable to zero with "WinCertificate->dwLength" > *purely*, and cause an endless loop. Note that we have (at commit > 65904cdbb33c): > > for (OffSet = SecDataDir->VirtualAddress; > OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); > OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) { > WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); > if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) || > (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) { > break; > } > > The last sub-condition checks whether the Security Data Directory has > enough room left for "WinCertificate->dwLength". If not, then we break > out of the loop. > > If we *do* have enough room, that is: > > (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) >= WinCertificate->dwLength > > then we have (by adding OffSet to both sides): > > SecDataDir->VirtualAddress + SecDataDir->Size >= OffSet + WinCertificate->dwLength > > The left hand side is a known-good UINT32, and so incrementing OffSet (a > UINT32) *solely* by "WinCertificate->dwLength" (also a UINT32) does not > cause an overflow. > > Instead, the problem is with the alignment. The "if" statement checks > whether we have enough room for "dwLength", but then "OffSet" is > advanced by "dwLength" *aligned up* to the next multiple of 8. And that > may indeed cause various overflows. > > Now, the main problem with the present patch is that it does not fix one > of those overflows. Namely, consider that "dwLength" is very close to > MAX_UINT32 (or even think it's exactly MAX_UINT32). Then aligning it up > to the next multiple of 8 will yield 0. In other words, "AlignedLength" > will be zero. > > And when that happens, there's going to be an infinite loop just the > same: "OffSet" will not be zero, but it will be *stuck*. The > SafeUint32Add() call at the bottom will succeed, but it will not change > the value of "OffSet". > > More at the bottom. > > >> 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)) { >> > > There are other (smaller) reasons why I dislike this patch: > > - The "IsAuthDataAssigned" variable is superfluous; we could use the > existent "AuthData" variable (with a NULL-check and a NULL-assignment) > similarly. > > - The patch complicates / reorganizes the control flow needlessly. This > complication originates from placing the checked "OffSet" increment at > the bottom of the loop, which then requires the removal of all the > "continue" statements. But we don't need to check-and-increment at the > bottom. We can keep the increment inside the "for" statement, only > extend the *existent* room check (which I've quoted) to take the > alignment into account as well. If there is enough room for the > alignment in the security data directory, then that guarantees there > won't be a UINT32 overflow either. > > All in all, I'm proposing the following three patches instead. The first > two patches are preparation, the last patch is the fix. > > Patch#1: > >> From 11af0a104d34d39bf1b1aab256428ae4edbddd77 Mon Sep 17 00:00:00 2001 >> From: Laszlo Ersek >> Date: Thu, 13 Aug 2020 19:11:39 +0200 >> Subject: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract >> SecDataDirEnd, SecDataDirLeft >> >> The following two quantities: >> >> SecDataDir->VirtualAddress + SecDataDir->Size >> SecDataDir->VirtualAddress + SecDataDir->Size - OffSet >> >> are used multiple times in DxeImageVerificationHandler(). Introduce helper >> variables for them: "SecDataDirEnd" and "SecDataDirLeft", respectively. >> This saves us multiple calculations and significantly simplifies the code. >> >> Note that all three summands above have type UINT32, therefore the new >> variables are also of type UINT32. >> >> This patch does not change behavior. >> >> (Note that the code already handles the case when the >> >> SecDataDir->VirtualAddress + SecDataDir->Size >> >> UINT32 addition overflows -- namely, in that case, the certificate loop is >> never entered, and the corruption check right after the loop fires.) >> >> Signed-off-by: Laszlo Ersek >> --- >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> index 36b87e16d53d..8761980c88aa 100644 >> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> @@ -1652,6 +1652,8 @@ DxeImageVerificationHandler ( >> UINT8 *AuthData; >> UINTN AuthDataSize; >> EFI_IMAGE_DATA_DIRECTORY *SecDataDir; >> + UINT32 SecDataDirEnd; >> + UINT32 SecDataDirLeft; >> UINT32 OffSet; >> CHAR16 *NameStr; >> RETURN_STATUS PeCoffStatus; >> @@ -1849,12 +1851,14 @@ DxeImageVerificationHandler ( >> // "Attribute Certificate Table". >> // The first certificate starts at offset (SecDataDir->VirtualAddress) from the start of the file. >> // >> + SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size; >> for (OffSet = SecDataDir->VirtualAddress; >> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); >> + OffSet < SecDataDirEnd; >> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) { >> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); >> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) || >> - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) { >> + SecDataDirLeft = SecDataDirEnd - OffSet; >> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) || >> + SecDataDirLeft < WinCertificate->dwLength) { >> break; >> } >> >> @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler ( >> } >> } >> >> - if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) { >> + if (OffSet != SecDataDirEnd) { >> // >> // The Size in Certificate Table or the attribute certificate table is corrupted. >> // >> -- >> 2.19.1.3.g30247aa5d201 >> > > Patch#2: > >> From 72012c065a53582f7df695e7b9730c45f49226c6 Mon Sep 17 00:00:00 2001 >> From: Laszlo Ersek >> Date: Thu, 13 Aug 2020 19:19:06 +0200 >> Subject: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign >> WinCertificate after size check >> >> Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check only >> guards the de-referencing of the "WinCertificate" pointer. It does not >> guard the calculation of hte pointer itself: >> >> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); >> >> This is wrong; if we don't know for sure that we have enough room for a >> WIN_CERTIFICATE, then even creating such a pointer, not just >> de-referencing it, may invoke undefined behavior. >> >> Move the pointer calculation after the size check. >> >> Signed-off-by: Laszlo Ersek >> --- >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> index 8761980c88aa..461ed7cfb5ac 100644 >> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler ( >> for (OffSet = SecDataDir->VirtualAddress; >> OffSet < SecDataDirEnd; >> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) { >> - WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); >> SecDataDirLeft = SecDataDirEnd - OffSet; >> - if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) || >> - SecDataDirLeft < WinCertificate->dwLength) { >> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) { >> + break; >> + } >> + WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); >> + if (SecDataDirLeft < WinCertificate->dwLength) { >> break; >> } >> >> -- >> 2.19.1.3.g30247aa5d201 >> > > Patch#3: > >> From 0bbba15b84f8f9f2cdc770a89f418aaec6cfb31e Mon Sep 17 00:00:00 2001 >> From: Laszlo Ersek >> Date: Thu, 13 Aug 2020 19:34:33 +0200 >> Subject: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment >> overflow (CVE-2019-14562) >> >> The DxeImageVerificationHandler() function currently checks whether >> "SecDataDir" has enough room for "WinCertificate->dwLength". However, for >> advancing "OffSet", "WinCertificate->dwLength" is aligned to the next >> multiple of 8. If "WinCertificate->dwLength" is large enough, the >> alignment will return 0, and "OffSet" will be stuck at the same value. >> >> Check whether "SecDataDir" has room left for both >> "WinCertificate->dwLength" and the alignment. >> >> Signed-off-by: Laszlo Ersek >> --- >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> index 461ed7cfb5ac..e38eb981b7a0 100644 >> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler ( >> break; >> } >> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); >> - if (SecDataDirLeft < WinCertificate->dwLength) { >> + if (SecDataDirLeft < WinCertificate->dwLength || >> + (SecDataDirLeft - WinCertificate->dwLength < >> + ALIGN_SIZE (WinCertificate->dwLength))) { >> break; >> } >> >> -- >> 2.19.1.3.g30247aa5d201 >> > > If Wenyi and the reviewers are OK with these patches, I can submit them > as a standalone patch series. > > Note that I do not have any reproducer for the issue; the best testing > that I could offer would be some light-weight Secure Boot regression > tests. > > Thanks > Laszlo > > > . >