[edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
Mathews, John
john.mathews at intel.com
Tue Aug 18 15:18:32 UTC 2020
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 <lersek at redhat.com>
Sent: Tuesday, August 18, 2020 1:59 AM
To: Wang, Jian J <jian.j.wang at intel.com>; devel at edk2.groups.io; Yao, Jiewen <jiewen.yao at intel.com>; xiewenyi2 at huawei.com
Cc: huangming23 at huawei.com; songdongkuang at huawei.com; Mathews, John <john.mathews at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>> Sent: Tuesday, August 18, 2020 12:53 AM
>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io;
>> xiewenyi2 at huawei.com; Wang, Jian J <jian.j.wang at intel.com>
>> Cc: huangming23 at huawei.com; songdongkuang at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>> wenyi,xie
>>>> via groups.io
>>>> Sent: Friday, August 14, 2020 3:54 PM
>>>> To: Laszlo Ersek <lersek at redhat.com>; devel at edk2.groups.io; Yao,
>>>> Jiewen <jiewen.yao at intel.com>; Wang, Jian J <jian.j.wang at intel.com>
>>>> Cc: huangming23 at huawei.com; songdongkuang at 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.yao at intel.com>
>>>>>> Cc: Jian J Wang <jian.j.wang at intel.com>
>>>>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>>>>> Signed-off-by: Wenyi Xie <xiewenyi2 at 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 <lersek at 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 <lersek at 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 <lersek at 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 <lersek at 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 <lersek at 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 <lersek at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list