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]
-=-=-=-=-=-=-=-=-=-=-=-
