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 (#64871): https://edk2.groups.io/g/devel/message/64871 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] -=-=-=-=-=-=-=-=-=-=-=-