From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from huawei.com (huawei.com [45.249.212.32]) by mx.groups.io with SMTP id smtpd.web12.51059.1598873048849583519 for ; Mon, 31 Aug 2020 04:24:09 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.32, mailfrom: xiewenyi2@huawei.com) Received: from DGGEMS401-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id 73CB5D8AE98315F994E5; Mon, 31 Aug 2020 19:24:05 +0800 (CST) Received: from [127.0.0.1] (10.174.153.72) by DGGEMS401-HUB.china.huawei.com (10.3.19.201) with Microsoft SMTP Server id 14.3.487.0; Mon, 31 Aug 2020 19:23:58 +0800 From: "wenyi,xie" 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: "songdongkuang@huawei.com" , "Mathews, John" 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> <250fc485-8705-88b7-21c9-ecd28132934d@redhat.com> <196d1d81-3253-6c64-6632-a37da6f2771e@redhat.com> Message-ID: <3384c396-b397-928b-e09e-11979c90420e@huawei.com> Date: Mon, 31 Aug 2020 19:23:52 +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.72] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi=EF=BC=8CJiewen=EF=BC=8C I modify the PE file again, this time it can pass the check in PeCoffLib a= nd cause offset overflow. First, create a PE file and sign it(only one signature), then using binary= edit tool to modify content of PE file like below, 1.check the value of SecDataDir->VirtualAddress, in my PE file, it's 0xE0= 00 2.changing SecDataDir->Size from 0x5F8 to 0xFFFF1FFC 3.changing WinCertificate->dwLength from 0x5F1 to 0xFFFF1FFB 4.padding PE file with 0 until the size of the file is 0xFFFFFFFC(it will= make the PE file so large) OffSet + WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength) = is 0xE000 + 0xFFFF1FFB + 0x5 =3D 0x100000000 Below is the DEBUG code and log, in second loop the offset overflow and be= come 0 for (OffSet =3D SecDataDir->VirtualAddress; OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->d= wLength))) { WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + OffSet); DEBUG((DEBUG_INFO, "OffSet=3D0x%x.\n", OffSet)); if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <=3D sizeof= (WIN_CERTIFICATE) || (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertif= icate->dwLength) { break; } DEBUG((DEBUG_INFO, "WinCertificate->dwLength=3D0x%x, ALIGN_SIZE (WinCert= ificate->dwLength)=3D0x%x.\n", WinCertificate->dwLength, ALIGN_SIZE(WinCert= ificate->dwLength))); SecDataDir->VirtualAddress=3D0xE000, SecDataDir->Size=3D0xFFFF1FFC. OffSet=3D0xE000. WinCertificate->dwLength=3D0xFFFF1FFB, ALIGN_SIZE (WinCertificate->dwLengt= h)=3D0x5. DxeImageVerificationLib: Image is signed but signature is not allowed by D= B and SHA256 hash of image is notOffSet=3D0x0. WinCertificate->dwLength=3D0x5A4D, ALIGN_SIZE (WinCertificate->dwLength)= =3D0x3. OffSet=3D0x5A50. WinCertificate->dwLength=3D0x9024, ALIGN_SIZE (WinCertificate->dwLength)= =3D0x4. OffSet=3D0xEA78. WinCertificate->dwLength=3D0x0, ALIGN_SIZE (WinCertificate->dwLength)=3D0x= 0. The image doesn't pass verification: VenHw(5CF32E0B-8EDF-2E44-9CDA-93205E9= 9EC1C,00000000)/VenHw(964E5B22-6459-11D2-8E39-00A0C969723B,00000000)/\signe= d_1234_4G.efi Regards Wenyi On 2020/8/28 14:43, Yao, Jiewen wrote: > Apology that I did not say clearly. > I mean you should not modify any code to perform an attack. >=20 > I am not asking you to exploit the system. Attack here just means: to ca= use system hang or buffer overflow. That is enough. > But you cannot modify code to remove any existing checker. >=20 > Thank you > Yao Jiewen >=20 >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of wenyi,xi= e >> via groups.io >> Sent: Friday, August 28, 2020 2:18 PM >> To: Yao, Jiewen ; devel@edk2.groups.io; Laszlo Er= sek >> ; Wang, Jian J >> Cc: songdongkuang@huawei.com; Mathews, John >> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] >> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset >> >> Hi=EF=BC=8CJiewen=EF=BC=8C >> >> I don't really get the meaning "create a case that bypass all checks in= PeCoffLib", >> do you mean I need to create a PE file that can bypass all check in PeC= offLib >> without modify any >> code and then cause the problem in DxeImageVerificationLib, or just mod= ify >> some code in PeCoffLib to bypass check instead of removing the calling = of >> PeCoffLoaderGetImageInfo. Would >> you mind explaining a little more specifically? As far as I tried, it's= really hard to >> reproduce the issue without touching any code. >> >> Thanks >> Wenyi >> >> On 2020/8/28 11:50, Yao, Jiewen wrote: >>> HI Wenyi >>> Thank you very much to take time to reproduce. >>> >>> I am particular interested in below: >>> =09"As PE file is modified, function PeCoffLoaderGetImageInfo will ret= urn >> error, so I have to remove it so that for loop can be tested in >> DxeImageVerificationHandler." >>> >>> By design, the PeCoffLib should catch illegal PE/COFF image and return= error. >> (even it cannot catch all, it should catch most ones). >>> Other PE/COFF parser may rely on the checker in PeCoffLib and *no need= * to >> duplicate all checkers. >>> As such, DxeImageVerificationLib (and other PeCoff consumer) just need >> checks what has passed the check in PeCoffLib. >>> >>> I don=E2=80=99t think you should remove the checker. If people can rem= ove the checker, >> I am sure the rest code will be vulnerable, according to the current de= sign. >>> Could you please to create a case that bypass all checks in PeCoffLib,= then run >> into DxeImageVerificationLib and cause the problem? That would be more >> valuable for us. >>> >>> Thank you >>> Yao Jiewen >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of >> wenyi,xie >>>> via groups.io >>>> Sent: Friday, August 28, 2020 11:18 AM >>>> To: Laszlo Ersek ; Wang, Jian J >> ; >>>> devel@edk2.groups.io; Yao, Jiewen >>>> Cc: songdongkuang@huawei.com; Mathews, John >> >>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] >>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset >>>> >>>> Hi=EF=BC=8CLaszlo and everyone=EF=BC=8C >>>> >>>> These days I tried to reproduce the issue=EF=BC=8Cand made some progr= ess. I think >>>> there are two way to cause overflow from a mathematical point of view= , >>>> 1.As Laszlo analysed before, if WinCertificate->dwLength is large eno= ugh, >> close >>>> to MAX_UINT32, then (WinCertificate->dwLength + ALIGN_SIZE >> (WinCertificate- >>>>> dwLength)) will cause overflow. >>>> 2.(WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)) = is >> good, >>>> OffSet is good, but OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZ= E >>>> (WinCertificate->dwLength)) cause overflow. >>>> >>>> Here I choose the second way to reproduce the issue, I choose a PE fi= le and >> sign >>>> it with my own db certificate. Then I use binary edit tool to modify = the PE file >> like >>>> below, >>>> >>>> 1.change SecDataDir->Size from 0x5F8 to 0xFFFF1FFF >>>> 2.change WinCertificate->dwLength from 0x5F1 to 0xFFFF1FFE >>>> SecDataDir->VirtualAddress in my PE is 0xe000 and no need to change. >>>> >>>> As PE file is modified, function PeCoffLoaderGetImageInfo will return= error, >> so I >>>> have to remove it so that for loop can be tested in >> DxeImageVerificationHandler. >>>> The log is as below, >>>> >>>> SecDataDir->VirtualAddress=3D0xE000, SecDataDir->Size=3D0xFFFF1FFF. >>>> (First Loop) >>>> OffSet=3D0xE000. >>>> WinCertificate->dwLength=3D0xFFFF1FFE, ALIGN_SIZE (WinCertificate- >>>>> dwLength)=3D0x2. >>>> (Second Loop) >>>> OffSet=3D0x0. >>>> WinCertificate->dwLength=3D0x5A4D, ALIGN_SIZE (WinCertificate- >>>>> dwLength)=3D0x3. >>>> (Third Loop) >>>> OffSet=3D0x5A50. >>>> WinCertificate->dwLength=3D0x9024, ALIGN_SIZE (WinCertificate- >>>>> dwLength)=3D0x4. >>>> (Forth Loop) >>>> OffSet=3D0xEA78. >>>> WinCertificate->dwLength=3D0xAFAFAFAF, ALIGN_SIZE (WinCertificate- >>>>> dwLength)=3D0x1. >>>> (Fifth Loop) >>>> OffSet=3D0xAFB09A28. >>>> >>>> As I modify SecDataDir->Size and WinCertificate->dwLength, so in firs= t loop >> the >>>> condition check whether the Security Data Directory has enough room l= eft >> for >>>> "WinCertificate->dwLength" is ok. >>>> >>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <=3D siz= eof >>>> (WIN_CERTIFICATE) || >>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCer= tificate- >>>>> dwLength) { >>>> break; >>>> } >>>> >>>> In the beginning of second loop, WinCertificate->dwLength + ALIGN_SIZ= E >>>> (WinCertificate->dwLength) is 0xFFFF2000, offset is 0xE000 >>>> >>>> OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate- >>> dwLength)) >>>> >>>> Offset now is 0 and overflow happens. So even if my PE only have one >> signature, >>>> the for loop is still going untill exception happens. >>>> >>>> >>>> I can't reproduce the issue using the first way, because if WinCertif= icate- >>>>> dwLength is close to MAX_UINT32, it means SecDataDir->Size should al= so >> close >>>> to MAX_UINT32, or the condition check >>>> "(SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertif= icate- >>>>> dwLength" will break. But if SecDataDir->Size is very large, SecData= Dir- >>>>> VirtualAddress have to be smaller than 8 bytes, >>>> because SecDataDir->VirtualAddress + SecDataDir->Size < MAX_UINT32. >>>> SecDataDir->VirtualAddress is the beginning address of the signature,= and >> before >>>> SecDataDir->VirtualAddress is the content of origin PE file, I think = it's >> impossible >>>> that the size of PE file is only 8 bytes. >>>> >>>> As I changed the one line code in DxeImageVerificationHandler to repr= oduce >> the >>>> issue, I'm not sure whether it's ok. >>>> >>>> Thanks >>>> Wenyi >>>> >>>> On 2020/8/19 17:26, Laszlo Ersek wrote: >>>>> On 08/18/20 17:18, Mathews, John wrote: >>>>>> I dug up the original report details. This was noted as a concern = during a >>>> source code inspection. There was no demonstration of how it might b= e >>>> triggered. >>>>>> >>>>>> " There is an integer overflow vulnerability in the >>>> 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 caus= ing an >> endless >>>> loop." >>>>>> >>>>>> The recommendation was to add stricter checking of "Offset" and the >>>> embedded length fields of certificate data >>>>>> before using them. >>>>> >>>>> Thanks for checking! >>>>> >>>>> Laszlo >>>>> >>>>>> >>>>>> >>>>>> >>>>>> -----Original Message----- >>>>>> From: Laszlo Ersek >>>>>> Sent: Tuesday, August 18, 2020 1:59 AM >>>>>> To: Wang, Jian J ; devel@edk2.groups.io; Yao= , >>>> Jiewen ; xiewenyi2@huawei.com >>>>>> Cc: huangming23@huawei.com; songdongkuang@huawei.com; Mathews, >>>> John >>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] >>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset >>>>>> >>>>>> On 08/18/20 04:10, Wang, Jian J wrote: >>>>>>> Laszlo, >>>>>>> >>>>>>> My apologies for the slow response. I'm not the original reporter = but >>>>>>> just the BZ submitter. And I didn't do deep analysis to this issue= . >>>>>>> The issues was reported from one internal team. Add John in loop t= o see >> if >>>> he knows more about it or not. >>>>>>> >>>>>>> My superficial understanding on such issue is that, if there's >>>>>>> "potential" issue in theory and hard to reproduce, it's still wort= hy >>>>>>> of using an alternative way to replace the original implementation >>>>>>> with no "potential" issue at all. Maybe we don't have to prove old= way is >>>> something wrong but must prove that the new way is really safe. >>>>>> >>>>>> I agree, thanks. >>>>>> >>>>>> It would be nice to hear more from the internal team about the orig= inally >>>> reported (even if hard-to-trigger) issue. >>>>>> >>>>>> Thanks! >>>>>> Laszlo >>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> Jian >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: devel@edk2.groups.io On Behalf Of >> Laszlo >>>>>>>> Ersek >>>>>>>> Sent: Tuesday, August 18, 2020 12:53 AM >>>>>>>> To: Yao, Jiewen ; devel@edk2.groups.io; >>>>>>>> xiewenyi2@huawei.com; Wang, Jian J >>>>>>>> Cc: huangming23@huawei.com; songdongkuang@huawei.com >>>>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] >>>>>>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offs= et >>>>>>>> >>>>>>>> Hi Jiewen, >>>>>>>> >>>>>>>> On 08/14/20 10:53, Yao, Jiewen wrote: >>>>>>>>>> To Jiewen, >>>>>>>>>> Sorry, I don't have environment to reproduce the issue. >>>>>>>>> >>>>>>>>> Please help me understand, if you don=E2=80=99t have environment= to >>>>>>>>> reproduce the >>>>>>>> issue, how do you guarantee that your patch does fix the problem = and >>>>>>>> we don=E2=80=99t have any other vulnerabilities? >>>>>>>> >>>>>>>> The original bug report in >>>>>>>> is ser= iously >>>>>>>> lacking. It does not go into detail about the alleged integer ove= rflow. >>>>>>>> It does not quote the code, does not explain the control flow, do= es >>>>>>>> not identify the exact edk2 commit at which the vulnerability exi= sts. >>>>>>>> >>>>>>>> The bug report also does not offer a reproducer. >>>>>>>> >>>>>>>> Additionally, the exact statement that the bug report does make, >>>>>>>> namely >>>>>>>> >>>>>>>> it's possible to overflow Offset back to 0 causing an endless l= oop >>>>>>>> >>>>>>>> is wrong (as far as I can tell anyway). It is not "OffSet" that c= an >>>>>>>> be overflowed to zero, but the *addend* that is added to OffSet c= an >>>>>>>> be overflowed to zero. Therefore the infinite loop will arise bec= ause >>>>>>>> OffSet remains stuck at its present value, and not because OffSet >>>>>>>> will be re-set to zero. >>>>>>>> >>>>>>>> For the reasons, we can only speculate as to what the actual prob= lem >>>>>>>> is, unless Jian decides to join the discussion and clarifies what= he >>>>>>>> had in mind originally. >>>>>>>> >>>>>>>> My understanding (or even "reconstruction") of the vulnerability = is >>>>>>>> described above, and in the patches that I proposed. >>>>>>>> >>>>>>>> We can write a patch based on code analysis. It's possible to >>>>>>>> identify integer overflows based on code analysis, and it's possi= ble >>>>>>>> to verify the correctness of fixes by code review. Obviously test= ing >>>>>>>> is always good, but many times, constructing reproducers for such >>>>>>>> issues that were found by code review, is difficult and time >>>>>>>> consuming. We can say that we don't fix vulnerabilities without >>>>>>>> reproducers, or we can say that we make an effort to fix them eve= n if >>>>>>>> all we have is code analysis (and not a reproducer). >>>>>>>> >>>>>>>> So the above paragraph concerns "correctness". Regarding >>>>>>>> "completeness", I guarantee you that this patch does not fix *all= * >>>>>>>> problems related to PE parsing. (See the other BZ tickets.) It do= es >>>>>>>> fix *one* issue with PE parsing. We can say that we try to fix su= ch >>>>>>>> issues gradually (give different CVE numbers to different issues,= and >>>>>>>> address them one at a time), or we can say that we rewrite PE par= sing >>>> from the ground up. >>>>>>>> (BTW: I have seriously attempted that in the past, and I gave up, >>>>>>>> because the PE format is FUBAR.) >>>>>>>> >>>>>>>> In summary: >>>>>>>> >>>>>>>> - the problem statement is unclear, >>>>>>>> >>>>>>>> - it seems like there is indeed an integer overflow problem in th= e >>>>>>>> SecDataDir parsing loop, but it's uncertain whether the bug repor= ter >>>>>>>> had exactly that in mind >>>>>>>> >>>>>>>> - PE parsing is guaranteed to have other vulnerabilities elsewher= e in >>>>>>>> edk2, but I'm currently unaware of other such issues in >>>>>>>> DxeImageVerificationLib specifically >>>>>>>> >>>>>>>> - even if there are other such problems (in DxeImageVerificationL= ib >>>>>>>> or elswehere), fixing this bug that we know about is likely >>>>>>>> worthwhile >>>>>>>> >>>>>>>> - for many such bugs, constructing a reproducer is difficult and = time >>>>>>>> consuming; code analysis, and *regression-testing* are frequently= the >>>>>>>> only tools we have. That doesn't mean we should ignore this class= of >> bugs. >>>>>>>> >>>>>>>> (Fixing integer overflows retro-actively is more difficult than >>>>>>>> writing overflow-free code in the first place, but that ship has >>>>>>>> sailed; so we can only fight these bugs incrementally now, unless= we >>>>>>>> can rewrite PE parsing with a new data structure from the ground = up. >>>>>>>> Again I tried that and gave up, because the spec is not public, a= nd >>>>>>>> what I did manage to learn about PE, showed that it was insanely >>>>>>>> over-engineered. I'm not saying that other binary / executable >>>>>>>> formats are better, of course.) >>>>>>>> >>>>>>>> Please check out my patches (inlined elsewhere in this thread), a= nd >>>>>>>> comment whether you'd like me to post them to the list as a >>>>>>>> standalone series. >>>>>>>> >>>>>>>> Jian: it wouldn't hurt if you commented as well. >>>>>>>> >>>>>>>> Thanks >>>>>>>> Laszlo >>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: devel@edk2.groups.io On Behalf Of >>>>>>>> wenyi,xie >>>>>>>>>> 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 Of= fset >>>>>>>>>> >>>>>>>>>> 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 >>>>>>>>>>>> DxeImageVerificationHandler function when parsing the PE file= s >>>>>>>>>>>> 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 + Si= ze. >>>>>>>>>>>> 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/DxeImageVerificationL >>>>>>>>>>>> ib.inf >>>>>>>> | >>>>>>>>>> 1 + >>>>>>>>>>>> >>>>>>>>>>>> >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>>>> ib.h >>>>>>>> | >>>>>>>>>> 1 + >>>>>>>>>>>> >>>>>>>>>>>> >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>>>> ib.c >>>>>>>> | >>>>>>>>>> 111 +++++++++++--------- >>>>>>>>>>>> 3 files changed, 63 insertions(+), 50 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git >>>>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.inf >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.inf >>>>>>>>>>>> index 1e1a639857e0..a7ac4830b3d4 100644 >>>>>>>>>>>> --- >>>>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.inf >>>>>>>>>>>> +++ >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.inf >>>>>>>>>>>> @@ -53,6 +53,7 @@ [LibraryClasses] >>>>>>>>>>>> SecurityManagementLib >>>>>>>>>>>> PeCoffLib >>>>>>>>>>>> TpmMeasurementLib >>>>>>>>>>>> + SafeIntLib >>>>>>>>>>>> >>>>>>>>>>>> [Protocols] >>>>>>>>>>>> gEfiFirmwareVolume2ProtocolGuid ## >> SOMETIMES_CONSUMES >>>>>>>>>>>> diff --git >>>>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.h >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.h >>>>>>>>>>>> index 17955ff9774c..060273917d5d 100644 >>>>>>>>>>>> --- >>>>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.h >>>>>>>>>>>> +++ >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.h >>>>>>>>>>>> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Paten= t >>>>>>>>>>>> #include #include >>>>>>>>>>>> #include >>>>>>>>>>>> +#include >>>>>>>>>>>> #include #include >>>>>>>>>>>> #include diff -= -git >>>>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>>>> index 36b87e16d53d..dbc03e28c05b 100644 >>>>>>>>>>>> --- >>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib >>>>>>>> .c >>>>>>>>>>>> +++ >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.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->VirtualAddress) from >>>>>>>> the >>>>>>>>>> start of the file. >>>>>>>>>>>> // >>>>>>>>>>>> for (OffSet =3D SecDataDir->VirtualAddress; >>>>>>>>>>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Si= ze); >>>>>>>>>>>> - OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE >>>> (WinCertificate- >>>>>>>>>>> dwLength))) { >>>>>>>>>>>> + (OffSet >=3D SecDataDir->VirtualAddress) && (OffSet < >>>>>>>>>>>> + (SecDataDir- >>>>>>>>>>> VirtualAddress + SecDataDir->Size));) { >>>>>>>>>>>> + IsAuthDataAssigned =3D FALSE; >>>>>>>>>>>> WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + Off= Set); >>>>>>>>>>>> + AlignedLength =3D 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 com= mit >>>>>>>>>>> 65904cdbb33c): >>>>>>>>>>> >>>>>>>>>>> for (OffSet =3D SecDataDir->VirtualAddress; >>>>>>>>>>> OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size= ); >>>>>>>>>>> OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE >>>>>>>>>>> (WinCertificate- >>>>>>>>>>> dwLength))) { >>>>>>>>>>> WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + OffSe= t); >>>>>>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSe= t) >>>>>>>>>>> <=3D sizeof >>>>>>>>>> (WIN_CERTIFICATE) || >>>>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSe= t) < >>>>>>>> WinCertificate- >>>>>>>>>>> dwLength) { >>>>>>>>>>> break; >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> The last sub-condition checks whether the Security Data Direct= ory >>>>>>>>>>> has enough room left for "WinCertificate->dwLength". If not, t= hen >>>>>>>>>>> we break out of the loop. >>>>>>>>>>> >>>>>>>>>>> If we *do* have enough room, that is: >>>>>>>>>>> >>>>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) >= =3D >>>>>>>> WinCertificate- >>>>>>>>>>> dwLength >>>>>>>>>>> >>>>>>>>>>> then we have (by adding OffSet to both sides): >>>>>>>>>>> >>>>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size >=3D 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 n= ot >>>>>>>>>>> fix one of those overflows. Namely, consider that "dwLength" i= s >>>>>>>>>>> very close to >>>>>>>>>>> MAX_UINT32 (or even think it's exactly MAX_UINT32). Then align= ing >>>>>>>>>>> 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 ju= st >>>>>>>>>>> the >>>>>>>>>>> same: "OffSet" will not be zero, but it will be *stuck*. The >>>>>>>>>>> SafeUint32Add() call at the bottom will succeed, but it will n= ot >>>>>>>>>>> change the value of "OffSet". >>>>>>>>>>> >>>>>>>>>>> More at the bottom. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - Off= Set) >>>>>>>>>>>> <=3D sizeof >>>>>>>>>> (WIN_CERTIFICATE) || >>>>>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - Off= Set) >>>>>>>>>>>> < >>>>>>>>>> WinCertificate->dwLength) { >>>>>>>>>>>> break; >>>>>>>>>>>> @@ -1872,6 +1878,8 @@ DxeImageVerificationHandler ( >>>>>>>>>>>> } >>>>>>>>>>>> AuthData =3D PkcsCertData->CertData; >>>>>>>>>>>> AuthDataSize =3D PkcsCertData->Hdr.dwLength - >>>>>>>>>>>> sizeof(PkcsCertData- >>>>>>>>> Hdr); >>>>>>>>>>>> + IsAuthDataAssigned =3D TRUE; >>>>>>>>>>>> + HashStatus =3D HashPeImageByType (AuthData, AuthDataSi= ze); >>>>>>>>>>>> } else if (WinCertificate->wCertificateType =3D=3D >>>>>>>> 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 <=3D >>>>>>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData)) { >>>>>>>>>>>> break; >>>>>>>>>>>> } >>>>>>>>>>>> - if (!CompareGuid (&WinCertUefiGuid->CertType, >>>> &gEfiCertPkcs7Guid)) >>>>>>>> { >>>>>>>>>>>> - continue; >>>>>>>>>>>> + if (CompareGuid (&WinCertUefiGuid->CertType, >>>>>>>>>>>> + &gEfiCertPkcs7Guid)) >>>>>>>> { >>>>>>>>>>>> + AuthData =3D WinCertUefiGuid->CertData; >>>>>>>>>>>> + AuthDataSize =3D WinCertUefiGuid->Hdr.dwLength - >>>>>>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); >>>>>>>>>>>> + IsAuthDataAssigned =3D TRUE; >>>>>>>>>>>> + HashStatus =3D HashPeImageByType (AuthData, AuthData= Size); >>>>>>>>>>>> } >>>>>>>>>>>> - 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 certi= ficate in >>>>>>>> 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 certifi= cate 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 in 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 >>>> signed >>>>>>>> but >>>>>>>>>> signature is not allowed by DB and %s hash of image is not foun= d in >>>>>>>> DB/DBX.\n", >>>>>>>>>> mHashTypeStr)); >>>>>>>>>>>> + 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; >>>>>>>>>>>> } >>>>>>>>>>>> + >>>>>>>>>>>> + 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: Imag= e is >>>>>>>>>>>> + signed >>>>>>>> but >>>>>>>>>> signature is not allowed by DB and %s hash of image is not foun= d in >>>>>>>> DB/DBX.\n", >>>>>>>>>> mHashTypeStr)); >>>>>>>>>>>> + } >>>>>>>>>>>> + } >>>>>>>>>>>> + } >>>>>>>>>>>> + >>>>>>>>>>>> + AddStatus =3D SafeUint32Add (OffSet, AlignedLength, &Res= ult); >>>>>>>>>>>> + 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 u= se >>>>>>>>>>> the existent "AuthData" variable (with a NULL-check and a >>>>>>>>>>> NULL-assignment) similarly. >>>>>>>>>>> >>>>>>>>>>> - The patch complicates / reorganizes the control flow needles= sly. >>>>>>>>>>> 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 ch= eck >>>>>>>>>>> (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 overfl= ow >>>> 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: ext= ract >>>>>>>>>>>> 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 simplif= ies the >> code. >>>>>>>>>>>> >>>>>>>>>>>> Note that all three summands above have type UINT32, therefor= e >>>>>>>>>>>> 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 r= ight >>>>>>>>>>>> after the loop fires.) >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Laszlo Ersek >>>>>>>>>>>> --- >>>>>>>>>>>> >>>>>>>>>>>> >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>>>> ib.c | >>>>>>>> 12 >>>>>>>>>> ++++++++---- >>>>>>>>>>>> 1 file changed, 8 insertions(+), 4 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git >>>>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>>>> index 36b87e16d53d..8761980c88aa 100644 >>>>>>>>>>>> --- >>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib >>>>>>>> .c >>>>>>>>>>>> +++ >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.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 =3D SecDataDir->VirtualAddress + SecDataDir-= >Size; >>>>>>>>>>>> for (OffSet =3D SecDataDir->VirtualAddress; >>>>>>>>>>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Si= ze); >>>>>>>>>>>> + OffSet < SecDataDirEnd; >>>>>>>>>>>> OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE >>>>>>>>>>>> (WinCertificate- >>>>>>>>>>> dwLength))) { >>>>>>>>>>>> WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + Off= Set); >>>>>>>>>>>> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - Off= Set) <=3D >>>> sizeof >>>>>>>>>> (WIN_CERTIFICATE) || >>>>>>>>>>>> - (SecDataDir->VirtualAddress + SecDataDir->Size - Off= Set) < >>>>>>>>>> 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 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: ass= ign >>>>>>>>>>>> WinCertificate after size check >>>>>>>>>>>> >>>>>>>>>>>> Currently the (SecDataDirLeft <=3D sizeof (WIN_CERTIFICATE)) = check >>>>>>>>>>>> only guards the de-referencing of the "WinCertificate" pointe= r. >>>>>>>>>>>> It does not 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/DxeImageVerificationL >>>>>>>>>>>> ib.c | >>>>>>>> 8 >>>>>>>>>> +++++--- >>>>>>>>>>>> 1 file changed, 5 insertions(+), 3 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git >>>>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>>>> index 8761980c88aa..461ed7cfb5ac 100644 >>>>>>>>>>>> --- >>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib >>>>>>>> .c >>>>>>>>>>>> +++ >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>>>> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler ( >>>>>>>>>>>> for (OffSet =3D SecDataDir->VirtualAddress; >>>>>>>>>>>> OffSet < SecDataDirEnd; >>>>>>>>>>>> OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE >>>>>>>>>>>> (WinCertificate- >>>>>>>>>>> dwLength))) { >>>>>>>>>>>> - WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + Off= Set); >>>>>>>>>>>> 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 + Off= Set); >>>>>>>>>>>> + 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: cat= ch >>>>>>>> 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 stu= ck 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/DxeImageVerificationL >>>>>>>>>>>> ib.c | >>>>>>>> 4 >>>>>>>>>> +++- >>>>>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git >>>>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>>>> index 461ed7cfb5ac..e38eb981b7a0 100644 >>>>>>>>>>>> --- >>>>>>>> >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib >>>>>>>> .c >>>>>>>>>>>> +++ >>>>>>>>>> >> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL >>>>>>>>>> ib.c >>>>>>>>>>>> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler ( >>>>>>>>>>>> break; >>>>>>>>>>>> } >>>>>>>>>>>> WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + Off= Set); >>>>>>>>>>>> - 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 su= bmit >>>>>>>>>>> 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 B= oot >>>>>>>>>>> regression tests. >>>>>>>>>>> >>>>>>>>>>> Thanks >>>>>>>>>>> Laszlo >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> . >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>>> . >>>>> >>>> >>>> >>>> >>> >> >> >>=20 >=20