Thanks Laszlo. Good feedback. I think we need clarify the role and responsibility for the activity.
I provide my understanding and thought, and I would like to have more feedback from other people. > -----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. [Jiewen] Yes, I agree with you that this is problematic. > > The bug report also does not offer a reproducer. [Jiewen] I don’t believe it is reporter's responsibility to provide a reproducer. The reporter may provide a reproducer to convince other people the issue is there. It is encouraged and highly recommended, but it is NOT mandatory. Many times, people just review the code and happen to find something suspicious. People can report the suspicious and ask for help on investigation. We should encourage that activity. However, I do think the producer is mandatory for a fix or at least a security fix. The owner to fix the issue should guarantee the patch is good. The owner shall never rely on the code reviewer to figure out if the patch is good and complete. I have some bad experience that bug owner just wrote a patch and tried to fix a problem, without any test. And it happened passed code review from someone who does not well understand the problem, but give rb based upon the time pressure. Later, the fix was approved to be useless. In my memory, at least 3 cases were security fix. They are found, just because they are sensitive, more people took a look later. It was simple. It was one-line change. But it has not test, and it was wrong. "It was ridiculous" -- commented by the people who find the so-called security fix does not fix the issue. > > 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. [Jiewen] Would you please clarify what do you mean "we" here? If "we" means the bug dispatcher, it is totally OK. The dispatcher just assign the bug. If "we" means the developer assigned to fix the bug, it is NOT OK. The developer should take the responsibility to understand the problem. > > My understanding (or even "reconstruction") of the vulnerability is > described above, and in the patches that I proposed. [Jiewen] Yes. I tend to agree with you on the analysis. Appreciate your help! > > 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). [Jiewen] I would say: yes and no. Yes, I agree with you that it might be difficult and time consuming to construct the reproducer. However, "obviously" is a subject term. Someone may think something is obvious, but other people does not. We should be clear the responsibility of the patch provider is to provide high quality patch. Having basic unit test is the best way to prove that the fix is good. I have seen bad cases when I ask for the test for patch, then the answer I got is: "I test the windows boot". But the test - windows boot - has nothing related to the patch. It only proves no regression, but cannot prove the issue described is resolved. Let's think again in this case, if the patch provider does some basic unit test, he/she may find out the problem by himself/herself. That can save other people's time to review. I don’t prefer to move the responsibility from patch provider to the code reviewer to check if the fix is good. Otherwise, the code reviewer may be overwhelmed. We may clarify and document the role and responsibility in EDKII clearly. Once that is ready, we can follow the rule. Before that is ready, in this particular case, I still prefer we have producer to prove the patch is good. > > 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.) [Jiewen] Maybe there is misunderstanding. I do not mean to let patch provider to fix all issue in PE parsing. Just like we cannot file one Bugzilla to fix all issue in EDKII - it is unfair. What I mean is that the patch provider should guarantee the correctness and completeness of the issue in the bug report. One faked bad example of correctness is: A bug report is file to say: the code has overflow class A. The factor is: the code has overflow class A at line X and line Y. The patch only modified some code at line X, but the overflow class A at line X still exists. One faked bad example of completeness is: A bug report is file to say: the code has overflow class A. The factor is: the code has overflow class A at line X and line Y. The patch only fixed the overflow class A at line X but not line Y. The patch provider should take responsibility to do that work seriously to find out issue in line X and line Y and fix them. He/she may choose to just fix line X and line Y. Rewrite is whole module is NOT required. > > In summary: > > - the problem statement is unclear, [Jiewen] Agree. > > - 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 [Jiewen] Agree. > > - PE parsing is guaranteed to have other vulnerabilities elsewhere in > edk2, but I'm currently unaware of other such issues in > DxeImageVerificationLib specifically [Jiewen] Agree. > > - even if there are other such problems (in DxeImageVerificationLib or > elswehere), fixing this bug that we know about is likely worthwhile [Jiewen] Agree. > > - 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. [Jiewen] I agree with you above two sentence. And I still believe the reproducer is required. > > (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. [Jiewen] From code structure perspective, I agree with you on the analysis. Appreciate that. If I can give some comment, I would think about the provide the fix in BasePeCoffLib. The reason is that: The PE parsing may scattered in the EDKII. The BasePeCoffLib will be consumed as the root of parsing. Just in case, there is other parsing logic. It may consume the BasePeCoffLib but not ImageVerificationLib. Having fix in ImageVerificationLib does not help to resolve the problem in other parser. If we can reject the bad image in BasePeCoffLib, it would be the best way. But I might be myopia and overlook some problem. As such, having reproducer is the best way to prove and give confidence to other people. > > 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 (#64338): https://edk2.groups.io/g/devel/message/64338 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] -=-=-=-=-=-=-=-=-=-=-=-
