Apology that I did not say clearly.
I mean you should not modify any code to perform an attack.
I am not asking you to exploit the system. Attack here just means: to cause
system hang or buffer overflow. That is enough.
But you cannot modify code to remove any existing checker.
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of wenyi,xie
> via groups.io
> Sent: Friday, August 28, 2020 2:18 PM
> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; Laszlo Ersek
> <ler...@redhat.com>; Wang, Jian J <jian.j.w...@intel.com>
> Cc: songdongku...@huawei.com; Mathews, John <john.math...@intel.com>
> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
>
> Hi，Jiewen，
>
> I don't really get the meaning "create a case that bypass all checks in
> PeCoffLib",
> do you mean I need to create a PE file that can bypass all check in PeCoffLib
> without modify any
> code and then cause the problem in DxeImageVerificationLib, or just modify
> some code in PeCoffLib to bypass check instead of removing the calling of
> PeCoffLoaderGetImageInfo. Would
> you mind explaining a little more specifically? As far as I tried, it's
> really hard to
> reproduce the issue without touching any code.
>
> Thanks
> Wenyi
>
> On 2020/8/28 11:50, Yao, Jiewen wrote:
> > HI Wenyi
> > Thank you very much to take time to reproduce.
> >
> > I am particular interested in below:
> > "As PE file is modified, function PeCoffLoaderGetImageInfo will return
> error, so I have to remove it so that for loop can be tested in
> DxeImageVerificationHandler."
> >
> > By design, the PeCoffLib should catch illegal PE/COFF image and return
> > error.
> (even it cannot catch all, it should catch most ones).
> > Other PE/COFF parser may rely on the checker in PeCoffLib and *no need* to
> duplicate all checkers.
> > As such, DxeImageVerificationLib (and other PeCoff consumer) just need
> checks what has passed the check in PeCoffLib.
> >
> > I don’t think you should remove the checker. If people can remove the
> > checker,
> I am sure the rest code will be vulnerable, according to the current design.
> > Could you please to create a case that bypass all checks in PeCoffLib, then
> > run
> into DxeImageVerificationLib and cause the problem? That would be more
> valuable for us.
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> wenyi,xie
> >> via groups.io
> >> Sent: Friday, August 28, 2020 11:18 AM
> >> To: Laszlo Ersek <ler...@redhat.com>; Wang, Jian J
> <jian.j.w...@intel.com>;
> >> devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com>
> >> Cc: songdongku...@huawei.com; Mathews, John
> <john.math...@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> >> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
> >>
> >> Hi，Laszlo and everyone，
> >>
> >> These days I tried to reproduce the issue，and made some progress. I think
> >> there are two way to cause overflow from a mathematical point of view,
> >> 1.As Laszlo analysed before, if WinCertificate->dwLength is large enough,
> close
> >> to MAX_UINT32, then (WinCertificate->dwLength + ALIGN_SIZE
> (WinCertificate-
> >>> dwLength)) will cause overflow.
> >> 2.(WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)) is
> good,
> >> OffSet is good, but OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> >> (WinCertificate->dwLength)) cause overflow.
> >>
> >> Here I choose the second way to reproduce the issue, I choose a PE file and
> sign
> >> it with my own db certificate. Then I use binary edit tool to modify the
> >> PE file
> like
> >> below,
> >>
> >> 1.change SecDataDir->Size from 0x5F8 to 0xFFFF1FFF
> >> 2.change WinCertificate->dwLength from 0x5F1 to 0xFFFF1FFE
> >> SecDataDir->VirtualAddress in my PE is 0xe000 and no need to change.
> >>
> >> As PE file is modified, function PeCoffLoaderGetImageInfo will return
> >> error,
> so I
> >> have to remove it so that for loop can be tested in
> DxeImageVerificationHandler.
> >> The log is as below,
> >>
> >> SecDataDir->VirtualAddress=0xE000, SecDataDir->Size=0xFFFF1FFF.
> >> (First Loop)
> >> OffSet=0xE000.
> >> WinCertificate->dwLength=0xFFFF1FFE, ALIGN_SIZE (WinCertificate-
> >>> dwLength)=0x2.
> >> (Second Loop)
> >> OffSet=0x0.
> >> WinCertificate->dwLength=0x5A4D, ALIGN_SIZE (WinCertificate-
> >>> dwLength)=0x3.
> >> (Third Loop)
> >> OffSet=0x5A50.
> >> WinCertificate->dwLength=0x9024, ALIGN_SIZE (WinCertificate-
> >>> dwLength)=0x4.
> >> (Forth Loop)
> >> OffSet=0xEA78.
> >> WinCertificate->dwLength=0xAFAFAFAF, ALIGN_SIZE (WinCertificate-
> >>> dwLength)=0x1.
> >> (Fifth Loop)
> >> OffSet=0xAFB09A28.
> >>
> >> As I modify SecDataDir->Size and WinCertificate->dwLength, so in first loop
> the
> >> condition check whether the Security Data Directory has enough room left
> for
> >> "WinCertificate->dwLength" is ok.
> >>
> >> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof
> >> (WIN_CERTIFICATE) ||
> >> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
> >> WinCertificate-
> >>> dwLength) {
> >> break;
> >> }
> >>
> >> In the beginning of second loop, WinCertificate->dwLength + ALIGN_SIZE
> >> (WinCertificate->dwLength) is 0xFFFF2000, offset is 0xE000
> >>
> >> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-
> >dwLength))
> >>
> >> Offset now is 0 and overflow happens. So even if my PE only have one
> signature,
> >> the for loop is still going untill exception happens.
> >>
> >>
> >> I can't reproduce the issue using the first way, because if WinCertificate-
> >>> dwLength is close to MAX_UINT32, it means SecDataDir->Size should also
> close
> >> to MAX_UINT32, or the condition check
> >> "(SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate-
> >>> dwLength" will break. But if SecDataDir->Size is very large, SecDataDir-
> >>> VirtualAddress have to be smaller than 8 bytes,
> >> because SecDataDir->VirtualAddress + SecDataDir->Size < MAX_UINT32.
> >> SecDataDir->VirtualAddress is the beginning address of the signature, and
> before
> >> SecDataDir->VirtualAddress is the content of origin PE file, I think it's
> impossible
> >> that the size of PE file is only 8 bytes.
> >>
> >> As I changed the one line code in DxeImageVerificationHandler to reproduce
> the
> >> issue, I'm not sure whether it's ok.
> >>
> >> Thanks
> >> Wenyi
> >>
> >> On 2020/8/19 17:26, Laszlo Ersek wrote:
> >>> On 08/18/20 17:18, Mathews, John wrote:
> >>>> I dug up the original report details. This was noted as a concern
> >>>> during a
> >> source code inspection. There was no demonstration of how it might be
> >> triggered.
> >>>>
> >>>> " There is an integer overflow vulnerability in the
> >> DxeImageVerificationHandler function when
> >>>> parsing the PE files attribute certificate table. In cases where
> WinCertificate-
> >>> dwLength is
> >>>> sufficiently large, it's possible to overflow Offset back to 0 causing an
> endless
> >> loop."
> >>>>
> >>>> The recommendation was to add stricter checking of "Offset" and the
> >> embedded length fields of certificate data
> >>>> before using them.
> >>>
> >>> Thanks for checking!
> >>>
> >>> Laszlo
> >>>
> >>>>
> >>>>
> >>>>
> >>>> -----Original Message-----
> >>>> From: Laszlo Ersek <ler...@redhat.com>
> >>>> Sent: Tuesday, August 18, 2020 1:59 AM
> >>>> To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io; Yao,
> >> Jiewen <jiewen....@intel.com>; xiewen...@huawei.com
> >>>> Cc: huangmin...@huawei.com; songdongku...@huawei.com; Mathews,
> >> John <john.math...@intel.com>
> >>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> >> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
> >>>>
> >>>> On 08/18/20 04:10, Wang, Jian J wrote:
> >>>>> Laszlo,
> >>>>>
> >>>>> My apologies for the slow response. I'm not the original reporter but
> >>>>> just the BZ submitter. And I didn't do deep analysis to this issue.
> >>>>> The issues was reported from one internal team. Add John in loop to see
> if
> >> he knows more about it or not.
> >>>>>
> >>>>> My superficial understanding on such issue is that, if there's
> >>>>> "potential" issue in theory and hard to reproduce, it's still worthy
> >>>>> of using an alternative way to replace the original implementation
> >>>>> with no "potential" issue at all. Maybe we don't have to prove old way
> >>>>> is
> >> something wrong but must prove that the new way is really safe.
> >>>>
> >>>> I agree, thanks.
> >>>>
> >>>> It would be nice to hear more from the internal team about the originally
> >> reported (even if hard-to-trigger) issue.
> >>>>
> >>>> Thanks!
> >>>> Laszlo
> >>>>
> >>>>>
> >>>>> Regards,
> >>>>> Jian
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Laszlo
> >>>>>> Ersek
> >>>>>> Sent: Tuesday, August 18, 2020 12:53 AM
> >>>>>> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io;
> >>>>>> xiewen...@huawei.com; Wang, Jian J <jian.j.w...@intel.com>
> >>>>>> Cc: huangmin...@huawei.com; songdongku...@huawei.com
> >>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> >>>>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
> >>>>>>
> >>>>>> Hi Jiewen,
> >>>>>>
> >>>>>> On 08/14/20 10:53, Yao, Jiewen wrote:
> >>>>>>>> To Jiewen,
> >>>>>>>> Sorry, I don't have environment to reproduce the issue.
> >>>>>>>
> >>>>>>> Please help me understand, if you don’t have environment to
> >>>>>>> reproduce the
> >>>>>> issue, how do you guarantee that your patch does fix the problem and
> >>>>>> we don’t have any other vulnerabilities?
> >>>>>>
> >>>>>> The original bug report in
> >>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2215#c0> is seriously
> >>>>>> lacking. It does not go into detail about the alleged integer overflow.
> >>>>>> It does not quote the code, does not explain the control flow, does
> >>>>>> not identify the exact edk2 commit at which the vulnerability exists.
> >>>>>>
> >>>>>> The bug report also does not offer a reproducer.
> >>>>>>
> >>>>>> Additionally, the exact statement that the bug report does make,
> >>>>>> namely
> >>>>>>
> >>>>>> it's possible to overflow Offset back to 0 causing an endless loop
> >>>>>>
> >>>>>> is wrong (as far as I can tell anyway). It is not "OffSet" that can
> >>>>>> be overflowed to zero, but the *addend* that is added to OffSet can
> >>>>>> be overflowed to zero. Therefore the infinite loop will arise because
> >>>>>> OffSet remains stuck at its present value, and not because OffSet
> >>>>>> will be re-set to zero.
> >>>>>>
> >>>>>> For the reasons, we can only speculate as to what the actual problem
> >>>>>> is, unless Jian decides to join the discussion and clarifies what he
> >>>>>> had in mind originally.
> >>>>>>
> >>>>>> My understanding (or even "reconstruction") of the vulnerability is
> >>>>>> described above, and in the patches that I proposed.
> >>>>>>
> >>>>>> We can write a patch based on code analysis. It's possible to
> >>>>>> identify integer overflows based on code analysis, and it's possible
> >>>>>> to verify the correctness of fixes by code review. Obviously testing
> >>>>>> is always good, but many times, constructing reproducers for such
> >>>>>> issues that were found by code review, is difficult and time
> >>>>>> consuming. We can say that we don't fix vulnerabilities without
> >>>>>> reproducers, or we can say that we make an effort to fix them even if
> >>>>>> all we have is code analysis (and not a reproducer).
> >>>>>>
> >>>>>> So the above paragraph concerns "correctness". Regarding
> >>>>>> "completeness", I guarantee you that this patch does not fix *all*
> >>>>>> problems related to PE parsing. (See the other BZ tickets.) It does
> >>>>>> fix *one* issue with PE parsing. We can say that we try to fix such
> >>>>>> issues gradually (give different CVE numbers to different issues, and
> >>>>>> address them one at a time), or we can say that we rewrite PE parsing
> >> from the ground up.
> >>>>>> (BTW: I have seriously attempted that in the past, and I gave up,
> >>>>>> because the PE format is FUBAR.)
> >>>>>>
> >>>>>> In summary:
> >>>>>>
> >>>>>> - the problem statement is unclear,
> >>>>>>
> >>>>>> - it seems like there is indeed an integer overflow problem in the
> >>>>>> SecDataDir parsing loop, but it's uncertain whether the bug reporter
> >>>>>> had exactly that in mind
> >>>>>>
> >>>>>> - PE parsing is guaranteed to have other vulnerabilities elsewhere in
> >>>>>> edk2, but I'm currently unaware of other such issues in
> >>>>>> DxeImageVerificationLib specifically
> >>>>>>
> >>>>>> - even if there are other such problems (in DxeImageVerificationLib
> >>>>>> or elswehere), fixing this bug that we know about is likely
> >>>>>> worthwhile
> >>>>>>
> >>>>>> - for many such bugs, constructing a reproducer is difficult and time
> >>>>>> consuming; code analysis, and *regression-testing* are frequently the
> >>>>>> only tools we have. That doesn't mean we should ignore this class of
> bugs.
> >>>>>>
> >>>>>> (Fixing integer overflows retro-actively is more difficult than
> >>>>>> writing overflow-free code in the first place, but that ship has
> >>>>>> sailed; so we can only fight these bugs incrementally now, unless we
> >>>>>> can rewrite PE parsing with a new data structure from the ground up.
> >>>>>> Again I tried that and gave up, because the spec is not public, and
> >>>>>> what I did manage to learn about PE, showed that it was insanely
> >>>>>> over-engineered. I'm not saying that other binary / executable
> >>>>>> formats are better, of course.)
> >>>>>>
> >>>>>> Please check out my patches (inlined elsewhere in this thread), and
> >>>>>> comment whether you'd like me to post them to the list as a
> >>>>>> standalone series.
> >>>>>>
> >>>>>> Jian: it wouldn't hurt if you commented as well.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Laszlo
> >>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>>>>> wenyi,xie
> >>>>>>>> via groups.io
> >>>>>>>> Sent: Friday, August 14, 2020 3:54 PM
> >>>>>>>> To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io; Yao,
> >>>>>>>> Jiewen <jiewen....@intel.com>; Wang, Jian J
> <jian.j.w...@intel.com>
> >>>>>>>> Cc: huangmin...@huawei.com; songdongku...@huawei.com
> >>>>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> >>>>>>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
> >>>>>>>>
> >>>>>>>> To Laszlo,
> >>>>>>>> Thank you for your detailed description, I agree with what you
> >>>>>>>> analyzed and
> >>>>>> I'm
> >>>>>>>> OK with your patches, it's
> >>>>>>>> correct and much simpler.
> >>>>>>>>
> >>>>>>>> To Jiewen,
> >>>>>>>> Sorry, I don't have environment to reproduce the issue.
> >>>>>>>>
> >>>>>>>> Thanks
> >>>>>>>> Wenyi
> >>>>>>>>
> >>>>>>>> On 2020/8/14 2:50, Laszlo Ersek wrote:
> >>>>>>>>> On 08/13/20 13:55, Wenyi Xie wrote:
> >>>>>>>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> >>>>>>>>>>
> >>>>>>>>>> There is an integer overflow vulnerability in
> >>>>>>>>>> DxeImageVerificationHandler function when parsing the PE files
> >>>>>>>>>> attribute certificate table. In cases where
> >>>>>>>>>> WinCertificate->dwLength is sufficiently large, it's possible to
> >> overflow Offset back to 0 causing an endless loop.
> >>>>>>>>>>
> >>>>>>>>>> Check offset inbetween VirtualAddress and VirtualAddress + Size.
> >>>>>>>>>> Using SafeintLib to do offset addition with result check.
> >>>>>>>>>>
> >>>>>>>>>> Cc: Jiewen Yao <jiewen....@intel.com>
> >>>>>>>>>> Cc: Jian J Wang <jian.j.w...@intel.com>
> >>>>>>>>>> Cc: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>>>> Signed-off-by: Wenyi Xie <xiewen...@huawei.com>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>>>> ib.inf
> >>>>>> |
> >>>>>>>> 1 +
> >>>>>>>>>>
> >>>>>>>>>>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>>>> ib.h
> >>>>>> |
> >>>>>>>> 1 +
> >>>>>>>>>>
> >>>>>>>>>>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>>>> ib.c
> >>>>>> |
> >>>>>>>> 111 +++++++++++---------
> >>>>>>>>>> 3 files changed, 63 insertions(+), 50 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git
> >>>>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.inf
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.inf
> >>>>>>>>>> index 1e1a639857e0..a7ac4830b3d4 100644
> >>>>>>>>>> ---
> >>>>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.inf
> >>>>>>>>>> +++
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.inf
> >>>>>>>>>> @@ -53,6 +53,7 @@ [LibraryClasses]
> >>>>>>>>>> SecurityManagementLib
> >>>>>>>>>> PeCoffLib
> >>>>>>>>>> TpmMeasurementLib
> >>>>>>>>>> + SafeIntLib
> >>>>>>>>>>
> >>>>>>>>>> [Protocols]
> >>>>>>>>>> gEfiFirmwareVolume2ProtocolGuid ##
> SOMETIMES_CONSUMES
> >>>>>>>>>> diff --git
> >>>>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.h
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.h
> >>>>>>>>>> index 17955ff9774c..060273917d5d 100644
> >>>>>>>>>> ---
> >>>>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.h
> >>>>>>>>>> +++
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.h
> >>>>>>>>>> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>>>>>>>> #include <Library/DevicePathLib.h> #include
> >>>>>>>>>> <Library/SecurityManagementLib.h> #include <Library/PeCoffLib.h>
> >>>>>>>>>> +#include <Library/SafeIntLib.h>
> >>>>>>>>>> #include <Protocol/FirmwareVolume2.h> #include
> >>>>>>>>>> <Protocol/DevicePath.h> #include <Protocol/BlockIo.h> diff --git
> >>>>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>>> index 36b87e16d53d..dbc03e28c05b 100644
> >>>>>>>>>> ---
> >>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
> >>>>>> .c
> >>>>>>>>>> +++
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>>> @@ -1658,6 +1658,10 @@ DxeImageVerificationHandler (
> >>>>>>>>>> EFI_STATUS HashStatus;
> >>>>>>>>>> EFI_STATUS DbStatus;
> >>>>>>>>>> BOOLEAN IsFound;
> >>>>>>>>>> + UINT32 AlignedLength;
> >>>>>>>>>> + UINT32 Result;
> >>>>>>>>>> + EFI_STATUS AddStatus;
> >>>>>>>>>> + BOOLEAN IsAuthDataAssigned;
> >>>>>>>>>>
> >>>>>>>>>> SignatureList = NULL;
> >>>>>>>>>> SignatureListSize = 0;
> >>>>>>>>>> @@ -1667,6 +1671,7 @@ DxeImageVerificationHandler (
> >>>>>>>>>> Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
> >>>>>>>>>> IsVerified = FALSE;
> >>>>>>>>>> IsFound = FALSE;
> >>>>>>>>>> + Result = 0;
> >>>>>>>>>>
> >>>>>>>>>> //
> >>>>>>>>>> // Check the image type and get policy setting.
> >>>>>>>>>> @@ -1850,9 +1855,10 @@ DxeImageVerificationHandler (
> >>>>>>>>>> // The first certificate starts at offset
> >>>>>>>>>> (SecDataDir->VirtualAddress) from
> >>>>>> the
> >>>>>>>> start of the file.
> >>>>>>>>>> //
> >>>>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
> >>>>>>>>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> >>>>>>>>>> - OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> >> (WinCertificate-
> >>>>>>>>> dwLength))) {
> >>>>>>>>>> + (OffSet >= SecDataDir->VirtualAddress) && (OffSet <
> >>>>>>>>>> + (SecDataDir-
> >>>>>>>>> VirtualAddress + SecDataDir->Size));) {
> >>>>>>>>>> + IsAuthDataAssigned = FALSE;
> >>>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>>>> + AlignedLength = WinCertificate->dwLength + ALIGN_SIZE
> >>>>>> (WinCertificate-
> >>>>>>>>> dwLength);
> >>>>>>>>>
> >>>>>>>>> I disagree with this patch.
> >>>>>>>>>
> >>>>>>>>> The primary reason for my disagreement is that the bug report
> >>>>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2215#c0> is
> >>>>>>>>> inexact, and so this patch tries to fix the wrong thing.
> >>>>>>>>>
> >>>>>>>>> With edk2 master at commit 65904cdbb33c, it is *not* possible to
> >>>>>>>>> overflow the OffSet variable to zero with "WinCertificate-
> >dwLength"
> >>>>>>>>> *purely*, and cause an endless loop. Note that we have (at commit
> >>>>>>>>> 65904cdbb33c):
> >>>>>>>>>
> >>>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
> >>>>>>>>> OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> >>>>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> >>>>>>>>> (WinCertificate-
> >>>>>>>>> dwLength))) {
> >>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
> >>>>>>>>> <= sizeof
> >>>>>>>> (WIN_CERTIFICATE) ||
> >>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
> >>>>>> WinCertificate-
> >>>>>>>>> dwLength) {
> >>>>>>>>> break;
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> The last sub-condition checks whether the Security Data Directory
> >>>>>>>>> has enough room left for "WinCertificate->dwLength". If not, then
> >>>>>>>>> we break out of the loop.
> >>>>>>>>>
> >>>>>>>>> If we *do* have enough room, that is:
> >>>>>>>>>
> >>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) >=
> >>>>>> WinCertificate-
> >>>>>>>>> dwLength
> >>>>>>>>>
> >>>>>>>>> then we have (by adding OffSet to both sides):
> >>>>>>>>>
> >>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size >= OffSet +
> >>>>>>>>> WinCertificate- dwLength
> >>>>>>>>>
> >>>>>>>>> The left hand side is a known-good UINT32, and so incrementing
> >>>>>>>>> OffSet (a
> >>>>>>>>> UINT32) *solely* by "WinCertificate->dwLength" (also a UINT32)
> >>>>>>>>> does not cause an overflow.
> >>>>>>>>>
> >>>>>>>>> Instead, the problem is with the alignment. The "if" statement
> >>>>>>>>> checks whether we have enough room for "dwLength", but then
> >>>>>>>>> "OffSet" is advanced by "dwLength" *aligned up* to the next
> >>>>>>>>> multiple of 8. And that may indeed cause various overflows.
> >>>>>>>>>
> >>>>>>>>> Now, the main problem with the present patch is that it does not
> >>>>>>>>> fix one of those overflows. Namely, consider that "dwLength" is
> >>>>>>>>> very close to
> >>>>>>>>> MAX_UINT32 (or even think it's exactly MAX_UINT32). Then aligning
> >>>>>>>>> it up to the next multiple of 8 will yield 0. In other words,
> >> "AlignedLength"
> >>>>>>>>> will be zero.
> >>>>>>>>>
> >>>>>>>>> And when that happens, there's going to be an infinite loop just
> >>>>>>>>> the
> >>>>>>>>> same: "OffSet" will not be zero, but it will be *stuck*. The
> >>>>>>>>> SafeUint32Add() call at the bottom will succeed, but it will not
> >>>>>>>>> change the value of "OffSet".
> >>>>>>>>>
> >>>>>>>>> More at the bottom.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
> >>>>>>>>>> <= sizeof
> >>>>>>>> (WIN_CERTIFICATE) ||
> >>>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
> >>>>>>>>>> <
> >>>>>>>> WinCertificate->dwLength) {
> >>>>>>>>>> break;
> >>>>>>>>>> @@ -1872,6 +1878,8 @@ DxeImageVerificationHandler (
> >>>>>>>>>> }
> >>>>>>>>>> AuthData = PkcsCertData->CertData;
> >>>>>>>>>> AuthDataSize = PkcsCertData->Hdr.dwLength -
> >>>>>>>>>> sizeof(PkcsCertData-
> >>>>>>> Hdr);
> >>>>>>>>>> + IsAuthDataAssigned = TRUE;
> >>>>>>>>>> + HashStatus = HashPeImageByType (AuthData, AuthDataSize);
> >>>>>>>>>> } else if (WinCertificate->wCertificateType ==
> >>>>>> WIN_CERT_TYPE_EFI_GUID)
> >>>>>>>> {
> >>>>>>>>>> //
> >>>>>>>>>> // The certificate is formatted as
> >>>>>>>>>> WIN_CERTIFICATE_UEFI_GUID which
> >>>>>> is
> >>>>>>>> described in UEFI Spec.
> >>>>>>>>>> @@ -1880,72 +1888,75 @@ DxeImageVerificationHandler (
> >>>>>>>>>> if (WinCertUefiGuid->Hdr.dwLength <=
> >>>>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData)) {
> >>>>>>>>>> break;
> >>>>>>>>>> }
> >>>>>>>>>> - if (!CompareGuid (&WinCertUefiGuid->CertType,
> >> &gEfiCertPkcs7Guid))
> >>>>>> {
> >>>>>>>>>> - continue;
> >>>>>>>>>> + if (CompareGuid (&WinCertUefiGuid->CertType,
> >>>>>>>>>> + &gEfiCertPkcs7Guid))
> >>>>>> {
> >>>>>>>>>> + AuthData = WinCertUefiGuid->CertData;
> >>>>>>>>>> + AuthDataSize = WinCertUefiGuid->Hdr.dwLength -
> >>>>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData);
> >>>>>>>>>> + IsAuthDataAssigned = TRUE;
> >>>>>>>>>> + HashStatus = HashPeImageByType (AuthData, AuthDataSize);
> >>>>>>>>>> }
> >>>>>>>>>> - AuthData = WinCertUefiGuid->CertData;
> >>>>>>>>>> - AuthDataSize = WinCertUefiGuid->Hdr.dwLength -
> >>>>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData);
> >>>>>>>>>> } else {
> >>>>>>>>>> if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) {
> >>>>>>>>>> break;
> >>>>>>>>>> }
> >>>>>>>>>> - continue;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> - HashStatus = HashPeImageByType (AuthData, AuthDataSize);
> >>>>>>>>>> - if (EFI_ERROR (HashStatus)) {
> >>>>>>>>>> - continue;
> >>>>>>>>>> - }
> >>>>>>>>>> -
> >>>>>>>>>> - //
> >>>>>>>>>> - // Check the digital signature against the revoked
> >>>>>>>>>> certificate in
> >>>>>> forbidden
> >>>>>>>> database (dbx).
> >>>>>>>>>> - //
> >>>>>>>>>> - if (IsForbiddenByDbx (AuthData, AuthDataSize)) {
> >>>>>>>>>> - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED;
> >>>>>>>>>> - IsVerified = FALSE;
> >>>>>>>>>> - break;
> >>>>>>>>>> - }
> >>>>>>>>>> -
> >>>>>>>>>> - //
> >>>>>>>>>> - // Check the digital signature against the valid certificate
> >>>>>>>>>> in
> >> allowed
> >>>>>>>> database (db).
> >>>>>>>>>> - //
> >>>>>>>>>> - if (!IsVerified) {
> >>>>>>>>>> - if (IsAllowedByDb (AuthData, AuthDataSize)) {
> >>>>>>>>>> - IsVerified = TRUE;
> >>>>>>>>>> + if (IsAuthDataAssigned && !EFI_ERROR (HashStatus)) {
> >>>>>>>>>> + //
> >>>>>>>>>> + // Check the digital signature against the revoked
> >>>>>>>>>> + certificate in
> >>>>>> forbidden
> >>>>>>>> database (dbx).
> >>>>>>>>>> + //
> >>>>>>>>>> + if (IsForbiddenByDbx (AuthData, AuthDataSize)) {
> >>>>>>>>>> + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED;
> >>>>>>>>>> + IsVerified = FALSE;
> >>>>>>>>>> + break;
> >>>>>>>>>> }
> >>>>>>>>>> - }
> >>>>>>>>>>
> >>>>>>>>>> - //
> >>>>>>>>>> - // Check the image's hash value.
> >>>>>>>>>> - //
> >>>>>>>>>> - DbStatus = IsSignatureFoundInDatabase (
> >>>>>>>>>> - EFI_IMAGE_SECURITY_DATABASE1,
> >>>>>>>>>> - mImageDigest,
> >>>>>>>>>> - &mCertType,
> >>>>>>>>>> - mImageDigestSize,
> >>>>>>>>>> - &IsFound
> >>>>>>>>>> - );
> >>>>>>>>>> - if (EFI_ERROR (DbStatus) || IsFound) {
> >>>>>>>>>> - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
> >>>>>>>>>> - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> >> signed
> >>>>>> but %s
> >>>>>>>> hash of image is found in DBX.\n", mHashTypeStr));
> >>>>>>>>>> - IsVerified = FALSE;
> >>>>>>>>>> - break;
> >>>>>>>>>> - }
> >>>>>>>>>> + //
> >>>>>>>>>> + // Check the digital signature against the valid
> >>>>>>>>>> + certificate in allowed
> >>>>>>>> database (db).
> >>>>>>>>>> + //
> >>>>>>>>>> + if (!IsVerified) {
> >>>>>>>>>> + if (IsAllowedByDb (AuthData, AuthDataSize)) {
> >>>>>>>>>> + IsVerified = TRUE;
> >>>>>>>>>> + }
> >>>>>>>>>> + }
> >>>>>>>>>>
> >>>>>>>>>> - if (!IsVerified) {
> >>>>>>>>>> + //
> >>>>>>>>>> + // Check the image's hash value.
> >>>>>>>>>> + //
> >>>>>>>>>> DbStatus = IsSignatureFoundInDatabase (
> >>>>>>>>>> - EFI_IMAGE_SECURITY_DATABASE,
> >>>>>>>>>> + EFI_IMAGE_SECURITY_DATABASE1,
> >>>>>>>>>> mImageDigest,
> >>>>>>>>>> &mCertType,
> >>>>>>>>>> mImageDigestSize,
> >>>>>>>>>> &IsFound
> >>>>>>>>>> );
> >>>>>>>>>> - if (!EFI_ERROR (DbStatus) && IsFound) {
> >>>>>>>>>> - IsVerified = TRUE;
> >>>>>>>>>> - } else {
> >>>>>>>>>> - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> >> signed
> >>>>>> but
> >>>>>>>> signature is not allowed by DB and %s hash of image is not found in
> >>>>>> DB/DBX.\n",
> >>>>>>>> mHashTypeStr));
> >>>>>>>>>> + if (EFI_ERROR (DbStatus) || IsFound) {
> >>>>>>>>>> + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
> >>>>>>>>>> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> >>>>>>>>>> + signed
> >>>>>>>> but %s hash of image is found in DBX.\n", mHashTypeStr));
> >>>>>>>>>> + IsVerified = FALSE;
> >>>>>>>>>> + break;
> >>>>>>>>>> }
> >>>>>>>>>> +
> >>>>>>>>>> + if (!IsVerified) {
> >>>>>>>>>> + DbStatus = IsSignatureFoundInDatabase (
> >>>>>>>>>> + EFI_IMAGE_SECURITY_DATABASE,
> >>>>>>>>>> + mImageDigest,
> >>>>>>>>>> + &mCertType,
> >>>>>>>>>> + mImageDigestSize,
> >>>>>>>>>> + &IsFound
> >>>>>>>>>> + );
> >>>>>>>>>> + if (!EFI_ERROR (DbStatus) && IsFound) {
> >>>>>>>>>> + IsVerified = TRUE;
> >>>>>>>>>> + } else {
> >>>>>>>>>> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> >>>>>>>>>> + signed
> >>>>>> but
> >>>>>>>> signature is not allowed by DB and %s hash of image is not found in
> >>>>>> DB/DBX.\n",
> >>>>>>>> mHashTypeStr));
> >>>>>>>>>> + }
> >>>>>>>>>> + }
> >>>>>>>>>> + }
> >>>>>>>>>> +
> >>>>>>>>>> + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result);
> >>>>>>>>>> + if (EFI_ERROR (AddStatus)) {
> >>>>>>>>>> + break;
> >>>>>>>>>> }
> >>>>>>>>>> + OffSet = Result;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size))
> >>>>>>>>>> {
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> There are other (smaller) reasons why I dislike this patch:
> >>>>>>>>>
> >>>>>>>>> - The "IsAuthDataAssigned" variable is superfluous; we could use
> >>>>>>>>> the existent "AuthData" variable (with a NULL-check and a
> >>>>>>>>> NULL-assignment) similarly.
> >>>>>>>>>
> >>>>>>>>> - The patch complicates / reorganizes the control flow needlessly.
> >>>>>>>>> This complication originates from placing the checked "OffSet"
> >>>>>>>>> increment at the bottom of the loop, which then requires the
> >>>>>>>>> removal of all the "continue" statements. But we don't need to
> >>>>>>>>> check-and-increment at the bottom. We can keep the increment
> >>>>>>>>> inside the "for" statement, only extend the *existent* room check
> >>>>>>>>> (which I've quoted) to take the alignment into account as well. If
> >>>>>>>>> there is enough room for the alignment in the security data
> >>>>>>>>> directory, then that guarantees there won't be a UINT32 overflow
> >> either.
> >>>>>>>>>
> >>>>>>>>> All in all, I'm proposing the following three patches instead. The
> >>>>>>>>> first two patches are preparation, the last patch is the fix.
> >>>>>>>>>
> >>>>>>>>> Patch#1:
> >>>>>>>>>
> >>>>>>>>>> From 11af0a104d34d39bf1b1aab256428ae4edbddd77 Mon Sep 17
> >>>>>> 00:00:00
> >>>>>>>> 2001
> >>>>>>>>>> From: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>>>> Date: Thu, 13 Aug 2020 19:11:39 +0200
> >>>>>>>>>> Subject: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract
> >>>>>>>>>> SecDataDirEnd, SecDataDirLeft
> >>>>>>>>>>
> >>>>>>>>>> The following two quantities:
> >>>>>>>>>>
> >>>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size
> >>>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size - OffSet
> >>>>>>>>>>
> >>>>>>>>>> are used multiple times in DxeImageVerificationHandler().
> >>>>>>>>>> Introduce helper variables for them: "SecDataDirEnd" and
> >> "SecDataDirLeft", respectively.
> >>>>>>>>>> This saves us multiple calculations and significantly simplifies
> >>>>>>>>>> the
> code.
> >>>>>>>>>>
> >>>>>>>>>> Note that all three summands above have type UINT32, therefore
> >>>>>>>>>> the new variables are also of type UINT32.
> >>>>>>>>>>
> >>>>>>>>>> This patch does not change behavior.
> >>>>>>>>>>
> >>>>>>>>>> (Note that the code already handles the case when the
> >>>>>>>>>>
> >>>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size
> >>>>>>>>>>
> >>>>>>>>>> UINT32 addition overflows -- namely, in that case, the
> >>>>>>>>>> certificate loop is never entered, and the corruption check right
> >>>>>>>>>> after the loop fires.)
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>>>> ib.c |
> >>>>>> 12
> >>>>>>>> ++++++++----
> >>>>>>>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git
> >>>>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>>> index 36b87e16d53d..8761980c88aa 100644
> >>>>>>>>>> ---
> >>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
> >>>>>> .c
> >>>>>>>>>> +++
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>>> @@ -1652,6 +1652,8 @@ DxeImageVerificationHandler (
> >>>>>>>>>> UINT8 *AuthData;
> >>>>>>>>>> UINTN AuthDataSize;
> >>>>>>>>>> EFI_IMAGE_DATA_DIRECTORY *SecDataDir;
> >>>>>>>>>> + UINT32 SecDataDirEnd;
> >>>>>>>>>> + UINT32 SecDataDirLeft;
> >>>>>>>>>> UINT32 OffSet;
> >>>>>>>>>> CHAR16 *NameStr;
> >>>>>>>>>> RETURN_STATUS PeCoffStatus;
> >>>>>>>>>> @@ -1849,12 +1851,14 @@ DxeImageVerificationHandler (
> >>>>>>>>>> // "Attribute Certificate Table".
> >>>>>>>>>> // The first certificate starts at offset
> >>>>>>>>>> (SecDataDir->VirtualAddress) from
> >>>>>> the
> >>>>>>>> start of the file.
> >>>>>>>>>> //
> >>>>>>>>>> + SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size;
> >>>>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
> >>>>>>>>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> >>>>>>>>>> + OffSet < SecDataDirEnd;
> >>>>>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> >>>>>>>>>> (WinCertificate-
> >>>>>>>>> dwLength))) {
> >>>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>>>> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
> >>>>>>>>>> <=
> >> sizeof
> >>>>>>>> (WIN_CERTIFICATE) ||
> >>>>>>>>>> - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
> >>>>>>>> WinCertificate->dwLength) {
> >>>>>>>>>> + SecDataDirLeft = SecDataDirEnd - OffSet;
> >>>>>>>>>> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> >>>>>>>>>> + SecDataDirLeft < WinCertificate->dwLength) {
> >>>>>>>>>> break;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler (
> >>>>>>>>>> }
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> - if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size))
> >>>>>>>>>> {
> >>>>>>>>>> + if (OffSet != SecDataDirEnd) {
> >>>>>>>>>> //
> >>>>>>>>>> // The Size in Certificate Table or the attribute
> >>>>>>>>>> certificate table is
> >>>>>> corrupted.
> >>>>>>>>>> //
> >>>>>>>>>> --
> >>>>>>>>>> 2.19.1.3.g30247aa5d201
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Patch#2:
> >>>>>>>>>
> >>>>>>>>>> From 72012c065a53582f7df695e7b9730c45f49226c6 Mon Sep 17
> >> 00:00:00
> >>>>>>>> 2001
> >>>>>>>>>> From: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>>>> Date: Thu, 13 Aug 2020 19:19:06 +0200
> >>>>>>>>>> Subject: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign
> >>>>>>>>>> WinCertificate after size check
> >>>>>>>>>>
> >>>>>>>>>> Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check
> >>>>>>>>>> only guards the de-referencing of the "WinCertificate" pointer.
> >>>>>>>>>> It does not guard the calculation of hte pointer itself:
> >>>>>>>>>>
> >>>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>>>>
> >>>>>>>>>> This is wrong; if we don't know for sure that we have enough room
> >>>>>>>>>> for a WIN_CERTIFICATE, then even creating such a pointer, not
> >>>>>>>>>> just de-referencing it, may invoke undefined behavior.
> >>>>>>>>>>
> >>>>>>>>>> Move the pointer calculation after the size check.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>>>> ib.c |
> >>>>>> 8
> >>>>>>>> +++++---
> >>>>>>>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git
> >>>>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>>> index 8761980c88aa..461ed7cfb5ac 100644
> >>>>>>>>>> ---
> >>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
> >>>>>> .c
> >>>>>>>>>> +++
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>>> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler (
> >>>>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
> >>>>>>>>>> OffSet < SecDataDirEnd;
> >>>>>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> >>>>>>>>>> (WinCertificate-
> >>>>>>>>> dwLength))) {
> >>>>>>>>>> - WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>>>> SecDataDirLeft = SecDataDirEnd - OffSet;
> >>>>>>>>>> - if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> >>>>>>>>>> - SecDataDirLeft < WinCertificate->dwLength) {
> >>>>>>>>>> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) {
> >>>>>>>>>> + break;
> >>>>>>>>>> + }
> >>>>>>>>>> + WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>>>> + if (SecDataDirLeft < WinCertificate->dwLength) {
> >>>>>>>>>> break;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> 2.19.1.3.g30247aa5d201
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Patch#3:
> >>>>>>>>>
> >>>>>>>>>> From 0bbba15b84f8f9f2cdc770a89f418aaec6cfb31e Mon Sep 17
> >> 00:00:00
> >>>>>>>> 2001
> >>>>>>>>>> From: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>>>> Date: Thu, 13 Aug 2020 19:34:33 +0200
> >>>>>>>>>> Subject: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch
> >>>>>> alignment
> >>>>>>>>>> overflow (CVE-2019-14562)
> >>>>>>>>>>
> >>>>>>>>>> The DxeImageVerificationHandler() function currently checks
> >>>>>>>>>> whether "SecDataDir" has enough room for
> >>>>>>>>>> "WinCertificate->dwLength". However,
> >>>>>>>> for
> >>>>>>>>>> advancing "OffSet", "WinCertificate->dwLength" is aligned to the
> >>>>>>>>>> next multiple of 8. If "WinCertificate->dwLength" is large
> >>>>>>>>>> enough, the alignment will return 0, and "OffSet" will be stuck at
> the
> >> same value.
> >>>>>>>>>>
> >>>>>>>>>> Check whether "SecDataDir" has room left for both
> >>>>>>>>>> "WinCertificate->dwLength" and the alignment.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>>>> ib.c |
> >>>>>> 4
> >>>>>>>> +++-
> >>>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git
> >>>>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>>> index 461ed7cfb5ac..e38eb981b7a0 100644
> >>>>>>>>>> ---
> >>>>>>
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
> >>>>>> .c
> >>>>>>>>>> +++
> >>>>>>>>
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>>>>>>>> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler (
> >>>>>>>>>> break;
> >>>>>>>>>> }
> >>>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>>>> - if (SecDataDirLeft < WinCertificate->dwLength) {
> >>>>>>>>>> + if (SecDataDirLeft < WinCertificate->dwLength ||
> >>>>>>>>>> + (SecDataDirLeft - WinCertificate->dwLength <
> >>>>>>>>>> + ALIGN_SIZE (WinCertificate->dwLength))) {
> >>>>>>>>>> break;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> 2.19.1.3.g30247aa5d201
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> If Wenyi and the reviewers are OK with these patches, I can submit
> >>>>>>>>> them as a standalone patch series.
> >>>>>>>>>
> >>>>>>>>> Note that I do not have any reproducer for the issue; the best
> >>>>>>>>> testing that I could offer would be some light-weight Secure Boot
> >>>>>>>>> regression tests.
> >>>>>>>>>
> >>>>>>>>> Thanks
> >>>>>>>>> Laszlo
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> .
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>> .
> >>>
> >>
> >>
> >>
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64742): https://edk2.groups.io/g/devel/message/64742
Mute This Topic: https://groups.io/mt/76165658/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
