Sounds great. Appreciate your hard work on that.
Will you post a patch to fix the issue again?
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of wenyi,xie
> via groups.io
> Sent: Monday, August 31, 2020 7:24 PM
> To: Yao, Jiewen <jiewen....@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 modify the PE file again, this time it can pass the check in PeCoffLib and
> cause
> offset overflow.
>
> First, create a PE file and sign it(only one signature), then using binary
> edit tool
> to modify content of PE file like below,
> 1.check the value of SecDataDir->VirtualAddress, in my PE file, it's 0xE000
> 2.changing SecDataDir->Size from 0x5F8 to 0xFFFF1FFC
> 3.changing WinCertificate->dwLength from 0x5F1 to 0xFFFF1FFB
> 4.padding PE file with 0 until the size of the file is 0xFFFFFFFC(it will
> make the PE
> file so large)
> OffSet + WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength) is
> 0xE000 + 0xFFFF1FFB + 0x5 = 0x100000000
>
> Below is the DEBUG code and log, in second loop the offset overflow and
> become 0
>
> for (OffSet = SecDataDir->VirtualAddress;
> OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-
> >dwLength))) {
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> DEBUG((DEBUG_INFO, "OffSet=0x%x.\n", OffSet));
> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof
> (WIN_CERTIFICATE) ||
> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
> WinCertificate-
> >dwLength) {
> break;
> }
> DEBUG((DEBUG_INFO, "WinCertificate->dwLength=0x%x, ALIGN_SIZE
> (WinCertificate->dwLength)=0x%x.\n", WinCertificate->dwLength,
> ALIGN_SIZE(WinCertificate->dwLength)));
>
>
> SecDataDir->VirtualAddress=0xE000, SecDataDir->Size=0xFFFF1FFC.
> OffSet=0xE000.
> WinCertificate->dwLength=0xFFFF1FFB, ALIGN_SIZE (WinCertificate-
> >dwLength)=0x5.
> DxeImageVerificationLib: Image is signed but signature is not allowed by DB
> and
> SHA256 hash of image is notOffSet=0x0.
> WinCertificate->dwLength=0x5A4D, ALIGN_SIZE (WinCertificate-
> >dwLength)=0x3.
> OffSet=0x5A50.
> WinCertificate->dwLength=0x9024, ALIGN_SIZE (WinCertificate-
> >dwLength)=0x4.
> OffSet=0xEA78.
> WinCertificate->dwLength=0x0, ALIGN_SIZE (WinCertificate->dwLength)=0x0.
> The image doesn't pass verification: VenHw(5CF32E0B-8EDF-2E44-9CDA-
> 93205E99EC1C,00000000)/VenHw(964E5B22-6459-11D2-8E39-
> 00A0C969723B,00000000)/\signed_1234_4G.efi
>
>
> Regards
> Wenyi
>
>
> On 2020/8/28 14:43, Yao, Jiewen wrote:
> > Apology that I did not say clearly.
> > I mean you should not modify any code to perform an attack.
> >
> > I am not asking you to exploit the system. Attack here just means: to cause
> system hang or buffer overflow. That is enough.
> > But you cannot modify code to remove any existing checker.
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> wenyi,xie
> >> via groups.io
> >> Sent: Friday, August 28, 2020 2:18 PM
> >> To: Yao, Jiewen <jiewen....@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 (#64844): https://edk2.groups.io/g/devel/message/64844
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]
-=-=-=-=-=-=-=-=-=-=-=-
