-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Tuesday, August 18, 2020 1:59 AM
To: Wang, Jian J <jian.j.wang@...>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; xiewenyi2@...
Cc: huangming23@...; songdongkuang@...; Mathews, John <john.mathews@...>
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Tuesday, August 18, 2020 12:53 AM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
xiewenyi2@...; Wang, Jian J <jian.j.wang@...>
Cc: huangming23@...; songdongkuang@...
Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset

Hi Jiewen,

On 08/14/20 10:53, Yao, Jiewen wrote:

To Jiewen,
Sorry, I don't have environment to reproduce the issue.

Please help me understand, if you don’t have environment to
reproduce the

issue, how do you guarantee that your patch does fix the problem and
we don’t have any other vulnerabilities?

The original bug report in
<https://bugzilla.tianocore.org/show_bug.cgi?id=2215#c0> is seriously
lacking. It does not go into detail about the alleged integer overflow.
It does not quote the code, does not explain the control flow, does
not identify the exact edk2 commit at which the vulnerability exists.

The bug report also does not offer a reproducer.

Additionally, the exact statement that the bug report does make,
namely

it's possible to overflow Offset back to 0 causing an endless loop

is wrong (as far as I can tell anyway). It is not "OffSet" that can
be overflowed to zero, but the *addend* that is added to OffSet can
be overflowed to zero. Therefore the infinite loop will arise because
OffSet remains stuck at its present value, and not because OffSet
will be re-set to zero.

For the reasons, we can only speculate as to what the actual problem
is, unless Jian decides to join the discussion and clarifies what he
had in mind originally.

My understanding (or even "reconstruction") of the vulnerability is
described above, and in the patches that I proposed.

We can write a patch based on code analysis. It's possible to
identify integer overflows based on code analysis, and it's possible
to verify the correctness of fixes by code review. Obviously testing
is always good, but many times, constructing reproducers for such
issues that were found by code review, is difficult and time
consuming. We can say that we don't fix vulnerabilities without
reproducers, or we can say that we make an effort to fix them even if
all we have is code analysis (and not a reproducer).

So the above paragraph concerns "correctness". Regarding
"completeness", I guarantee you that this patch does not fix *all*
problems related to PE parsing. (See the other BZ tickets.) It does
fix *one* issue with PE parsing. We can say that we try to fix such
issues gradually (give different CVE numbers to different issues, and
address them one at a time), or we can say that we rewrite PE parsing from the ground up.
(BTW: I have seriously attempted that in the past, and I gave up,
because the PE format is FUBAR.)

In summary:

- the problem statement is unclear,

- it seems like there is indeed an integer overflow problem in the
SecDataDir parsing loop, but it's uncertain whether the bug reporter
had exactly that in mind

- PE parsing is guaranteed to have other vulnerabilities elsewhere in
edk2, but I'm currently unaware of other such issues in
DxeImageVerificationLib specifically

- even if there are other such problems (in DxeImageVerificationLib
or elswehere), fixing this bug that we know about is likely
worthwhile

- for many such bugs, constructing a reproducer is difficult and time
consuming; code analysis, and *regression-testing* are frequently the
only tools we have. That doesn't mean we should ignore this class of bugs.

(Fixing integer overflows retro-actively is more difficult than
writing overflow-free code in the first place, but that ship has
sailed; so we can only fight these bugs incrementally now, unless we
can rewrite PE parsing with a new data structure from the ground up.
Again I tried that and gave up, because the spec is not public, and
what I did manage to learn about PE, showed that it was insanely
over-engineered. I'm not saying that other binary / executable
formats are better, of course.)

Please check out my patches (inlined elsewhere in this thread), and
comment whether you'd like me to post them to the list as a
standalone series.

Jian: it wouldn't hurt if you commented as well.

Thanks
Laszlo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of

wenyi,xie

via groups.io
Sent: Friday, August 14, 2020 3:54 PM
To: Laszlo Ersek <lersek@...>; devel@edk2.groups.io; Yao,
Jiewen <jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>
Cc: huangming23@...; songdongkuang@...
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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Wenyi Xie <xiewenyi2@...>
---

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@...>
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@...>
---

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@...>
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@...>
---

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@...>
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@...>
---

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


.