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