[edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
wenyi,xie via groups.io
xiewenyi2=huawei.com at groups.io
Fri Aug 14 11:29:09 UTC 2020
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 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/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 <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/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 <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/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 <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/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/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