[edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
wenyi,xie via groups.io
xiewenyi2=huawei.com at groups.io
Fri Aug 28 03:17:45 UTC 2020
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 (#64710): https://edk2.groups.io/g/devel/message/64710
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