SecurityPkg/DxeImageVerificationLib: Differentiate error/search result (1) (CVE-2019...

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608

To avoid false-negative issue in check hash against dbx, both error
condition (as return value) and check result (as out parameter) of
IsCertHashFoundInDatabase() are added. So the caller of this function
will know exactly if a failure is caused by a black list hit or
other error happening, and enforce a more secure operation to prevent
secure boot from being bypassed. For a white list check (db), there's
no such necessity.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 8739d1fa294612ebb930143bac7d157532ec6550..85261ba7f2ae853c7cb67e846bda5297c98c2ef2 100644 (file)
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -822,22 +822,23 @@ AddImageExeInfo (
   @param[in]  SignatureList     Pointer to the Signature List in forbidden database.\r
   @param[in]  SignatureListSize Size of Signature List.\r
   @param[out] RevocationTime    Return the time that the certificate was revoked.\r
   @param[in]  SignatureList     Pointer to the Signature List in forbidden database.\r
   @param[in]  SignatureListSize Size of Signature List.\r
   @param[out] RevocationTime    Return the time that the certificate was revoked.\r

+  @param[out] IsFound           Search result. Only valid if EFI_SUCCESS returned.\r

 \r
 \r

-  @return TRUE   The certificate hash is found in the forbidden database.\r
-  @return FALSE  The certificate hash is not found in the forbidden database.\r
+  @retval EFI_SUCCESS           Finished the search without any error.\r
+  @retval Others                Error occurred in the search of database.\r

 \r
 **/\r
 \r
 **/\r

-BOOLEAN\r
+EFI_STATUS\r

 IsCertHashFoundInDatabase (\r
   IN  UINT8               *Certificate,\r
   IN  UINTN               CertSize,\r
   IN  EFI_SIGNATURE_LIST  *SignatureList,\r
   IN  UINTN               SignatureListSize,\r
 IsCertHashFoundInDatabase (\r
   IN  UINT8               *Certificate,\r
   IN  UINTN               CertSize,\r
   IN  EFI_SIGNATURE_LIST  *SignatureList,\r
   IN  UINTN               SignatureListSize,\r

-  OUT EFI_TIME            *RevocationTime\r
+  OUT EFI_TIME            *RevocationTime,\r
+  OUT BOOLEAN             *IsFound\r

   )\r
 {\r
   )\r
 {\r

-  BOOLEAN             IsFound;\r
-  BOOLEAN             Status;\r
+  EFI_STATUS          Status;\r

   EFI_SIGNATURE_LIST  *DbxList;\r
   UINTN               DbxSize;\r
   EFI_SIGNATURE_DATA  *CertHash;\r
   EFI_SIGNATURE_LIST  *DbxList;\r
   UINTN               DbxSize;\r
   EFI_SIGNATURE_DATA  *CertHash;\r


@@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
   UINT8               *TBSCert;\r
   UINTN               TBSCertSize;\r
 \r
   UINT8               *TBSCert;\r
   UINTN               TBSCertSize;\r
 \r

-  IsFound  = FALSE;\r
+  Status   = EFI_ABORTED;\r
+  *IsFound = FALSE;\r

   DbxList  = SignatureList;\r
   DbxSize  = SignatureListSize;\r
   HashCtx  = NULL;\r
   HashAlg  = HASHALG_MAX;\r
 \r
   if ((RevocationTime == NULL) || (DbxList == NULL)) {\r
   DbxList  = SignatureList;\r
   DbxSize  = SignatureListSize;\r
   HashCtx  = NULL;\r
   HashAlg  = HASHALG_MAX;\r
 \r
   if ((RevocationTime == NULL) || (DbxList == NULL)) {\r

-    return FALSE;\r
+    return EFI_INVALID_PARAMETER;\r

   }\r
 \r
   //\r
   // Retrieve the TBSCertificate from the X.509 Certificate.\r
   //\r
   if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {\r
   }\r
 \r
   //\r
   // Retrieve the TBSCertificate from the X.509 Certificate.\r
   //\r
   if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {\r

-    return FALSE;\r
+    return Status;\r

   }\r
 \r
   while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {\r
   }\r
 \r
   while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {\r


@@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
     if (HashCtx == NULL) {\r
       goto Done;\r
     }\r
     if (HashCtx == NULL) {\r
       goto Done;\r
     }\r

-    Status = mHash[HashAlg].HashInit (HashCtx);\r
-    if (!Status) {\r
+    if (!mHash[HashAlg].HashInit (HashCtx)) {\r

       goto Done;\r
     }\r
       goto Done;\r
     }\r

-    Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);\r
-    if (!Status) {\r
+    if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {\r

       goto Done;\r
     }\r
       goto Done;\r
     }\r

-    Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);\r
-    if (!Status) {\r
+    if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {\r

       goto Done;\r
     }\r
 \r
       goto Done;\r
     }\r
 \r


@@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
         //\r
         // Hash of Certificate is found in forbidden database.\r
         //\r
         //\r
         // Hash of Certificate is found in forbidden database.\r
         //\r

-        IsFound = TRUE;\r
+        Status   = EFI_SUCCESS;\r
+        *IsFound = TRUE;\r

 \r
         //\r
         // Return the revocation time.\r
 \r
         //\r
         // Return the revocation time.\r


@@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
     DbxList  = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList->SignatureListSize);\r
   }\r
 \r
     DbxList  = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList->SignatureListSize);\r
   }\r
 \r

