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.15905.1597404562976602323 for ; Fri, 14 Aug 2020 04:29:23 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.191, mailfrom: xiewenyi2@huawei.com) Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id 3F1A410544683BA2C326; Fri, 14 Aug 2020 19:29:19 +0800 (CST) Received: from [127.0.0.1] (10.174.152.231) by DGGEMS411-HUB.china.huawei.com (10.3.19.211) with Microsoft SMTP Server id 14.3.487.0; Fri, 14 Aug 2020 19:29:10 +0800 Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset To: "Yao, Jiewen" , "devel@edk2.groups.io" , Laszlo Ersek , "Wang, Jian J" CC: "huangming23@huawei.com" , "songdongkuang@huawei.com" References: <1597319741-59646-1-git-send-email-xiewenyi2@huawei.com> <1597319741-59646-2-git-send-email-xiewenyi2@huawei.com> <024b1279-609d-fefa-8535-5af072815bf8@huawei.com> From: "wenyi,xie" Message-ID: <770a20e3-d1c5-ad9a-63ed-a16ee12dd366@huawei.com> Date: Fri, 14 Aug 2020 19:29:09 +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: quoted-printable On 2020/8/14 16:53, Yao, Jiewen wrote: >> To Jiewen, >> Sorry, I don't have environment to reproduce the issue. >=20 > Please help me understand, if you don=E2=80=99t have environment to repr= oduce the issue, how do you guarantee that your patch does fix the problem = and we don=E2=80=99t have any other vulnerabilities? Hi, Jiewen, You're right, as I can't reproduce the issue, I can't guarantee my patches= can fix the problem. And as Laszlo analyzed, my patches can't solve overflow issue indeed. Sincerely Wenyi >=20 > Thank you > Yao Jiewen >=20 >=20 >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of wenyi,xi= e >> via groups.io >> Sent: Friday, August 14, 2020 3:54 PM >> To: Laszlo Ersek ; devel@edk2.groups.io; Yao, Jiewen >> ; Wang, Jian J >> Cc: huangming23@huawei.com; songdongkuang@huawei.com >> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] >> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset >> >> 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=3D2215 >>>> >>>> There is an integer overflow vulnerability in DxeImageVerificationHan= dler >>>> function when parsing the PE files attribute certificate table. In ca= ses >>>> where WinCertificate->dwLength is sufficiently large, it's possible t= o >>>> 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.i= nf >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.i= nf >>>> index 1e1a639857e0..a7ac4830b3d4 100644 >>>> --- >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.i= nf >>>> +++ >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.i= nf >>>> @@ -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/DxeImageVerificatio= nLib.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 =3D NULL; >>>> SignatureListSize =3D 0; >>>> @@ -1667,6 +1671,7 @@ DxeImageVerificationHandler ( >>>> Action =3D EFI_IMAGE_EXECUTION_AUTH_UNTESTED; >>>> IsVerified =3D FALSE; >>>> IsFound =3D FALSE; >>>> + Result =3D 0; >>>> >>>> // >>>> // Check the image type and get policy setting. >>>> @@ -1850,9 +1855,10 @@ DxeImageVerificationHandler ( >>>> // The first certificate starts at offset (SecDataDir->VirtualAddr= ess) from the >> start of the file. >>>> // >>>> for (OffSet =3D SecDataDir->VirtualAddress; >>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); >>>> - OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE (WinCertif= icate- >>> dwLength))) { >>>> + (OffSet >=3D SecDataDir->VirtualAddress) && (OffSet < (SecDat= aDir- >>> VirtualAddress + SecDataDir->Size));) { >>>> + IsAuthDataAssigned =3D FALSE; >>>> WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + OffSet); >>>> + AlignedLength =3D WinCertificate->dwLength + ALIGN_SIZE (WinCert= ificate- >>> 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 =3D SecDataDir->VirtualAddress; >>> OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); >>> OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE (WinCertific= ate- >>> dwLength))) { >>> WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + OffSet); >>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <=3D = sizeof >> (WIN_CERTIFICATE) || >>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < Win= Certificate- >>> 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) >=3D WinCer= tificate- >>> dwLength >>> >>> then we have (by adding OffSet to both sides): >>> >>> SecDataDir->VirtualAddress + SecDataDir->Size >=3D OffSet + WinCerti= ficate- >>> dwLength >>> >>> The left hand side is a known-good UINT32, and so incrementing OffSet = (a >>> UINT32) *solely* by "WinCertificate->dwLength" (also a UINT32) does no= t >>> 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 tha= t >>> may indeed cause various overflows. >>> >>> Now, the main problem with the present patch is that it does not fix o= ne >>> of those overflows. Namely, consider that "dwLength" is very close to >>> MAX_UINT32 (or even think it's exactly MAX_UINT32). Then aligning it u= p >>> 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 chang= e >>> the value of "OffSet". >>> >>> More at the bottom. >>> >>> >>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= =3D sizeof >> (WIN_CERTIFICATE) || >>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < >> WinCertificate->dwLength) { >>>> break; >>>> @@ -1872,6 +1878,8 @@ DxeImageVerificationHandler ( >>>> } >>>> AuthData =3D PkcsCertData->CertData; >>>> AuthDataSize =3D PkcsCertData->Hdr.dwLength - sizeof(PkcsCertD= ata->Hdr); >>>> + IsAuthDataAssigned =3D TRUE; >>>> + HashStatus =3D HashPeImageByType (AuthData, AuthDataSize); >>>> } else if (WinCertificate->wCertificateType =3D=3D WIN_CERT_TYPE= _EFI_GUID) >> { >>>> // >>>> // The certificate is formatted as WIN_CERTIFICATE_UEFI_GUID w= hich is >> described in UEFI Spec. >>>> @@ -1880,72 +1888,75 @@ DxeImageVerificationHandler ( >>>> if (WinCertUefiGuid->Hdr.dwLength <=3D >> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData)) { >>>> break; >>>> } >>>> - if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Gu= id)) { >>>> - continue; >>>> + if (CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Gui= d)) { >>>> + AuthData =3D WinCertUefiGuid->CertData; >>>> + AuthDataSize =3D WinCertUefiGuid->Hdr.dwLength - >> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); >>>> + IsAuthDataAssigned =3D TRUE; >>>> + HashStatus =3D HashPeImageByType (AuthData, AuthDataSize); >>>> } >>>> - AuthData =3D WinCertUefiGuid->CertData; >>>> - AuthDataSize =3D WinCertUefiGuid->Hdr.dwLength - >> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); >>>> } else { >>>> if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) { >>>> break; >>>> } >>>> - continue; >>>> } >>>> >>>> - HashStatus =3D HashPeImageByType (AuthData, AuthDataSize); >>>> - if (EFI_ERROR (HashStatus)) { >>>> - continue; >>>> - } >>>> - >>>> - // >>>> - // Check the digital signature against the revoked certificate i= n forbidden >> database (dbx). >>>> - // >>>> - if (IsForbiddenByDbx (AuthData, AuthDataSize)) { >>>> - Action =3D EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED; >>>> - IsVerified =3D FALSE; >>>> - break; >>>> - } >>>> - >>>> - // >>>> - // Check the digital signature against the valid certificate in = allowed >> database (db). >>>> - // >>>> - if (!IsVerified) { >>>> - if (IsAllowedByDb (AuthData, AuthDataSize)) { >>>> - IsVerified =3D TRUE; >>>> + if (IsAuthDataAssigned && !EFI_ERROR (HashStatus)) { >>>> + // >>>> + // Check the digital signature against the revoked certificate= in forbidden >> database (dbx). >>>> + // >>>> + if (IsForbiddenByDbx (AuthData, AuthDataSize)) { >>>> + Action =3D EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED; >>>> + IsVerified =3D FALSE; >>>> + break; >>>> } >>>> - } >>>> >>>> - // >>>> - // Check the image's hash value. >>>> - // >>>> - DbStatus =3D IsSignatureFoundInDatabase ( >>>> - EFI_IMAGE_SECURITY_DATABASE1, >>>> - mImageDigest, >>>> - &mCertType, >>>> - mImageDigestSize, >>>> - &IsFound >>>> - ); >>>> - if (EFI_ERROR (DbStatus) || IsFound) { >>>> - Action =3D 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 =3D FALSE; >>>> - break; >>>> - } >>>> + // >>>> + // Check the digital signature against the valid certificate i= n allowed >> database (db). >>>> + // >>>> + if (!IsVerified) { >>>> + if (IsAllowedByDb (AuthData, AuthDataSize)) { >>>> + IsVerified =3D TRUE; >>>> + } >>>> + } >>>> >>>> - if (!IsVerified) { >>>> + // >>>> + // Check the image's hash value. >>>> + // >>>> DbStatus =3D IsSignatureFoundInDatabase ( >>>> - EFI_IMAGE_SECURITY_DATABASE, >>>> + EFI_IMAGE_SECURITY_DATABASE1, >>>> mImageDigest, >>>> &mCertType, >>>> mImageDigestSize, >>>> &IsFound >>>> ); >>>> - if (!EFI_ERROR (DbStatus) && IsFound) { >>>> - IsVerified =3D TRUE; >>>> - } else { >>>> - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signe= d 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 =3D EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND; >>>> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signe= d >> but %s hash of image is found in DBX.\n", mHashTypeStr)); >>>> + IsVerified =3D FALSE; >>>> + break; >>>> } >>>> + >>>> + if (!IsVerified) { >>>> + DbStatus =3D IsSignatureFoundInDatabase ( >>>> + EFI_IMAGE_SECURITY_DATABASE, >>>> + mImageDigest, >>>> + &mCertType, >>>> + mImageDigestSize, >>>> + &IsFound >>>> + ); >>>> + if (!EFI_ERROR (DbStatus) && IsFound) { >>>> + IsVerified =3D TRUE; >>>> + } else { >>>> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is sig= ned but >> signature is not allowed by DB and %s hash of image is not found in DB/= DBX.\n", >> mHashTypeStr)); >>>> + } >>>> + } >>>> + } >>>> + >>>> + AddStatus =3D SafeUint32Add (OffSet, AlignedLength, &Result); >>>> + if (EFI_ERROR (AddStatus)) { >>>> + break; >>>> } >>>> + OffSet =3D Result; >>>> } >>>> >>>> if (OffSet !=3D (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. Thi= s >>> 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 fir= st >>> 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 h= elper >>>> variables for them: "SecDataDirEnd" and "SecDataDirLeft", respectivel= y. >>>> This saves us multiple calculations and significantly simplifies the = code. >>>> >>>> Note that all three summands above have type UINT32, therefore the ne= w >>>> 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 lo= op 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/DxeImageVerificatio= nLib.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->VirtualAddr= ess) from the >> start of the file. >>>> // >>>> + SecDataDirEnd =3D SecDataDir->VirtualAddress + SecDataDir->Size; >>>> for (OffSet =3D SecDataDir->VirtualAddress; >>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); >>>> + OffSet < SecDataDirEnd; >>>> OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE (WinCertif= icate- >>> dwLength))) { >>>> WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + OffSet); >>>> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= =3D sizeof >> (WIN_CERTIFICATE) || >>>> - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < >> WinCertificate->dwLength) { >>>> + SecDataDirLeft =3D SecDataDirEnd - OffSet; >>>> + if (SecDataDirLeft <=3D sizeof (WIN_CERTIFICATE) || >>>> + SecDataDirLeft < WinCertificate->dwLength) { >>>> break; >>>> } >>>> >>>> @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler ( >>>> } >>>> } >>>> >>>> - if (OffSet !=3D (SecDataDir->VirtualAddress + SecDataDir->Size)) { >>>> + if (OffSet !=3D SecDataDirEnd) { >>>> // >>>> // The Size in Certificate Table or the attribute certificate ta= ble 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 <=3D sizeof (WIN_CERTIFICATE)) check on= ly >>>> guards the de-referencing of the "WinCertificate" pointer. It does no= t >>>> guard the calculation of hte pointer itself: >>>> >>>> WinCertificate =3D (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/DxeImageVerificatio= nLib.c >>>> +++ >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler ( >>>> for (OffSet =3D SecDataDir->VirtualAddress; >>>> OffSet < SecDataDirEnd; >>>> OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE (WinCertif= icate- >>> dwLength))) { >>>> - WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + OffSet); >>>> SecDataDirLeft =3D SecDataDirEnd - OffSet; >>>> - if (SecDataDirLeft <=3D sizeof (WIN_CERTIFICATE) || >>>> - SecDataDirLeft < WinCertificate->dwLength) { >>>> + if (SecDataDirLeft <=3D sizeof (WIN_CERTIFICATE)) { >>>> + break; >>>> + } >>>> + WinCertificate =3D (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 align= ment >>>> 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/DxeImageVerificatio= nLib.c >>>> +++ >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler ( >>>> break; >>>> } >>>> WinCertificate =3D (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 the= m >>> 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 >>> >>> >>> . >>> >> >> >>=20 >=20