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 (#64828): https://edk2.groups.io/g/devel/message/64828
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]
-=-=-=-=-=-=-=-=-=-=-=-
