[edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
Laszlo Ersek
lersek at redhat.com
Tue Sep 1 07:56:11 UTC 2020
On 09/01/20 09:43, xiewenyi (A) wrote:
> 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.
Thank you!, I'll send them soon.
Cheers!
Laszlo
>
> 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of wenyi,xie
>>> via groups.io
>>> Sent: Tuesday, September 1, 2020 3:11 PM
>>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Laszlo Ersek
>>> <lersek at redhat.com>; Wang, Jian J <jian.j.wang at intel.com>
>>> Cc: songdongkuang at huawei.com; Mathews, John <john.mathews at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>>> wenyi,xie
>>>>> via groups.io
>>>>> Sent: Monday, August 31, 2020 7:24 PM
>>>>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Laszlo
>>> Ersek
>>>>> <lersek at redhat.com>; Wang, Jian J <jian.j.wang at intel.com>
>>>>> Cc: songdongkuang at huawei.com; Mathews, John
>>> <john.mathews at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>>>>> wenyi,xie
>>>>>>> via groups.io
>>>>>>> Sent: Friday, August 28, 2020 2:18 PM
>>>>>>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Laszlo
>>>>> Ersek
>>>>>>> <lersek at redhat.com>; Wang, Jian J <jian.j.wang at intel.com>
>>>>>>> Cc: songdongkuang at huawei.com; Mathews, John
>>>>> <john.mathews at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>>>>>>> wenyi,xie
>>>>>>>>> via groups.io
>>>>>>>>> Sent: Friday, August 28, 2020 11:18 AM
>>>>>>>>> To: Laszlo Ersek <lersek at redhat.com>; Wang, Jian J
>>>>>>> <jian.j.wang at intel.com>;
>>>>>>>>> devel at edk2.groups.io; Yao, Jiewen <jiewen.yao at intel.com>
>>>>>>>>> Cc: songdongkuang at huawei.com; Mathews, John
>>>>>>> <john.mathews at 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 at redhat.com>
>>>>>>>>>>> Sent: Tuesday, August 18, 2020 1:59 AM
>>>>>>>>>>> To: Wang, Jian J <jian.j.wang at intel.com>; devel at edk2.groups.io;
>>> Yao,
>>>>>>>>> Jiewen <jiewen.yao at intel.com>; xiewenyi2 at huawei.com
>>>>>>>>>>> Cc: huangming23 at huawei.com; songdongkuang at huawei.com;
>>>>> Mathews,
>>>>>>>>> John <john.mathews at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf
>>> Of
>>>>>>> Laszlo
>>>>>>>>>>>>> Ersek
>>>>>>>>>>>>> Sent: Tuesday, August 18, 2020 12:53 AM
>>>>>>>>>>>>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io;
>>>>>>>>>>>>> xiewenyi2 at huawei.com; Wang, Jian J <jian.j.wang at intel.com>
>>>>>>>>>>>>> Cc: huangming23 at huawei.com; songdongkuang at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf
>>>>> Of
>>>>>>>>>>>>> wenyi,xie
>>>>>>>>>>>>>>> via groups.io
>>>>>>>>>>>>>>> Sent: Friday, August 14, 2020 3:54 PM
>>>>>>>>>>>>>>> To: Laszlo Ersek <lersek at redhat.com>; devel at edk2.groups.io;
>>> Yao,
>>>>>>>>>>>>>>> Jiewen <jiewen.yao at intel.com>; Wang, Jian J
>>>>>>> <jian.j.wang at intel.com>
>>>>>>>>>>>>>>> Cc: huangming23 at huawei.com; songdongkuang at 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 at intel.com>
>>>>>>>>>>>>>>>>> Cc: Jian J Wang <jian.j.wang at intel.com>
>>>>>>>>>>>>>>>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>>>>>>>>>>>>>>>> Signed-off-by: Wenyi Xie <xiewenyi2 at 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 at 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 at 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 at 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 at 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 at 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 at 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 (#64877): https://edk2.groups.io/g/devel/message/64877
Mute This Topic: https://groups.io/mt/76165658/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list