On 2020/8/14 16: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?
Hi, Jiewen,
You're right, as I can't reproduce the issue, I can't guarantee my patches can
fix the problem.
And as Laszlo analyzed, my patches can't solve overflow issue indeed.
Sincerely
Wenyi
>
> Thank you
> Yao Jiewen
>
>
>> -----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 (#64289): https://edk2.groups.io/g/devel/message/64289
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]
-=-=-=-=-=-=-=-=-=-=-=-
