[PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset

["wenyi,xie" <xiewenyi2@huawei.com>]

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@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Wenyi Xie <xiewenyi2@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);
     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)) {
-- 
2.20.1.windows.1