+  Status = EFI_SUCCESS;\r
+\r

 Done:\r
   if (HashCtx != NULL) {\r
     FreePool (HashCtx);\r
   }\r
 \r
 Done:\r
   if (HashCtx != NULL) {\r
     FreePool (HashCtx);\r
   }\r
 \r

-  return IsFound;\r
+  return Status;\r

 }\r
 \r
 /**\r
 }\r
 \r
 /**\r


@@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
 {\r
   EFI_STATUS                Status;\r
   BOOLEAN                   IsForbidden;\r
 {\r
   EFI_STATUS                Status;\r
   BOOLEAN                   IsForbidden;\r

+  BOOLEAN                   IsFound;\r

   UINT8                     *Data;\r
   UINTN                     DataSize;\r
   EFI_SIGNATURE_LIST        *CertList;\r
   UINT8                     *Data;\r
   UINTN                     DataSize;\r
   EFI_SIGNATURE_LIST        *CertList;\r


@@ -1344,20 +1347,29 @@ IsForbiddenByDbx (
     //\r
     CertPtr = CertPtr + sizeof (UINT32) + CertSize;\r
 \r
     //\r
     CertPtr = CertPtr + sizeof (UINT32) + CertSize;\r
 \r

-    if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data, DataSize, &RevocationTime)) {\r
+    Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data, DataSize, &RevocationTime, &IsFound);\r
+    if (EFI_ERROR (Status)) {\r

       //\r
       //\r

-      // Check the timestamp signature and signing time to determine if the image can be trusted.\r
+      // Error in searching dbx. Consider it as 'found'. RevocationTime might\r
+      // not be valid in such situation.\r

       //\r
       IsForbidden = TRUE;\r
       //\r
       IsForbidden = TRUE;\r

+    } else if (IsFound) {\r
+      //\r
+      // Found Cert in dbx successfully. Check the timestamp signature and\r
+      // signing time to determine if the image can be trusted.\r
+      //\r

       if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {\r
         IsForbidden = FALSE;\r
         //\r
         // Pass DBT check. Continue to check other certs in image signer's cert list against DBX, DBT\r
         //\r
         continue;\r
       if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {\r
         IsForbidden = FALSE;\r
         //\r
         // Pass DBT check. Continue to check other certs in image signer's cert list against DBX, DBT\r
         //\r
         continue;\r

+      } else {\r
+        IsForbidden = TRUE;\r
+        DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature failed the timestamp check.\n"));\r
+        goto Done;\r

       }\r
       }\r

-      DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature failed the timestamp check.\n"));\r
-      goto Done;\r

     }\r
 \r
   }\r
     }\r
 \r
   }\r


@@ -1392,6 +1404,7 @@ IsAllowedByDb (
 {\r
   EFI_STATUS                Status;\r
   BOOLEAN                   VerifyStatus;\r
 {\r
   EFI_STATUS                Status;\r
   BOOLEAN                   VerifyStatus;\r

+  BOOLEAN                   IsFound;\r

   EFI_SIGNATURE_LIST        *CertList;\r
   EFI_SIGNATURE_DATA        *CertData;\r
   UINTN                     DataSize;\r
   EFI_SIGNATURE_LIST        *CertList;\r
   EFI_SIGNATURE_DATA        *CertData;\r
   UINTN                     DataSize;\r


@@ -1498,7 +1511,14 @@ IsAllowedByDb (
             //\r
             // Here We still need to check if this RootCert's Hash is revoked\r
             //\r
             //\r
             // Here We still need to check if this RootCert's Hash is revoked\r
             //\r

-            if (IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {\r
+            Status = IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);\r
+            if (EFI_ERROR (Status)) {\r
+              //\r
+              // Error in searching dbx. Consider it as 'found'. RevocationTime might\r
+              // not be valid in such situation.\r
+              //\r
+              VerifyStatus = FALSE;\r
+            } else if (IsFound) {\r

               //\r
               // Check the timestamp signature and signing time to determine if the RootCert can be trusted.\r
               //\r
               //\r
               // Check the timestamp signature and signing time to determine if the RootCert can be trusted.\r
               //\r