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

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

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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> .
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>> 
>