I am sorry, that I am a little lost here. We have discussed different patches. I am not 100% sure which one is "Laszlo's patches".
To make thing easy and record all actions, would you please reply the patch(es) you have verified, with your "tested-by:" tag? Thank you Yao Jiewen > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of wenyi,xie > via groups.io > Sent: Tuesday, September 1, 2020 3:11 PM > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; Laszlo Ersek > <ler...@redhat.com>; Wang, Jian J <jian.j.w...@intel.com> > Cc: songdongku...@huawei.com; Mathews, John <john.math...@intel.com> > Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] > SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset > > I think Laszlo's patches is OK, I have applied and tested it using my case. > It can > catch the issue. > DEBUG code and log below, > > SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size; > DEBUG((DEBUG_INFO, "SecDataDirEnd=0x%x.\n", SecDataDirEnd)); > for (OffSet = SecDataDir->VirtualAddress; > OffSet < SecDataDirEnd; > OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate- > >dwLength))) { > SecDataDirLeft = SecDataDirEnd - OffSet; > DEBUG((DEBUG_INFO, "OffSet=0x%x, SecDataDirLeft=0x%x.\n", OffSet, > SecDataDirLeft)); > if (SecDataDirLeft <= sizeof(WIN_CERTIFICATE)) { > break; > } > WinCertificate = (WIN_CERTIFICATE *)(mImageBase + OffSet); > DEBUG((DEBUG_INFO, "WinCertificate->dwLength=0x%x, ALIGN_SIZE > (WinCertificate->dwLength)=0x%x.\n", WinCertificate->dwLength, > ALIGN_SIZE(WinCertificate->dwLength))); > if (SecDataDirLeft < WinCertificate->dwLength || > (SecDataDirLeft - WinCertificate->dwLength < > ALIGN_SIZE(WinCertificate->dwLength))) { > DEBUG((DEBUG_INFO, "Parameter is invalid and break.\n")); > break; > } > > SecDataDirEnd=0xFFFFFFFC. > OffSet=0xE000, SecDataDirLeft=0xFFFF1FFC. > WinCertificate->dwLength=0xFFFF1FFB, ALIGN_SIZE (WinCertificate- > >dwLength)=0x5. > Parameter is invalid and break. > The image doesn't pass verification: VenHw(5CF32E0B-8EDF-2E44-9CDA- > 93205E99EC1C,00000000)/VenHw(964E5B22-6459-11D2-8E39- > 00A0C969723B,00000000)/\signed_1234_4G.efi > > Regards > Wenyi > > On 2020/9/1 0:06, Yao, Jiewen wrote: > > Sounds great. Appreciate your hard work on that. > > > > Will you post a patch to fix the issue again? > > > > Thank you > > Yao Jiewen > > > >> -----Original Message----- > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > wenyi,xie > >> via groups.io > >> Sent: Monday, August 31, 2020 7:24 PM > >> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; Laszlo > Ersek > >> <ler...@redhat.com>; Wang, Jian J <jian.j.w...@intel.com> > >> Cc: songdongku...@huawei.com; Mathews, John > <john.math...@intel.com> > >> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] > >> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset > >> > >> Hi,Jiewen, > >> > >> I modify the PE file again, this time it can pass the check in PeCoffLib > >> and > 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 > >> 0xE000 > >> 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 = 0x100000000 > >> > >> Below is the DEBUG code and log, in second loop the offset overflow and > >> become 0 > >> > >> for (OffSet = SecDataDir->VirtualAddress; > >> OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); > >> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate- > >>> dwLength))) { > >> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); > >> DEBUG((DEBUG_INFO, "OffSet=0x%x.\n", OffSet)); > >> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof > >> (WIN_CERTIFICATE) || > >> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < > >> WinCertificate- > >>> dwLength) { > >> break; > >> } > >> DEBUG((DEBUG_INFO, "WinCertificate->dwLength=0x%x, ALIGN_SIZE > >> (WinCertificate->dwLength)=0x%x.\n", WinCertificate->dwLength, > >> ALIGN_SIZE(WinCertificate->dwLength))); > >> > >> > >> SecDataDir->VirtualAddress=0xE000, SecDataDir->Size=0xFFFF1FFC. > >> OffSet=0xE000. > >> WinCertificate->dwLength=0xFFFF1FFB, ALIGN_SIZE (WinCertificate- > >>> dwLength)=0x5. > >> DxeImageVerificationLib: Image is signed but signature is not allowed by DB > and > >> SHA256 hash of image is notOffSet=0x0. > >> WinCertificate->dwLength=0x5A4D, ALIGN_SIZE (WinCertificate- > >>> dwLength)=0x3. > >> OffSet=0x5A50. > >> WinCertificate->dwLength=0x9024, ALIGN_SIZE (WinCertificate- > >>> dwLength)=0x4. > >> OffSet=0xEA78. > >> WinCertificate->dwLength=0x0, ALIGN_SIZE (WinCertificate->dwLength)=0x0. > >> The image doesn't pass verification: VenHw(5CF32E0B-8EDF-2E44-9CDA- > >> 93205E99EC1C,00000000)/VenHw(964E5B22-6459-11D2-8E39- > >> 00A0C969723B,00000000)/\signed_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. > >>> > >>> I am not asking you to exploit the system. Attack here just means: to > >>> cause > >> system hang or buffer overflow. That is enough. > >>> But you cannot modify code to remove any existing checker. > >>> > >>> Thank you > >>> Yao Jiewen > >>> > >>>> -----Original Message----- > >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > >> wenyi,xie > >>>> via groups.io > >>>> Sent: Friday, August 28, 2020 2:18 PM > >>>> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; Laszlo > >> Ersek > >>>> <ler...@redhat.com>; Wang, Jian J <jian.j.w...@intel.com> > >>>> Cc: songdongku...@huawei.com; Mathews, John > >> <john.math...@intel.com> > >>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] > >>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset > >>>> > >>>> Hi,Jiewen, > >>>> > >>>> 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 > PeCoffLib > >>>> without modify any > >>>> code and then cause the problem in DxeImageVerificationLib, or just > modify > >>>> 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: > >>>>> "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." > >>>>> > >>>>> 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’t think you should remove the checker. If people can remove the > >> checker, > >>>> I am sure the rest code will be vulnerable, according to the current > >>>> design. > >>>>> 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 <devel@edk2.groups.io> On Behalf Of > >>>> wenyi,xie > >>>>>> via groups.io > >>>>>> Sent: Friday, August 28, 2020 11:18 AM > >>>>>> To: Laszlo Ersek <ler...@redhat.com>; Wang, Jian J > >>>> <jian.j.w...@intel.com>; > >>>>>> devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com> > >>>>>> Cc: songdongku...@huawei.com; Mathews, John > >>>> <john.math...@intel.com> > >>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] > >>>>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset > >>>>>> > >>>>>> Hi,Laszlo and everyone, > >>>>>> > >>>>>> These days I tried to reproduce the issue,and made some progress. 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 > enough, > >>>> 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 += (WinCertificate->dwLength + ALIGN_SIZE > >>>>>> (WinCertificate->dwLength)) cause overflow. > >>>>>> > >>>>>> Here I choose the second way to reproduce the issue, I choose a PE file > >> 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=0xE000, SecDataDir->Size=0xFFFF1FFF. > >>>>>> (First Loop) > >>>>>> OffSet=0xE000. > >>>>>> WinCertificate->dwLength=0xFFFF1FFE, ALIGN_SIZE (WinCertificate- > >>>>>>> dwLength)=0x2. > >>>>>> (Second Loop) > >>>>>> OffSet=0x0. > >>>>>> WinCertificate->dwLength=0x5A4D, ALIGN_SIZE (WinCertificate- > >>>>>>> dwLength)=0x3. > >>>>>> (Third Loop) > >>>>>> OffSet=0x5A50. > >>>>>> WinCertificate->dwLength=0x9024, ALIGN_SIZE (WinCertificate- > >>>>>>> dwLength)=0x4. > >>>>>> (Forth Loop) > >>>>>> OffSet=0xEA78. > >>>>>> WinCertificate->dwLength=0xAFAFAFAF, ALIGN_SIZE (WinCertificate- > >>>>>>> dwLength)=0x1. > >>>>>> (Fifth Loop) > >>>>>> OffSet=0xAFB09A28. > >>>>>> > >>>>>> As I modify SecDataDir->Size and WinCertificate->dwLength, so in first > >> loop > >>>> the > >>>>>> condition check whether the Security Data Directory has enough room > left > >>>> for > >>>>>> "WinCertificate->dwLength" is ok. > >>>>>> > >>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof > >>>>>> (WIN_CERTIFICATE) || > >>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < > >> WinCertificate- > >>>>>>> dwLength) { > >>>>>> break; > >>>>>> } > >>>>>> > >>>>>> In the beginning of second loop, WinCertificate->dwLength + > ALIGN_SIZE > >>>>>> (WinCertificate->dwLength) is 0xFFFF2000, offset is 0xE000 > >>>>>> > >>>>>> OffSet += (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 > >>>>>> WinCertificate- > >>>>>>> dwLength is close to MAX_UINT32, it means SecDataDir->Size should > also > >>>> close > >>>>>> to MAX_UINT32, or the condition check > >>>>>> "(SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < > >> WinCertificate- > >>>>>>> dwLength" will break. But if SecDataDir->Size is very large, > >>>>>>> SecDataDir- > >>>>>>> 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 > >> reproduce > >>>> 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 be > >>>>>> 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 > >>>>>>>> causing 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 <ler...@redhat.com> > >>>>>>>> Sent: Tuesday, August 18, 2020 1:59 AM > >>>>>>>> To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io; > Yao, > >>>>>> Jiewen <jiewen....@intel.com>; xiewen...@huawei.com > >>>>>>>> Cc: huangmin...@huawei.com; songdongku...@huawei.com; > >> Mathews, > >>>>>> John <john.math...@intel.com> > >>>>>>>> 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 to > >> 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 worthy > >>>>>>>>> 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 > >> originally > >>>>>> reported (even if hard-to-trigger) issue. > >>>>>>>> > >>>>>>>> Thanks! > >>>>>>>> Laszlo > >>>>>>>> > >>>>>>>>> > >>>>>>>>> Regards, > >>>>>>>>> Jian > >>>>>>>>> > >>>>>>>>>> -----Original Message----- > >>>>>>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf > Of > >>>> Laszlo > >>>>>>>>>> Ersek > >>>>>>>>>> Sent: Tuesday, August 18, 2020 12:53 AM > >>>>>>>>>> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; > >>>>>>>>>> xiewen...@huawei.com; Wang, Jian J <jian.j.w...@intel.com> > >>>>>>>>>> Cc: huangmin...@huawei.com; songdongku...@huawei.com > >>>>>>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] > >>>>>>>>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of > Offset > >>>>>>>>>> > >>>>>>>>>> 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’t have environment to > >>>>>>>>>>> reproduce the > >>>>>>>>>> issue, how do you guarantee that your patch does fix the problem > and > >>>>>>>>>> we don’t have any other vulnerabilities? > >>>>>>>>>> > >>>>>>>>>> The original bug report in > >>>>>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2215#c0> is > >> seriously > >>>>>>>>>> lacking. It does not go into detail about the alleged integer > overflow. > >>>>>>>>>> It does not quote the code, does not explain the control flow, does > >>>>>>>>>> not identify the exact edk2 commit at which the vulnerability > >>>>>>>>>> exists. > >>>>>>>>>> > >>>>>>>>>> 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 > >>>>>>>>>> loop > >>>>>>>>>> > >>>>>>>>>> is wrong (as far as I can tell anyway). It is not "OffSet" that can > >>>>>>>>>> be overflowed to zero, but the *addend* that is added to OffSet > can > >>>>>>>>>> be overflowed to zero. Therefore the infinite loop will arise > because > >>>>>>>>>> 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 > problem > >>>>>>>>>> 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 > >>>>>>>>>> possible > >>>>>>>>>> to verify the correctness of fixes by code review. Obviously > >>>>>>>>>> testing > >>>>>>>>>> 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 even > 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 does > >>>>>>>>>> fix *one* issue with PE parsing. We can say that we try to fix such > >>>>>>>>>> issues gradually (give different CVE numbers to different issues, > >>>>>>>>>> and > >>>>>>>>>> address them one at a time), or we can say that we rewrite PE > parsing > >>>>>> 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 the > >>>>>>>>>> SecDataDir parsing loop, but it's uncertain whether the bug > reporter > >>>>>>>>>> had exactly that in mind > >>>>>>>>>> > >>>>>>>>>> - PE parsing is guaranteed to have other vulnerabilities elsewhere > >>>>>>>>>> in > >>>>>>>>>> edk2, but I'm currently unaware of other such issues in > >>>>>>>>>> DxeImageVerificationLib specifically > >>>>>>>>>> > >>>>>>>>>> - even if there are other such problems (in DxeImageVerificationLib > >>>>>>>>>> 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, and > >>>>>>>>>> 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), and > >>>>>>>>>> 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 <devel@edk2.groups.io> On Behalf > >> Of > >>>>>>>>>> wenyi,xie > >>>>>>>>>>>> via groups.io > >>>>>>>>>>>> Sent: Friday, August 14, 2020 3:54 PM > >>>>>>>>>>>> To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io; > Yao, > >>>>>>>>>>>> Jiewen <jiewen....@intel.com>; Wang, Jian J > >>>> <jian.j.w...@intel.com> > >>>>>>>>>>>> Cc: huangmin...@huawei.com; songdongku...@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=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 <jiewen....@intel.com> > >>>>>>>>>>>>>> Cc: Jian J Wang <jian.j.w...@intel.com> > >>>>>>>>>>>>>> Cc: Laszlo Ersek <ler...@redhat.com> > >>>>>>>>>>>>>> Signed-off-by: Wenyi Xie <xiewen...@huawei.com> > >>>>>>>>>>>>>> --- > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>> 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- > >> Patent > >>>>>>>>>>>>>> #include <Library/DevicePathLib.h> #include > >>>>>>>>>>>>>> <Library/SecurityManagementLib.h> #include > >> <Library/PeCoffLib.h> > >>>>>>>>>>>>>> +#include <Library/SafeIntLib.h> > >>>>>>>>>>>>>> #include <Protocol/FirmwareVolume2.h> #include > >>>>>>>>>>>>>> <Protocol/DevicePath.h> #include <Protocol/BlockIo.h> 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 = 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 > >>>>>>>>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2215#c0> 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 <ler...@redhat.com> > >>>>>>>>>>>>>> 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 <ler...@redhat.com> > >>>>>>>>>>>>>> --- > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>> 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 = 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 <ler...@redhat.com> > >>>>>>>>>>>>>> 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 <ler...@redhat.com> > >>>>>>>>>>>>>> --- > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>> 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 = 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 <ler...@redhat.com> > >>>>>>>>>>>>>> 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 <ler...@redhat.com> > >>>>>>>>>>>>>> --- > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>> 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 = (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 > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> . > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> . > >>>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>>> > >>>> > >>> > >> > >> > >> > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64875): https://edk2.groups.io/g/devel/message/64875 Mute This Topic: https://groups.io/mt/76165658/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-