[edk2-devel] [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset(CVE-2019-14562)

[wenyi,xie via groups.io]

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: Chao Zhang <chao.b.zh...@intel.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   | 21 
+++++++++++++++-----
 3 files changed, 18 insertions(+), 5 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..2b42d4595f2c 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1658,6 +1658,9 @@ DxeImageVerificationHandler (
   EFI_STATUS                           HashStatus;
   EFI_STATUS                           DbStatus;
   BOOLEAN                              IsFound;
+  UINT32                               AlignedLength;
+  UINT32                               Result;
+  EFI_STATUS                           AddStatus;
 
   SignatureList     = NULL;
   SignatureListSize = 0;
@@ -1667,6 +1670,7 @@ DxeImageVerificationHandler (
   Action            = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
   IsVerified        = FALSE;
   IsFound           = FALSE;
+  Result            = 0;
 
   //
   // Check the image type and get policy setting.
@@ -1850,9 +1854,9 @@ 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));) {
     WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
+    AlignedLength = WinCertificate->dwLength + ALIGN_SIZE 
(WinCertificate->dwLength);
     if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof 
(WIN_CERTIFICATE) ||
         (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < 
WinCertificate->dwLength) {
       break;
@@ -1881,7 +1885,7 @@ DxeImageVerificationHandler (
         break;
       }
       if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) {
-        continue;
+        goto NEXT_LOOP;
       }
       AuthData = WinCertUefiGuid->CertData;
       AuthDataSize = WinCertUefiGuid->Hdr.dwLength - 
OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData);
@@ -1889,12 +1893,12 @@ DxeImageVerificationHandler (
       if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) {
         break;
       }
-      continue;
+      goto NEXT_LOOP;
     }
 
     HashStatus = HashPeImageByType (AuthData, AuthDataSize);
     if (EFI_ERROR (HashStatus)) {
-      continue;
+      goto NEXT_LOOP;
     }
 
     //
@@ -1946,6 +1950,13 @@ DxeImageVerificationHandler (
         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));
       }
     }
+
+NEXT_LOOP:
+    AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result);
+    if (EFI_ERROR (AddStatus)) {
+      break;
+    }
+    OffSet = Result;
   }
 
   if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) {
-- 
2.20.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#64060): https://edk2.groups.io/g/devel/message/64060
Mute This Topic: https://groups.io/mt/76143922/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-