To Jiewen, Sorry to make you lost,I mean the patches Laszlo proposed in email, https://edk2.groups.io/g/devel/message/64243 eb0c6bcb-77fb-2fb9-783e-aa5025953a80@redhat.com">http://mid.mail-archive.com/eb0c6bcb-77fb-2fb9-783e-aa5025953a80@redhat.com
To Laszlo, Sure, I will test your patches against my reproducer and add "tested-by:" tag to the patches after you post them. Regards Wenyi On 2020/9/1 15:31, Yao, Jiewen wrote: > 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 (#64876): https://edk2.groups.io/g/devel/message/64876 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] -=-=-=-=-=-=-=-=-=-=-=-