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.web12.8908.1597322636867049975 for ; Thu, 13 Aug 2020 05:43:57 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.35, mailfrom: xiewenyi2@huawei.com) Received: from DGGEMS407-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id 5261D82F1AA05A4468BF; Thu, 13 Aug 2020 20:43:54 +0800 (CST) Received: from [127.0.0.1] (10.174.153.25) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.487.0; Thu, 13 Aug 2020 20:43:45 +0800 Subject: Re: [edk2-devel] [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset(CVE-2019-14562) To: Laszlo Ersek , , , CC: , References: <1597215886-48713-1-git-send-email-xiewenyi2@huawei.com> <1597215886-48713-2-git-send-email-xiewenyi2@huawei.com> From: "wenyi,xie" Message-ID: Date: Thu, 13 Aug 2020 20:43:37 +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.153.25] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit On 2020/8/12 20:23, Laszlo Ersek wrote: > On 08/12/20 09:04, wenyi,xie via groups.io 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: Chao Zhang >> Signed-off-by: Wenyi Xie >> --- >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf | 1 + >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h | 1 + >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 21 +++++++++++++++----- >> 3 files changed, 18 insertions(+), 5 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..2b42d4595f2c 100644 >> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> @@ -1658,6 +1658,9 @@ DxeImageVerificationHandler ( >> EFI_STATUS HashStatus; >> EFI_STATUS DbStatus; >> BOOLEAN IsFound; >> + UINT32 AlignedLength; >> + UINT32 Result; >> + EFI_STATUS AddStatus; >> >> SignatureList = NULL; >> SignatureListSize = 0; >> @@ -1667,6 +1670,7 @@ DxeImageVerificationHandler ( >> Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED; >> IsVerified = FALSE; >> IsFound = FALSE; >> + Result = 0; >> >> // >> // Check the image type and get policy setting. >> @@ -1850,9 +1854,9 @@ 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));) { >> 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; >> @@ -1881,7 +1885,7 @@ DxeImageVerificationHandler ( >> break; >> } >> if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) { >> - continue; >> + goto NEXT_LOOP; >> } >> AuthData = WinCertUefiGuid->CertData; >> AuthDataSize = WinCertUefiGuid->Hdr.dwLength - OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); >> @@ -1889,12 +1893,12 @@ DxeImageVerificationHandler ( >> if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) { >> break; >> } >> - continue; >> + goto NEXT_LOOP; >> } >> >> HashStatus = HashPeImageByType (AuthData, AuthDataSize); >> if (EFI_ERROR (HashStatus)) { >> - continue; >> + goto NEXT_LOOP; >> } >> >> // >> @@ -1946,6 +1950,13 @@ DxeImageVerificationHandler ( >> 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)); >> } >> } >> + >> +NEXT_LOOP: >> + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result); >> + if (EFI_ERROR (AddStatus)) { >> + break; >> + } >> + OffSet = Result; >> } >> >> if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) { >> > > (1) Please mark the patch URL > on the bugzilla ticket, > in a comment. I saw you have marked URL on the bugzilla ticket, so I didn't mark it again. > > (2) We should not use "goto" statements for such control transfers, only > for error handling. If necessary, please split the function in two, or > reorganize the loop otherwise. Ok, I removed "goto" and add a new flag IsAuthDataAssigned. When AuthData and AuthDataSize are assigned, the flag is set true. If either this flag or status of HashPeImageByType is false, then skip signature verification, only increasing offset and step to next loop. And thank you for your review. > > Thanks > Laszlo > > > . >