Re: [edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset

["wenyi,xie" <xiewenyi2@huawei.com>]

To Jiewen,

Sorry to make you lost，I mean the patches Laszlo proposed in email，
https://edk2.groups.io/g/devel/message/64243
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.yao@intel.com>; devel@edk2.groups.io; Laszlo Ersek
>> <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>
>> Cc: songdongkuang@huawei.com; Mathews, John <john.mathews@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.yao@intel.com>; devel@edk2.groups.io; Laszlo
>> Ersek
>>>> <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>
>>>> Cc: songdongkuang@huawei.com; Mathews, John
>> <john.mathews@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.yao@intel.com>; devel@edk2.groups.io; Laszlo
>>>> Ersek
>>>>>> <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>
>>>>>> Cc: songdongkuang@huawei.com; Mathews, John
>>>> <john.mathews@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 <lersek@redhat.com>; Wang, Jian J
>>>>>> <jian.j.wang@intel.com>;
>>>>>>>> devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
>>>>>>>> Cc: songdongkuang@huawei.com; Mathews, John
>>>>>> <john.mathews@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 <lersek@redhat.com>
>>>>>>>>>> Sent: Tuesday, August 18, 2020 1:59 AM
>>>>>>>>>> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io;
>> Yao,
>>>>>>>> Jiewen <jiewen.yao@intel.com>; xiewenyi2@huawei.com
>>>>>>>>>> Cc: huangming23@huawei.com; songdongkuang@huawei.com;
>>>> Mathews,
>>>>>>>> John <john.mathews@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.yao@intel.com>; devel@edk2.groups.io;
>>>>>>>>>>>> xiewenyi2@huawei.com; Wang, Jian J <jian.j.wang@intel.com>
>>>>>>>>>>>> Cc: huangming23@huawei.com; songdongkuang@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 <lersek@redhat.com>; devel@edk2.groups.io;
>> Yao,
>>>>>>>>>>>>>> Jiewen <jiewen.yao@intel.com>; Wang, Jian J
>>>>>> <jian.j.wang@intel.com>
>>>>>>>>>>>>>> Cc: huangming23@huawei.com; songdongkuang@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.yao@intel.com>
>>>>>>>>>>>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>>>>>>>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>>>>>>>>>>>> Signed-off-by: Wenyi Xie <xiewenyi2@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 <lersek@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 <lersek@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 <lersek@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 <lersek@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 <lersek@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 <lersek@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
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> .
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> .
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>> 
>