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