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