[edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
wenyi,xie via groups.io
xiewenyi2=huawei.com at groups.io
Tue Sep 1 07:10:54 UTC 2020
I think Laszlo's patches is OK, I have applied and tested it using my case. It can catch the issue.
DEBUG code and log below,
SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size;
DEBUG((DEBUG_INFO, "SecDataDirEnd=0x%x.\n", SecDataDirEnd));
for (OffSet = SecDataDir->VirtualAddress;
OffSet < SecDataDirEnd;
OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) {
SecDataDirLeft = SecDataDirEnd - OffSet;
DEBUG((DEBUG_INFO, "OffSet=0x%x, SecDataDirLeft=0x%x.\n", OffSet, SecDataDirLeft));
if (SecDataDirLeft <= sizeof(WIN_CERTIFICATE)) {
break;
}
WinCertificate = (WIN_CERTIFICATE *)(mImageBase + OffSet);
DEBUG((DEBUG_INFO, "WinCertificate->dwLength=0x%x, ALIGN_SIZE (WinCertificate->dwLength)=0x%x.\n", WinCertificate->dwLength, ALIGN_SIZE(WinCertificate->dwLength)));
if (SecDataDirLeft < WinCertificate->dwLength ||
(SecDataDirLeft - WinCertificate->dwLength < ALIGN_SIZE(WinCertificate->dwLength))) {
DEBUG((DEBUG_INFO, "Parameter is invalid and break.\n"));
break;
}
SecDataDirEnd=0xFFFFFFFC.
OffSet=0xE000, SecDataDirLeft=0xFFFF1FFC.
WinCertificate->dwLength=0xFFFF1FFB, ALIGN_SIZE (WinCertificate->dwLength)=0x5.
Parameter is invalid and break.
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/9/1 0:06, Yao, Jiewen wrote:
> 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of wenyi,xie
>> via groups.io
>> Sent: Monday, August 31, 2020 7:24 PM
>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Laszlo Ersek
>> <lersek at redhat.com>; Wang, Jian J <jian.j.wang at intel.com>
>> Cc: songdongkuang at huawei.com; Mathews, John <john.mathews at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>> wenyi,xie
>>>> via groups.io
>>>> Sent: Friday, August 28, 2020 2:18 PM
>>>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Laszlo
>> Ersek
>>>> <lersek at redhat.com>; Wang, Jian J <jian.j.wang at intel.com>
>>>> Cc: songdongkuang at huawei.com; Mathews, John
>> <john.mathews at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>>>> wenyi,xie
>>>>>> via groups.io
>>>>>> Sent: Friday, August 28, 2020 11:18 AM
>>>>>> To: Laszlo Ersek <lersek at redhat.com>; Wang, Jian J
>>>> <jian.j.wang at intel.com>;
>>>>>> devel at edk2.groups.io; Yao, Jiewen <jiewen.yao at intel.com>
>>>>>> Cc: songdongkuang at huawei.com; Mathews, John
>>>> <john.mathews at intel.com>
>>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
>>>>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
>>>>>>
>>>>>> Hi，Laszlo and everyone，
>>>>>>
>>>>>> These days I tried to reproduce the issue，and made some progress. I
>> think
>>>>>> there are two way to cause overflow from a mathematical point of view,
>>>>>> 1.As Laszlo analysed before, if WinCertificate->dwLength is large enough,
>>>> close
>>>>>> to MAX_UINT32, then (WinCertificate->dwLength + ALIGN_SIZE
>>>> (WinCertificate-
>>>>>>> dwLength)) will cause overflow.
>>>>>> 2.(WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)) is
>>>> good,
>>>>>> OffSet is good, but OffSet += (WinCertificate->dwLength + ALIGN_SIZE
>>>>>> (WinCertificate->dwLength)) cause overflow.
>>>>>>
>>>>>> Here I choose the second way to reproduce the issue, I choose a PE file
>> and
>>>> sign
>>>>>> it with my own db certificate. Then I use binary edit tool to modify the PE
>> file
>>>> like
>>>>>> below,
>>>>>>
>>>>>> 1.change SecDataDir->Size from 0x5F8 to 0xFFFF1FFF
>>>>>> 2.change WinCertificate->dwLength from 0x5F1 to 0xFFFF1FFE
>>>>>> SecDataDir->VirtualAddress in my PE is 0xe000 and no need to change.
>>>>>>
>>>>>> As PE file is modified, function PeCoffLoaderGetImageInfo will return error,
>>>> so I
>>>>>> have to remove it so that for loop can be tested in
>>>> DxeImageVerificationHandler.
>>>>>> The log is as below,
>>>>>>
>>>>>> SecDataDir->VirtualAddress=0xE000, SecDataDir->Size=0xFFFF1FFF.
>>>>>> (First Loop)
>>>>>> OffSet=0xE000.
>>>>>> WinCertificate->dwLength=0xFFFF1FFE, ALIGN_SIZE (WinCertificate-
>>>>>>> dwLength)=0x2.
>>>>>> (Second Loop)
>>>>>> OffSet=0x0.
>>>>>> WinCertificate->dwLength=0x5A4D, ALIGN_SIZE (WinCertificate-
>>>>>>> dwLength)=0x3.
>>>>>> (Third Loop)
>>>>>> OffSet=0x5A50.
>>>>>> WinCertificate->dwLength=0x9024, ALIGN_SIZE (WinCertificate-
>>>>>>> dwLength)=0x4.
>>>>>> (Forth Loop)
>>>>>> OffSet=0xEA78.
>>>>>> WinCertificate->dwLength=0xAFAFAFAF, ALIGN_SIZE (WinCertificate-
>>>>>>> dwLength)=0x1.
>>>>>> (Fifth Loop)
>>>>>> OffSet=0xAFB09A28.
>>>>>>
>>>>>> As I modify SecDataDir->Size and WinCertificate->dwLength, so in first
>> loop
>>>> the
>>>>>> condition check whether the Security Data Directory has enough room left
>>>> for
>>>>>> "WinCertificate->dwLength" is ok.
>>>>>>
>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof
>>>>>> (WIN_CERTIFICATE) ||
>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
>> WinCertificate-
>>>>>>> dwLength) {
>>>>>> break;
>>>>>> }
>>>>>>
>>>>>> In the beginning of second loop, WinCertificate->dwLength + ALIGN_SIZE
>>>>>> (WinCertificate->dwLength) is 0xFFFF2000, offset is 0xE000
>>>>>>
>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-
>>>>> dwLength))
>>>>>>
>>>>>> Offset now is 0 and overflow happens. So even if my PE only have one
>>>> signature,
>>>>>> the for loop is still going untill exception happens.
>>>>>>
>>>>>>
>>>>>> I can't reproduce the issue using the first way, because if WinCertificate-
>>>>>>> dwLength is close to MAX_UINT32, it means SecDataDir->Size should also
>>>> close
>>>>>> to MAX_UINT32, or the condition check
>>>>>> "(SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
>> WinCertificate-
>>>>>>> dwLength" will break. But if SecDataDir->Size is very large, SecDataDir-
>>>>>>> VirtualAddress have to be smaller than 8 bytes,
>>>>>> because SecDataDir->VirtualAddress + SecDataDir->Size < MAX_UINT32.
>>>>>> SecDataDir->VirtualAddress is the beginning address of the signature, and
>>>> before
>>>>>> SecDataDir->VirtualAddress is the content of origin PE file, I think it's
>>>> impossible
>>>>>> that the size of PE file is only 8 bytes.
>>>>>>
>>>>>> As I changed the one line code in DxeImageVerificationHandler to
>> reproduce
>>>> the
>>>>>> issue, I'm not sure whether it's ok.
>>>>>>
>>>>>> Thanks
>>>>>> Wenyi
>>>>>>
>>>>>> On 2020/8/19 17:26, Laszlo Ersek wrote:
>>>>>>> On 08/18/20 17:18, Mathews, John wrote:
>>>>>>>> I dug up the original report details. This was noted as a concern during a
>>>>>> source code inspection. There was no demonstration of how it might be
>>>>>> triggered.
>>>>>>>>
>>>>>>>> " There is an integer overflow vulnerability in the
>>>>>> DxeImageVerificationHandler function when
>>>>>>>> parsing the PE files attribute certificate table. In cases where
>>>> WinCertificate-
>>>>>>> dwLength is
>>>>>>>> sufficiently large, it's possible to overflow Offset back to 0 causing an
>>>> endless
>>>>>> loop."
>>>>>>>>
>>>>>>>> The recommendation was to add stricter checking of "Offset" and the
>>>>>> embedded length fields of certificate data
>>>>>>>> before using them.
>>>>>>>
>>>>>>> Thanks for checking!
>>>>>>>
>>>>>>> Laszlo
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Laszlo Ersek <lersek at redhat.com>
>>>>>>>> Sent: Tuesday, August 18, 2020 1:59 AM
>>>>>>>> To: Wang, Jian J <jian.j.wang at intel.com>; devel at edk2.groups.io; Yao,
>>>>>> Jiewen <jiewen.yao at intel.com>; xiewenyi2 at huawei.com
>>>>>>>> Cc: huangming23 at huawei.com; songdongkuang at huawei.com;
>> Mathews,
>>>>>> John <john.mathews at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>>>> Laszlo
>>>>>>>>>> Ersek
>>>>>>>>>> Sent: Tuesday, August 18, 2020 12:53 AM
>>>>>>>>>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io;
>>>>>>>>>> xiewenyi2 at huawei.com; Wang, Jian J <jian.j.wang at intel.com>
>>>>>>>>>> Cc: huangming23 at huawei.com; songdongkuang at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf
>> Of
>>>>>>>>>> wenyi,xie
>>>>>>>>>>>> via groups.io
>>>>>>>>>>>> Sent: Friday, August 14, 2020 3:54 PM
>>>>>>>>>>>> To: Laszlo Ersek <lersek at redhat.com>; devel at edk2.groups.io; Yao,
>>>>>>>>>>>> Jiewen <jiewen.yao at intel.com>; Wang, Jian J
>>>> <jian.j.wang at intel.com>
>>>>>>>>>>>> Cc: huangming23 at huawei.com; songdongkuang at huawei.com
>>>>>>>>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
>>>>>>>>>>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of
>> Offset
>>>>>>>>>>>>
>>>>>>>>>>>> To Laszlo,
>>>>>>>>>>>> Thank you for your detailed description, I agree with what you
>>>>>>>>>>>> analyzed and
>>>>>>>>>> I'm
>>>>>>>>>>>> OK with your patches, it's
>>>>>>>>>>>> correct and much simpler.
>>>>>>>>>>>>
>>>>>>>>>>>> To Jiewen,
>>>>>>>>>>>> Sorry, I don't have environment to reproduce the issue.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Wenyi
>>>>>>>>>>>>
>>>>>>>>>>>> On 2020/8/14 2:50, Laszlo Ersek wrote:
>>>>>>>>>>>>> On 08/13/20 13:55, Wenyi Xie wrote:
>>>>>>>>>>>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> There is an integer overflow vulnerability in
>>>>>>>>>>>>>> DxeImageVerificationHandler function when parsing the PE files
>>>>>>>>>>>>>> attribute certificate table. In cases where
>>>>>>>>>>>>>> WinCertificate->dwLength is sufficiently large, it's possible to
>>>>>> overflow Offset back to 0 causing an endless loop.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Check offset inbetween VirtualAddress and VirtualAddress + Size.
>>>>>>>>>>>>>> Using SafeintLib to do offset addition with result check.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>>>>>>>>>>>>>> Cc: Jian J Wang <jian.j.wang at intel.com>
>>>>>>>>>>>>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>>>>>>>>>>>>> Signed-off-by: Wenyi Xie <xiewenyi2 at huawei.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>>>> ib.inf
>>>>>>>>>> |
>>>>>>>>>>>> 1 +
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>>>> ib.h
>>>>>>>>>> |
>>>>>>>>>>>> 1 +
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>>>> ib.c
>>>>>>>>>> |
>>>>>>>>>>>> 111 +++++++++++---------
>>>>>>>>>>>>>> 3 files changed, 63 insertions(+), 50 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git
>>>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.inf
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.inf
>>>>>>>>>>>>>> index 1e1a639857e0..a7ac4830b3d4 100644
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.inf
>>>>>>>>>>>>>> +++
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.inf
>>>>>>>>>>>>>> @@ -53,6 +53,7 @@ [LibraryClasses]
>>>>>>>>>>>>>> SecurityManagementLib
>>>>>>>>>>>>>> PeCoffLib
>>>>>>>>>>>>>> TpmMeasurementLib
>>>>>>>>>>>>>> + SafeIntLib
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [Protocols]
>>>>>>>>>>>>>> gEfiFirmwareVolume2ProtocolGuid ##
>>>> SOMETIMES_CONSUMES
>>>>>>>>>>>>>> diff --git
>>>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.h
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.h
>>>>>>>>>>>>>> index 17955ff9774c..060273917d5d 100644
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.h
>>>>>>>>>>>>>> +++
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.h
>>>>>>>>>>>>>> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-
>> Patent
>>>>>>>>>>>>>> #include <Library/DevicePathLib.h> #include
>>>>>>>>>>>>>> <Library/SecurityManagementLib.h> #include
>> <Library/PeCoffLib.h>
>>>>>>>>>>>>>> +#include <Library/SafeIntLib.h>
>>>>>>>>>>>>>> #include <Protocol/FirmwareVolume2.h> #include
>>>>>>>>>>>>>> <Protocol/DevicePath.h> #include <Protocol/BlockIo.h> diff --
>> git
>>>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>>> index 36b87e16d53d..dbc03e28c05b 100644
>>>>>>>>>>>>>> ---
>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
>>>>>>>>>> .c
>>>>>>>>>>>>>> +++
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>>> @@ -1658,6 +1658,10 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> EFI_STATUS HashStatus;
>>>>>>>>>>>>>> EFI_STATUS DbStatus;
>>>>>>>>>>>>>> BOOLEAN IsFound;
>>>>>>>>>>>>>> + UINT32 AlignedLength;
>>>>>>>>>>>>>> + UINT32 Result;
>>>>>>>>>>>>>> + EFI_STATUS AddStatus;
>>>>>>>>>>>>>> + BOOLEAN IsAuthDataAssigned;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> SignatureList = NULL;
>>>>>>>>>>>>>> SignatureListSize = 0;
>>>>>>>>>>>>>> @@ -1667,6 +1671,7 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
>>>>>>>>>>>>>> IsVerified = FALSE;
>>>>>>>>>>>>>> IsFound = FALSE;
>>>>>>>>>>>>>> + Result = 0;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> //
>>>>>>>>>>>>>> // Check the image type and get policy setting.
>>>>>>>>>>>>>> @@ -1850,9 +1855,10 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> // The first certificate starts at offset
>>>>>>>>>>>>>> (SecDataDir->VirtualAddress) from
>>>>>>>>>> the
>>>>>>>>>>>> start of the file.
>>>>>>>>>>>>>> //
>>>>>>>>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
>>>>>>>>>>>>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
>>>>>>>>>>>>>> - OffSet += (WinCertificate->dwLength + ALIGN_SIZE
>>>>>> (WinCertificate-
>>>>>>>>>>>>> dwLength))) {
>>>>>>>>>>>>>> + (OffSet >= SecDataDir->VirtualAddress) && (OffSet <
>>>>>>>>>>>>>> + (SecDataDir-
>>>>>>>>>>>>> VirtualAddress + SecDataDir->Size));) {
>>>>>>>>>>>>>> + IsAuthDataAssigned = FALSE;
>>>>>>>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>>>>>>>>>>>>>> + AlignedLength = WinCertificate->dwLength + ALIGN_SIZE
>>>>>>>>>> (WinCertificate-
>>>>>>>>>>>>> dwLength);
>>>>>>>>>>>>>
>>>>>>>>>>>>> I disagree with this patch.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The primary reason for my disagreement is that the bug report
>>>>>>>>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2215#c0> is
>>>>>>>>>>>>> inexact, and so this patch tries to fix the wrong thing.
>>>>>>>>>>>>>
>>>>>>>>>>>>> With edk2 master at commit 65904cdbb33c, it is *not* possible to
>>>>>>>>>>>>> overflow the OffSet variable to zero with "WinCertificate-
>>>>> dwLength"
>>>>>>>>>>>>> *purely*, and cause an endless loop. Note that we have (at
>> commit
>>>>>>>>>>>>> 65904cdbb33c):
>>>>>>>>>>>>>
>>>>>>>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
>>>>>>>>>>>>> OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
>>>>>>>>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
>>>>>>>>>>>>> (WinCertificate-
>>>>>>>>>>>>> dwLength))) {
>>>>>>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>>>>>>>>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
>>>>>>>>>>>>> <= sizeof
>>>>>>>>>>>> (WIN_CERTIFICATE) ||
>>>>>>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
>>>>>>>>>> WinCertificate-
>>>>>>>>>>>>> dwLength) {
>>>>>>>>>>>>> break;
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> The last sub-condition checks whether the Security Data Directory
>>>>>>>>>>>>> has enough room left for "WinCertificate->dwLength". If not, then
>>>>>>>>>>>>> we break out of the loop.
>>>>>>>>>>>>>
>>>>>>>>>>>>> If we *do* have enough room, that is:
>>>>>>>>>>>>>
>>>>>>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) >=
>>>>>>>>>> WinCertificate-
>>>>>>>>>>>>> dwLength
>>>>>>>>>>>>>
>>>>>>>>>>>>> then we have (by adding OffSet to both sides):
>>>>>>>>>>>>>
>>>>>>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size >= OffSet +
>>>>>>>>>>>>> WinCertificate- dwLength
>>>>>>>>>>>>>
>>>>>>>>>>>>> The left hand side is a known-good UINT32, and so incrementing
>>>>>>>>>>>>> OffSet (a
>>>>>>>>>>>>> UINT32) *solely* by "WinCertificate->dwLength" (also a UINT32)
>>>>>>>>>>>>> does not cause an overflow.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Instead, the problem is with the alignment. The "if" statement
>>>>>>>>>>>>> checks whether we have enough room for "dwLength", but then
>>>>>>>>>>>>> "OffSet" is advanced by "dwLength" *aligned up* to the next
>>>>>>>>>>>>> multiple of 8. And that may indeed cause various overflows.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Now, the main problem with the present patch is that it does not
>>>>>>>>>>>>> fix one of those overflows. Namely, consider that "dwLength" is
>>>>>>>>>>>>> very close to
>>>>>>>>>>>>> MAX_UINT32 (or even think it's exactly MAX_UINT32). Then
>> aligning
>>>>>>>>>>>>> it up to the next multiple of 8 will yield 0. In other words,
>>>>>> "AlignedLength"
>>>>>>>>>>>>> will be zero.
>>>>>>>>>>>>>
>>>>>>>>>>>>> And when that happens, there's going to be an infinite loop just
>>>>>>>>>>>>> the
>>>>>>>>>>>>> same: "OffSet" will not be zero, but it will be *stuck*. The
>>>>>>>>>>>>> SafeUint32Add() call at the bottom will succeed, but it will not
>>>>>>>>>>>>> change the value of "OffSet".
>>>>>>>>>>>>>
>>>>>>>>>>>>> More at the bottom.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
>>>>>>>>>>>>>> <= sizeof
>>>>>>>>>>>> (WIN_CERTIFICATE) ||
>>>>>>>>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
>>>>>>>>>>>>>> <
>>>>>>>>>>>> WinCertificate->dwLength) {
>>>>>>>>>>>>>> break;
>>>>>>>>>>>>>> @@ -1872,6 +1878,8 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> AuthData = PkcsCertData->CertData;
>>>>>>>>>>>>>> AuthDataSize = PkcsCertData->Hdr.dwLength -
>>>>>>>>>>>>>> sizeof(PkcsCertData-
>>>>>>>>>>> Hdr);
>>>>>>>>>>>>>> + IsAuthDataAssigned = TRUE;
>>>>>>>>>>>>>> + HashStatus = HashPeImageByType (AuthData, AuthDataSize);
>>>>>>>>>>>>>> } else if (WinCertificate->wCertificateType ==
>>>>>>>>>> WIN_CERT_TYPE_EFI_GUID)
>>>>>>>>>>>> {
>>>>>>>>>>>>>> //
>>>>>>>>>>>>>> // The certificate is formatted as
>>>>>>>>>>>>>> WIN_CERTIFICATE_UEFI_GUID which
>>>>>>>>>> is
>>>>>>>>>>>> described in UEFI Spec.
>>>>>>>>>>>>>> @@ -1880,72 +1888,75 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> if (WinCertUefiGuid->Hdr.dwLength <=
>>>>>>>>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData)) {
>>>>>>>>>>>>>> break;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> - if (!CompareGuid (&WinCertUefiGuid->CertType,
>>>>>> &gEfiCertPkcs7Guid))
>>>>>>>>>> {
>>>>>>>>>>>>>> - continue;
>>>>>>>>>>>>>> + if (CompareGuid (&WinCertUefiGuid->CertType,
>>>>>>>>>>>>>> + &gEfiCertPkcs7Guid))
>>>>>>>>>> {
>>>>>>>>>>>>>> + AuthData = WinCertUefiGuid->CertData;
>>>>>>>>>>>>>> + AuthDataSize = WinCertUefiGuid->Hdr.dwLength -
>>>>>>>>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData);
>>>>>>>>>>>>>> + IsAuthDataAssigned = TRUE;
>>>>>>>>>>>>>> + HashStatus = HashPeImageByType (AuthData,
>> AuthDataSize);
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> - AuthData = WinCertUefiGuid->CertData;
>>>>>>>>>>>>>> - AuthDataSize = WinCertUefiGuid->Hdr.dwLength -
>>>>>>>>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData);
>>>>>>>>>>>>>> } else {
>>>>>>>>>>>>>> if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) {
>>>>>>>>>>>>>> break;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> - continue;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - HashStatus = HashPeImageByType (AuthData, AuthDataSize);
>>>>>>>>>>>>>> - if (EFI_ERROR (HashStatus)) {
>>>>>>>>>>>>>> - continue;
>>>>>>>>>>>>>> - }
>>>>>>>>>>>>>> -
>>>>>>>>>>>>>> - //
>>>>>>>>>>>>>> - // Check the digital signature against the revoked certificate
>> in
>>>>>>>>>> forbidden
>>>>>>>>>>>> database (dbx).
>>>>>>>>>>>>>> - //
>>>>>>>>>>>>>> - if (IsForbiddenByDbx (AuthData, AuthDataSize)) {
>>>>>>>>>>>>>> - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED;
>>>>>>>>>>>>>> - IsVerified = FALSE;
>>>>>>>>>>>>>> - break;
>>>>>>>>>>>>>> - }
>>>>>>>>>>>>>> -
>>>>>>>>>>>>>> - //
>>>>>>>>>>>>>> - // Check the digital signature against the valid certificate in
>>>>>> allowed
>>>>>>>>>>>> database (db).
>>>>>>>>>>>>>> - //
>>>>>>>>>>>>>> - if (!IsVerified) {
>>>>>>>>>>>>>> - if (IsAllowedByDb (AuthData, AuthDataSize)) {
>>>>>>>>>>>>>> - IsVerified = TRUE;
>>>>>>>>>>>>>> + if (IsAuthDataAssigned && !EFI_ERROR (HashStatus)) {
>>>>>>>>>>>>>> + //
>>>>>>>>>>>>>> + // Check the digital signature against the revoked
>>>>>>>>>>>>>> + certificate in
>>>>>>>>>> forbidden
>>>>>>>>>>>> database (dbx).
>>>>>>>>>>>>>> + //
>>>>>>>>>>>>>> + if (IsForbiddenByDbx (AuthData, AuthDataSize)) {
>>>>>>>>>>>>>> + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED;
>>>>>>>>>>>>>> + IsVerified = FALSE;
>>>>>>>>>>>>>> + break;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> - }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - //
>>>>>>>>>>>>>> - // Check the image's hash value.
>>>>>>>>>>>>>> - //
>>>>>>>>>>>>>> - DbStatus = IsSignatureFoundInDatabase (
>>>>>>>>>>>>>> - EFI_IMAGE_SECURITY_DATABASE1,
>>>>>>>>>>>>>> - mImageDigest,
>>>>>>>>>>>>>> - &mCertType,
>>>>>>>>>>>>>> - mImageDigestSize,
>>>>>>>>>>>>>> - &IsFound
>>>>>>>>>>>>>> - );
>>>>>>>>>>>>>> - if (EFI_ERROR (DbStatus) || IsFound) {
>>>>>>>>>>>>>> - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
>>>>>>>>>>>>>> - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
>>>>>> signed
>>>>>>>>>> but %s
>>>>>>>>>>>> hash of image is found in DBX.\n", mHashTypeStr));
>>>>>>>>>>>>>> - IsVerified = FALSE;
>>>>>>>>>>>>>> - break;
>>>>>>>>>>>>>> - }
>>>>>>>>>>>>>> + //
>>>>>>>>>>>>>> + // Check the digital signature against the valid
>>>>>>>>>>>>>> + certificate in allowed
>>>>>>>>>>>> database (db).
>>>>>>>>>>>>>> + //
>>>>>>>>>>>>>> + if (!IsVerified) {
>>>>>>>>>>>>>> + if (IsAllowedByDb (AuthData, AuthDataSize)) {
>>>>>>>>>>>>>> + IsVerified = TRUE;
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - if (!IsVerified) {
>>>>>>>>>>>>>> + //
>>>>>>>>>>>>>> + // Check the image's hash value.
>>>>>>>>>>>>>> + //
>>>>>>>>>>>>>> DbStatus = IsSignatureFoundInDatabase (
>>>>>>>>>>>>>> - EFI_IMAGE_SECURITY_DATABASE,
>>>>>>>>>>>>>> + EFI_IMAGE_SECURITY_DATABASE1,
>>>>>>>>>>>>>> mImageDigest,
>>>>>>>>>>>>>> &mCertType,
>>>>>>>>>>>>>> mImageDigestSize,
>>>>>>>>>>>>>> &IsFound
>>>>>>>>>>>>>> );
>>>>>>>>>>>>>> - if (!EFI_ERROR (DbStatus) && IsFound) {
>>>>>>>>>>>>>> - IsVerified = TRUE;
>>>>>>>>>>>>>> - } else {
>>>>>>>>>>>>>> - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
>>>>>> signed
>>>>>>>>>> but
>>>>>>>>>>>> signature is not allowed by DB and %s hash of image is not found in
>>>>>>>>>> DB/DBX.\n",
>>>>>>>>>>>> mHashTypeStr));
>>>>>>>>>>>>>> + if (EFI_ERROR (DbStatus) || IsFound) {
>>>>>>>>>>>>>> + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
>>>>>>>>>>>>>> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
>>>>>>>>>>>>>> + signed
>>>>>>>>>>>> but %s hash of image is found in DBX.\n", mHashTypeStr));
>>>>>>>>>>>>>> + IsVerified = FALSE;
>>>>>>>>>>>>>> + break;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!IsVerified) {
>>>>>>>>>>>>>> + DbStatus = IsSignatureFoundInDatabase (
>>>>>>>>>>>>>> + EFI_IMAGE_SECURITY_DATABASE,
>>>>>>>>>>>>>> + mImageDigest,
>>>>>>>>>>>>>> + &mCertType,
>>>>>>>>>>>>>> + mImageDigestSize,
>>>>>>>>>>>>>> + &IsFound
>>>>>>>>>>>>>> + );
>>>>>>>>>>>>>> + if (!EFI_ERROR (DbStatus) && IsFound) {
>>>>>>>>>>>>>> + IsVerified = TRUE;
>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image
>> is
>>>>>>>>>>>>>> + signed
>>>>>>>>>> but
>>>>>>>>>>>> signature is not allowed by DB and %s hash of image is not found in
>>>>>>>>>> DB/DBX.\n",
>>>>>>>>>>>> mHashTypeStr));
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result);
>>>>>>>>>>>>>> + if (EFI_ERROR (AddStatus)) {
>>>>>>>>>>>>>> + break;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> + OffSet = Result;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size))
>>>>>>>>>>>>>> {
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> There are other (smaller) reasons why I dislike this patch:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - The "IsAuthDataAssigned" variable is superfluous; we could use
>>>>>>>>>>>>> the existent "AuthData" variable (with a NULL-check and a
>>>>>>>>>>>>> NULL-assignment) similarly.
>>>>>>>>>>>>>
>>>>>>>>>>>>> - The patch complicates / reorganizes the control flow needlessly.
>>>>>>>>>>>>> This complication originates from placing the checked "OffSet"
>>>>>>>>>>>>> increment at the bottom of the loop, which then requires the
>>>>>>>>>>>>> removal of all the "continue" statements. But we don't need to
>>>>>>>>>>>>> check-and-increment at the bottom. We can keep the increment
>>>>>>>>>>>>> inside the "for" statement, only extend the *existent* room check
>>>>>>>>>>>>> (which I've quoted) to take the alignment into account as well. If
>>>>>>>>>>>>> there is enough room for the alignment in the security data
>>>>>>>>>>>>> directory, then that guarantees there won't be a UINT32 overflow
>>>>>> either.
>>>>>>>>>>>>>
>>>>>>>>>>>>> All in all, I'm proposing the following three patches instead. The
>>>>>>>>>>>>> first two patches are preparation, the last patch is the fix.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Patch#1:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> From 11af0a104d34d39bf1b1aab256428ae4edbddd77 Mon Sep
>> 17
>>>>>>>>>> 00:00:00
>>>>>>>>>>>> 2001
>>>>>>>>>>>>>> From: Laszlo Ersek <lersek at redhat.com>
>>>>>>>>>>>>>> Date: Thu, 13 Aug 2020 19:11:39 +0200
>>>>>>>>>>>>>> Subject: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib:
>> extract
>>>>>>>>>>>>>> SecDataDirEnd, SecDataDirLeft
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The following two quantities:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size
>>>>>>>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size - OffSet
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> are used multiple times in DxeImageVerificationHandler().
>>>>>>>>>>>>>> Introduce helper variables for them: "SecDataDirEnd" and
>>>>>> "SecDataDirLeft", respectively.
>>>>>>>>>>>>>> This saves us multiple calculations and significantly simplifies the
>>>> code.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Note that all three summands above have type UINT32,
>> therefore
>>>>>>>>>>>>>> the new variables are also of type UINT32.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This patch does not change behavior.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> (Note that the code already handles the case when the
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> UINT32 addition overflows -- namely, in that case, the
>>>>>>>>>>>>>> certificate loop is never entered, and the corruption check right
>>>>>>>>>>>>>> after the loop fires.)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>>>> ib.c |
>>>>>>>>>> 12
>>>>>>>>>>>> ++++++++----
>>>>>>>>>>>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git
>>>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>>> index 36b87e16d53d..8761980c88aa 100644
>>>>>>>>>>>>>> ---
>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
>>>>>>>>>> .c
>>>>>>>>>>>>>> +++
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>>> @@ -1652,6 +1652,8 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> UINT8 *AuthData;
>>>>>>>>>>>>>> UINTN AuthDataSize;
>>>>>>>>>>>>>> EFI_IMAGE_DATA_DIRECTORY *SecDataDir;
>>>>>>>>>>>>>> + UINT32 SecDataDirEnd;
>>>>>>>>>>>>>> + UINT32 SecDataDirLeft;
>>>>>>>>>>>>>> UINT32 OffSet;
>>>>>>>>>>>>>> CHAR16 *NameStr;
>>>>>>>>>>>>>> RETURN_STATUS PeCoffStatus;
>>>>>>>>>>>>>> @@ -1849,12 +1851,14 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> // "Attribute Certificate Table".
>>>>>>>>>>>>>> // The first certificate starts at offset
>>>>>>>>>>>>>> (SecDataDir->VirtualAddress) from
>>>>>>>>>> the
>>>>>>>>>>>> start of the file.
>>>>>>>>>>>>>> //
>>>>>>>>>>>>>> + SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir-
>>> Size;
>>>>>>>>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
>>>>>>>>>>>>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
>>>>>>>>>>>>>> + OffSet < SecDataDirEnd;
>>>>>>>>>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
>>>>>>>>>>>>>> (WinCertificate-
>>>>>>>>>>>>> dwLength))) {
>>>>>>>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>>>>>>>>>>>>>> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <=
>>>>>> sizeof
>>>>>>>>>>>> (WIN_CERTIFICATE) ||
>>>>>>>>>>>>>> - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
>>>>>>>>>>>> WinCertificate->dwLength) {
>>>>>>>>>>>>>> + SecDataDirLeft = SecDataDirEnd - OffSet;
>>>>>>>>>>>>>> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
>>>>>>>>>>>>>> + SecDataDirLeft < WinCertificate->dwLength) {
>>>>>>>>>>>>>> break;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size))
>>>>>>>>>>>>>> {
>>>>>>>>>>>>>> + if (OffSet != SecDataDirEnd) {
>>>>>>>>>>>>>> //
>>>>>>>>>>>>>> // The Size in Certificate Table or the attribute
>>>>>>>>>>>>>> certificate table is
>>>>>>>>>> corrupted.
>>>>>>>>>>>>>> //
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> 2.19.1.3.g30247aa5d201
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Patch#2:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> From 72012c065a53582f7df695e7b9730c45f49226c6 Mon Sep
>> 17
>>>>>> 00:00:00
>>>>>>>>>>>> 2001
>>>>>>>>>>>>>> From: Laszlo Ersek <lersek at redhat.com>
>>>>>>>>>>>>>> Date: Thu, 13 Aug 2020 19:19:06 +0200
>>>>>>>>>>>>>> Subject: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib:
>> assign
>>>>>>>>>>>>>> WinCertificate after size check
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE))
>> check
>>>>>>>>>>>>>> only guards the de-referencing of the "WinCertificate" pointer.
>>>>>>>>>>>>>> It does not guard the calculation of hte pointer itself:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is wrong; if we don't know for sure that we have enough
>> room
>>>>>>>>>>>>>> for a WIN_CERTIFICATE, then even creating such a pointer, not
>>>>>>>>>>>>>> just de-referencing it, may invoke undefined behavior.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Move the pointer calculation after the size check.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>>>> ib.c |
>>>>>>>>>> 8
>>>>>>>>>>>> +++++---
>>>>>>>>>>>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git
>>>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>>> index 8761980c88aa..461ed7cfb5ac 100644
>>>>>>>>>>>>>> ---
>>>>>>>>>>
>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
>>>>>>>>>> .c
>>>>>>>>>>>>>> +++
>>>>>>>>>>>>
>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
>>>>>>>>>>>> ib.c
>>>>>>>>>>>>>> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler (
>>>>>>>>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
>>>>>>>>>>>>>> OffSet < SecDataDirEnd;
>>>>>>>>>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
>>>>>>>>>>>>>> (WinCertificate-
>>>>>>>>>>>>> dwLength))) {
>>>>>>>>>>>>>> - WinCertificate = (WIN_CERTIFICATE *) (mImageBase +
>> OffSet);
>>>>>>>>>>>>>> SecDataDirLeft = SecDataDirEnd - OffSet;
>>>>>>>>>>>>>> - if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
>>>>>>>>>>>>>> - SecDataDirLeft < WinCertificate->dwLength) {
>>>>>>>>>>>>>> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) {
>>>>>>>>>>>>>> + break;
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> + WinCertificate = (WIN_CERTIFICATE *) (mImageBase +
>> OffSet);
>>>>>>>>>>>>>> + if (SecDataDirLeft < WinCertificate->dwLength) {
>>>>>>>>>>>>>> break;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> 2.19.1.3.g30247aa5d201
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Patch#3:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> From 0bbba15b84f8f9f2cdc770a89f418aaec6cfb31e Mon Sep 17
>>>>>> 00:00:00
>>>>>>>>>>>> 2001
>>>>>>>>>>>>>> From: Laszlo Ersek <lersek at redhat.com>
>>>>>>>>>>>>>> Date: Thu, 13 Aug 2020 19:34:33 +0200
>>>>>>>>>>>>>> Subject: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch
>>>>>>>>>> alignment
>>>>>>>>>>>>>> overflow (CVE-2019-14562)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The DxeImageVerificationHandler() function currently checks
>>>>>>>>>>>>>> whether "SecDataDir" has enough room for
>>>>>>>>>>>>>> "WinCertificate->dwLength". However,
>>>>>>>>>>>> for
>>>>>>>>>>>>>> advancing "OffSet", "WinCertificate->dwLength" is aligned to the
>>>>>>>>>>>>>> next multiple of 8. If "WinCertificate->dwLength" is large
>>>>>>>>>>>>>> enough, the alignment will return 0, and "OffSet" will be stuck at
>>>> the
>>>>>> same value.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Check whether "SecDataDir" has room left for both
>>>>>>>>>>>>>> "WinCertificate->dwLength" and the alignment.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Laszlo Ersek <lersek at 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 (#64871): https://edk2.groups.io/g/devel/message/64871
Mute This Topic: https://groups.io/mt/76165658/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list