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/DxeImageVerificationLib.inf >> | >>>> 1 + >>>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h >> | >>>> 1 + >>>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >> | >>>> 111 +++++++++++--------- >>>>>> 3 files changed, 63 insertions(+), 50 deletions(-) >>>>>> >>>>>> diff --git >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf >>>>>> index 1e1a639857e0..a7ac4830b3d4 100644 >>>>>> --- >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf >>>>>> +++ >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf >>>>>> @@ -53,6 +53,7 @@ [LibraryClasses] >>>>>> SecurityManagementLib >>>>>> PeCoffLib >>>>>> TpmMeasurementLib >>>>>> + SafeIntLib >>>>>> >>>>>> [Protocols] >>>>>> gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES >>>>>> diff --git >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h >>>>>> index 17955ff9774c..060273917d5d 100644 >>>>>> --- >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h >>>>>> +++ >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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/DxeImageVerificationLib.c >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>>>> index 36b87e16d53d..dbc03e28c05b 100644 >>>>>> --- >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>>>> +++ >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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/DxeImageVerificationLib.c | >> 12 >>>> ++++++++---- >>>>>> 1 file changed, 8 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>>>> index 36b87e16d53d..8761980c88aa 100644 >>>>>> --- >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>>>> +++ >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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/DxeImageVerificationLib.c | >> 8 >>>> +++++--- >>>>>> 1 file changed, 5 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>>>> index 8761980c88aa..461ed7cfb5ac 100644 >>>>>> --- >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>>>> +++ >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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/DxeImageVerificationLib.c | >> 4 >>>> +++- >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>>>> index 461ed7cfb5ac..e38eb981b7a0 100644 >>>>>> --- >> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c >>>>>> +++ >>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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 (#64368): https://edk2.groups.io/g/devel/message/64368 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] -=-=-=-=-=-=-=-=-=-=-=-
