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

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

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