From 9d8bf6efcb18eb24035fd5e7c1d897f52179bcb7 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 1 Sep 2020 11:12:19 +0200
Subject: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract
 SecDataDirEnd, SecDataDirLeft
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Wenyi Xie <xiewenyi2@huawei.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Message-Id: <20200901091221.20948-2-lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Wenyi Xie <xiewenyi2@huawei.com>
Reviewed-by: Min M Xu <min.m.xu@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
(cherry picked from commit 503248ccdf45c14d4040ce44163facdc212e4991)
---
 .../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 8db6ff510be6..28f5e130a1d7 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1674,6 +1674,8 @@ DxeImageVerificationHandler (
   UINT8                                *AuthData;
   UINTN                                AuthDataSize;
   EFI_IMAGE_DATA_DIRECTORY             *SecDataDir;
+  UINT32                               SecDataDirEnd;
+  UINT32                               SecDataDirLeft;
   UINT32                               OffSet;
   CHAR16                               *NameStr;
   RETURN_STATUS                        PeCoffStatus;
@@ -1886,12 +1888,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;
     }
 
@@ -1985,7 +1989,7 @@ DxeImageVerificationHandler (
     }
   }
 
-  if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) {
+  if (OffSet != SecDataDirEnd) {
     //
     // The Size in Certificate Table or the attribute certicate table is corrupted.
     //
-- 
2.28.0


From 983ac55990f66fa980787e573965fc5d70dfc81e Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 1 Sep 2020 11:12:20 +0200
Subject: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign
 WinCertificate after size check
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check only
guards the de-referencing of the "WinCertificate" pointer. It does not
guard the calculation of the 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.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Wenyi Xie <xiewenyi2@huawei.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Message-Id: <20200901091221.20948-3-lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Wenyi Xie <xiewenyi2@huawei.com>
Reviewed-by: Min M Xu <min.m.xu@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
(cherry picked from commit a7632e913c1c106f436aefd5e76c394249c383a8)
---
 .../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 28f5e130a1d7..ec1718653a57 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1892,10 +1892,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.28.0


From c0cf5350b8493a97f1d3fec0ed4bd407491c113d Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 1 Sep 2020 11:12:21 +0200
Subject: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment
 overflow (CVE-2019-14562)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Wenyi Xie <xiewenyi2@huawei.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Message-Id: <20200901091221.20948-4-lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Wenyi Xie <xiewenyi2@huawei.com>
Reviewed-by: Min M Xu <min.m.xu@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
(cherry picked from commit 0b143fa43e92be15d11e22f80773bcb1b2b0608f)
---
 .../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 ec1718653a57..40486dee9acf 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1897,7 +1897,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.28.0

