[v1] Fix false negative issue in DxeImageVerificationHandler

Expand all Fold all

[edk2-devel] [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)

Posted by Wang, Jian J 4 years, 9 months ago

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>
---
 .../DxeImageVerificationLib.c                 | 68 +++++++++++--------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 8739d1fa29..a5dfee0f8e 100644
--- 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.
   @param[in]  SignatureListSize Size of Signature List.
   @param[out] RevocationTime    Return the time that the certificate was revoked.
+  @param[out] IsFound           Search result. Only valid if EFI_SUCCESS returned.
 
-  @return TRUE   The certificate hash is found in the forbidden database.
-  @return FALSE  The certificate hash is not found in the forbidden database.
+  @retval EFI_SUCCESS           Finished the search without any error.
+  @retval Others                Error occurred in the search of database.
 
 **/
-BOOLEAN
+EFI_STATUS
 IsCertHashFoundInDatabase (
   IN  UINT8               *Certificate,
   IN  UINTN               CertSize,
   IN  EFI_SIGNATURE_LIST  *SignatureList,
   IN  UINTN               SignatureListSize,
-  OUT EFI_TIME            *RevocationTime
+  OUT EFI_TIME            *RevocationTime,
+  OUT BOOLEAN             *IsFound
   )
 {
-  BOOLEAN             IsFound;
-  BOOLEAN             Status;
+  EFI_STATUS          Status;
   EFI_SIGNATURE_LIST  *DbxList;
   UINTN               DbxSize;
   EFI_SIGNATURE_DATA  *CertHash;
@@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
   UINT8               *TBSCert;
   UINTN               TBSCertSize;
 
-  IsFound  = FALSE;
+  Status   = EFI_ABORTED;
+  *IsFound = FALSE;
   DbxList  = SignatureList;
   DbxSize  = SignatureListSize;
   HashCtx  = NULL;
   HashAlg  = HASHALG_MAX;
 
   if ((RevocationTime == NULL) || (DbxList == NULL)) {
-    return FALSE;
+    return EFI_INVALID_PARAMETER;
   }
 
   //
   // Retrieve the TBSCertificate from the X.509 Certificate.
   //
   if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
-    return FALSE;
+    return Status;
   }
 
   while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
@@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
     if (HashCtx == NULL) {
       goto Done;
     }
-    Status = mHash[HashAlg].HashInit (HashCtx);
-    if (!Status) {
+    if (!mHash[HashAlg].HashInit (HashCtx)) {
       goto Done;
     }
-    Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
-    if (!Status) {
+    if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
       goto Done;
     }
-    Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
-    if (!Status) {
+    if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
       goto Done;
     }
 
@@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
         //
         // Hash of Certificate is found in forbidden database.
         //
-        IsFound = TRUE;
+        Status   = EFI_SUCCESS;
+        *IsFound = TRUE;
 
         //
         // Return the revocation time.
@@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
     DbxList  = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList->SignatureListSize);
   }
 
+  Status = EFI_SUCCESS;
+
 Done:
   if (HashCtx != NULL) {
     FreePool (HashCtx);
   }
 
-  return IsFound;
+  return Status;
 }
 
 /**
@@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
 {
   EFI_STATUS                Status;
   BOOLEAN                   IsForbidden;
+  BOOLEAN                   IsFound;
   UINT8                     *Data;
   UINTN                     DataSize;
   EFI_SIGNATURE_LIST        *CertList;
@@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
     //
     CertPtr = CertPtr + sizeof (UINT32) + CertSize;
 
-    if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data, DataSize, &RevocationTime)) {
+    Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data, DataSize, &RevocationTime, &IsFound);
+    if (EFI_ERROR (Status) || IsFound) {
       //
       // Check the timestamp signature and signing time to determine if the image can be trusted.
       //
       IsForbidden = TRUE;
-      if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
+      if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
         IsForbidden = FALSE;
         //
         // Pass DBT check. Continue to check other certs in image signer's cert list against DBX, DBT
@@ -1392,6 +1396,7 @@ IsAllowedByDb (
 {
   EFI_STATUS                Status;
   BOOLEAN                   VerifyStatus;
+  BOOLEAN                   IsFound;
   EFI_SIGNATURE_LIST        *CertList;
   EFI_SIGNATURE_DATA        *CertData;
   UINTN                     DataSize;
@@ -1495,17 +1500,26 @@ IsAllowedByDb (
           // The image is signed and its signature is found in 'db'.
           //
           if (DbxData != NULL) {
+            VerifyStatus = FALSE;
             //
             // Here We still need to check if this RootCert's Hash is revoked
             //
-            if (IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
-              //
-              // Check the timestamp signature and signing time to determine if the RootCert can be trusted.
-              //
-              VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
-              if (!VerifyStatus) {
-                DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and signature is accepted by DB, but its root cert failed the timestamp check.\n"));
-              }
+            Status = IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
+            if (EFI_ERROR (Status)) {
+              goto Done;
+            }
+
+            if (!IsFound) {
+              VerifyStatus = TRUE;
+              goto Done;
+            }
+
+            //
+            // Check the timestamp signature and signing time to determine if the RootCert can be trusted.
+            //
+            VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
+            if (!VerifyStatus) {
+              DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and signature is accepted by DB, but its root cert failed the timestamp check.\n"));
             }
           }
 
-- 
2.24.0.windows.2


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

View/Reply Online (#53872): https://edk2.groups.io/g/devel/message/53872
Mute This Topic: https://groups.io/mt/71023424/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)

Posted by Yao, Jiewen 4 years, 9 months ago

Comment below:

1) I think the function name - IsCertHashFoundInDatabase() and the implementation {  DbxList  = SignatureList;   DbxSize  = SignatureListSize; } bring some confusion to me.

If this is a *generic* database search function, I recommend we use a generic name - not use DbxList/DbxSize in the function implementation.

If the input SignatureList of the function must be *Dbx*, I recommend we use IsCertHashFoundInDbx() as the function name.

Either change is OK for me.

2) Now we have to check 2 output: Status and IsFound in IsCertHashFoundInDatabase().

I am struggling to understand the different between 2 different ways of error handling:

===========================
    Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data, DataSize, &RevocationTime, &IsFound);
    if (EFI_ERROR (Status) || IsFound) {
      //
      // Check the timestamp signature and signing time to determine if the image can be trusted.
      //
      IsForbidden = TRUE;
      if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
        IsForbidden = FALSE;
============================

and

============================
            VerifyStatus = FALSE;
            //
            // Here We still need to check if this RootCert's Hash is revoked
            //
            Status = IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
            if (EFI_ERROR (Status)) {
              goto Done;
            }

            if (!IsFound) {
              VerifyStatus = TRUE;
              goto Done;
            }

            //
            // Check the timestamp signature and signing time to determine if the RootCert can be trusted.
            //
            VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
            if (!VerifyStatus) {
===============================

I *believe* the logic behind is same. If so, we can use a consistent way to check the 2 output and decide if PassTimestampCheck() is required.

Or, can we create a one single function to perform such check for both IsCertHashFoundInDatabase() and PassTimestampCheck() ?

If I am wrong, there is *difference* between them. Then I think we need much better description to help reviewer to catch the difference.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Thursday, February 6, 2020 10:20 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error
> and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> 
> 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>
> ---
>  .../DxeImageVerificationLib.c                 | 68 +++++++++++--------
>  1 file changed, 41 insertions(+), 27 deletions(-)
> 
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 8739d1fa29..a5dfee0f8e 100644
> --- 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.
> 
>    @param[in]  SignatureListSize Size of Signature List.
> 
>    @param[out] RevocationTime    Return the time that the certificate was
> revoked.
> 
> +  @param[out] IsFound           Search result. Only valid if EFI_SUCCESS returned.
> 
> 
> 
> -  @return TRUE   The certificate hash is found in the forbidden database.
> 
> -  @return FALSE  The certificate hash is not found in the forbidden database.
> 
> +  @retval EFI_SUCCESS           Finished the search without any error.
> 
> +  @retval Others                Error occurred in the search of database.
> 
> 
> 
>  **/
> 
> -BOOLEAN
> 
> +EFI_STATUS
> 
>  IsCertHashFoundInDatabase (
> 
>    IN  UINT8               *Certificate,
> 
>    IN  UINTN               CertSize,
> 
>    IN  EFI_SIGNATURE_LIST  *SignatureList,
> 
>    IN  UINTN               SignatureListSize,
> 
> -  OUT EFI_TIME            *RevocationTime
> 
> +  OUT EFI_TIME            *RevocationTime,
> 
> +  OUT BOOLEAN             *IsFound
> 
>    )
> 
>  {
> 
> -  BOOLEAN             IsFound;
> 
> -  BOOLEAN             Status;
> 
> +  EFI_STATUS          Status;
> 
>    EFI_SIGNATURE_LIST  *DbxList;
> 
>    UINTN               DbxSize;
> 
>    EFI_SIGNATURE_DATA  *CertHash;
> 
> @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
>    UINT8               *TBSCert;
> 
>    UINTN               TBSCertSize;
> 
> 
> 
> -  IsFound  = FALSE;
> 
> +  Status   = EFI_ABORTED;
> 
> +  *IsFound = FALSE;
> 
>    DbxList  = SignatureList;
> 
>    DbxSize  = SignatureListSize;
> 
>    HashCtx  = NULL;
> 
>    HashAlg  = HASHALG_MAX;
> 
> 
> 
>    if ((RevocationTime == NULL) || (DbxList == NULL)) {
> 
> -    return FALSE;
> 
> +    return EFI_INVALID_PARAMETER;
> 
>    }
> 
> 
> 
>    //
> 
>    // Retrieve the TBSCertificate from the X.509 Certificate.
> 
>    //
> 
>    if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> 
> -    return FALSE;
> 
> +    return Status;
> 
>    }
> 
> 
> 
>    while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
> 
> @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
>      if (HashCtx == NULL) {
> 
>        goto Done;
> 
>      }
> 
> -    Status = mHash[HashAlg].HashInit (HashCtx);
> 
> -    if (!Status) {
> 
> +    if (!mHash[HashAlg].HashInit (HashCtx)) {
> 
>        goto Done;
> 
>      }
> 
> -    Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> 
> -    if (!Status) {
> 
> +    if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> 
>        goto Done;
> 
>      }
> 
> -    Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> 
> -    if (!Status) {
> 
> +    if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> 
>        goto Done;
> 
>      }
> 
> 
> 
> @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
>          //
> 
>          // Hash of Certificate is found in forbidden database.
> 
>          //
> 
> -        IsFound = TRUE;
> 
> +        Status   = EFI_SUCCESS;
> 
> +        *IsFound = TRUE;
> 
> 
> 
>          //
> 
>          // Return the revocation time.
> 
> @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
>      DbxList  = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> >SignatureListSize);
> 
>    }
> 
> 
> 
> +  Status = EFI_SUCCESS;
> 
> +
> 
>  Done:
> 
>    if (HashCtx != NULL) {
> 
>      FreePool (HashCtx);
> 
>    }
> 
> 
> 
> -  return IsFound;
> 
> +  return Status;
> 
>  }
> 
> 
> 
>  /**
> 
> @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
>  {
> 
>    EFI_STATUS                Status;
> 
>    BOOLEAN                   IsForbidden;
> 
> +  BOOLEAN                   IsFound;
> 
>    UINT8                     *Data;
> 
>    UINTN                     DataSize;
> 
>    EFI_SIGNATURE_LIST        *CertList;
> 
> @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
>      //
> 
>      CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> 
> 
> 
> -    if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data,
> DataSize, &RevocationTime)) {
> 
> +    Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> *)Data, DataSize, &RevocationTime, &IsFound);
> 
> +    if (EFI_ERROR (Status) || IsFound) {
> 
>        //
> 
>        // Check the timestamp signature and signing time to determine if the image
> can be trusted.
> 
>        //
> 
>        IsForbidden = TRUE;
> 
> -      if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
> 
> +      if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime)) {
> 
>          IsForbidden = FALSE;
> 
>          //
> 
>          // Pass DBT check. Continue to check other certs in image signer's cert list
> against DBX, DBT
> 
> @@ -1392,6 +1396,7 @@ IsAllowedByDb (
>  {
> 
>    EFI_STATUS                Status;
> 
>    BOOLEAN                   VerifyStatus;
> 
> +  BOOLEAN                   IsFound;
> 
>    EFI_SIGNATURE_LIST        *CertList;
> 
>    EFI_SIGNATURE_DATA        *CertData;
> 
>    UINTN                     DataSize;
> 
> @@ -1495,17 +1500,26 @@ IsAllowedByDb (
>            // The image is signed and its signature is found in 'db'.
> 
>            //
> 
>            if (DbxData != NULL) {
> 
> +            VerifyStatus = FALSE;
> 
>              //
> 
>              // Here We still need to check if this RootCert's Hash is revoked
> 
>              //
> 
> -            if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> 
> -              //
> 
> -              // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
> 
> -              //
> 
> -              VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
> 
> -              if (!VerifyStatus) {
> 
> -                DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and
> signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> 
> -              }
> 
> +            Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
> 
> +            if (EFI_ERROR (Status)) {
> 
> +              goto Done;
> 
> +            }
> 
> +
> 
> +            if (!IsFound) {
> 
> +              VerifyStatus = TRUE;
> 
> +              goto Done;
> 
> +            }
> 
> +
> 
> +            //
> 
> +            // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
> 
> +            //
> 
> +            VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
> 
> +            if (!VerifyStatus) {
> 
> +              DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and
> signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> 
>              }
> 
>            }
> 
> 
> 
> --
> 2.24.0.windows.2


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

View/Reply Online (#54351): https://edk2.groups.io/g/devel/message/54351
Mute This Topic: https://groups.io/mt/71023424/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)

Posted by Wang, Jian J 4 years, 9 months ago

Jiewen,

Thanks for the comments.

1) You're right. IsCertHashFoundInDatabase is quite general and cause confusions between
db and dbx situation. Since it's not newly introduced in this patch series, do you think it's ok
to fix it in separate patch series later? Or do you prefer fix it in this patch series? I'm ok with
both.

2) I checked both code again. I think you're right. Both callings are for dbx, any error Status
should be taken as IsFound(==TRUE). What about following change for the second case?
Please help double check if any logic hole here.

            Status = IsCertHashFoundInDatabase (...);
            if (EFI_ERROR (Status) || IsFound) {
              //
              // Check the timestamp signature and signing time to determine if the RootCert can be trusted.
              //
              VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
              if (!VerifyStatus) {
                DEBUG ((...));
              }
            } else  {
              VerifyStatus = TRUE;
            }

             goto Done;

Regards,
Jian

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Thursday, February 13, 2020 6:11 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> 
> Comment below:
> 
> 1) I think the function name - IsCertHashFoundInDatabase() and the
> implementation {  DbxList  = SignatureList;   DbxSize  = SignatureListSize; } bring
> some confusion to me.
> 
> If this is a *generic* database search function, I recommend we use a generic
> name - not use DbxList/DbxSize in the function implementation.
> 
> If the input SignatureList of the function must be *Dbx*, I recommend we use
> IsCertHashFoundInDbx() as the function name.
> 
> Either change is OK for me.
> 
> 2) Now we have to check 2 output: Status and IsFound in
> IsCertHashFoundInDatabase().
> 
> I am struggling to understand the different between 2 different ways of error
> handling:
> 
> ===========================
>     Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> *)Data, DataSize, &RevocationTime, &IsFound);
>     if (EFI_ERROR (Status) || IsFound) {
>       //
>       // Check the timestamp signature and signing time to determine if the image
> can be trusted.
>       //
>       IsForbidden = TRUE;
>       if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime)) {
>         IsForbidden = FALSE;
> ============================
> 
> and
> 
> ============================
>             VerifyStatus = FALSE;
>             //
>             // Here We still need to check if this RootCert's Hash is revoked
>             //
>             Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
>             if (EFI_ERROR (Status)) {
>               goto Done;
>             }
> 
>             if (!IsFound) {
>               VerifyStatus = TRUE;
>               goto Done;
>             }
> 
>             //
>             // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
>             //
>             VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
>             if (!VerifyStatus) {
> ===============================
> 
> I *believe* the logic behind is same. If so, we can use a consistent way to check
> the 2 output and decide if PassTimestampCheck() is required.
> 
> Or, can we create a one single function to perform such check for both
> IsCertHashFoundInDatabase() and PassTimestampCheck() ?
> 
> If I am wrong, there is *difference* between them. Then I think we need much
> better description to help reviewer to catch the difference.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Thursday, February 6, 2020 10:20 PM
> > To: devel@edk2.groups.io
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error
> > and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> >
> > 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>
> > ---
> >  .../DxeImageVerificationLib.c                 | 68 +++++++++++--------
> >  1 file changed, 41 insertions(+), 27 deletions(-)
> >
> > diff --git
> > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > index 8739d1fa29..a5dfee0f8e 100644
> > --- 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.
> >
> >    @param[in]  SignatureListSize Size of Signature List.
> >
> >    @param[out] RevocationTime    Return the time that the certificate was
> > revoked.
> >
> > +  @param[out] IsFound           Search result. Only valid if EFI_SUCCESS
> returned.
> >
> >
> >
> > -  @return TRUE   The certificate hash is found in the forbidden database.
> >
> > -  @return FALSE  The certificate hash is not found in the forbidden database.
> >
> > +  @retval EFI_SUCCESS           Finished the search without any error.
> >
> > +  @retval Others                Error occurred in the search of database.
> >
> >
> >
> >  **/
> >
> > -BOOLEAN
> >
> > +EFI_STATUS
> >
> >  IsCertHashFoundInDatabase (
> >
> >    IN  UINT8               *Certificate,
> >
> >    IN  UINTN               CertSize,
> >
> >    IN  EFI_SIGNATURE_LIST  *SignatureList,
> >
> >    IN  UINTN               SignatureListSize,
> >
> > -  OUT EFI_TIME            *RevocationTime
> >
> > +  OUT EFI_TIME            *RevocationTime,
> >
> > +  OUT BOOLEAN             *IsFound
> >
> >    )
> >
> >  {
> >
> > -  BOOLEAN             IsFound;
> >
> > -  BOOLEAN             Status;
> >
> > +  EFI_STATUS          Status;
> >
> >    EFI_SIGNATURE_LIST  *DbxList;
> >
> >    UINTN               DbxSize;
> >
> >    EFI_SIGNATURE_DATA  *CertHash;
> >
> > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> >    UINT8               *TBSCert;
> >
> >    UINTN               TBSCertSize;
> >
> >
> >
> > -  IsFound  = FALSE;
> >
> > +  Status   = EFI_ABORTED;
> >
> > +  *IsFound = FALSE;
> >
> >    DbxList  = SignatureList;
> >
> >    DbxSize  = SignatureListSize;
> >
> >    HashCtx  = NULL;
> >
> >    HashAlg  = HASHALG_MAX;
> >
> >
> >
> >    if ((RevocationTime == NULL) || (DbxList == NULL)) {
> >
> > -    return FALSE;
> >
> > +    return EFI_INVALID_PARAMETER;
> >
> >    }
> >
> >
> >
> >    //
> >
> >    // Retrieve the TBSCertificate from the X.509 Certificate.
> >
> >    //
> >
> >    if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> >
> > -    return FALSE;
> >
> > +    return Status;
> >
> >    }
> >
> >
> >
> >    while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
> >
> > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> >      if (HashCtx == NULL) {
> >
> >        goto Done;
> >
> >      }
> >
> > -    Status = mHash[HashAlg].HashInit (HashCtx);
> >
> > -    if (!Status) {
> >
> > +    if (!mHash[HashAlg].HashInit (HashCtx)) {
> >
> >        goto Done;
> >
> >      }
> >
> > -    Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> >
> > -    if (!Status) {
> >
> > +    if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> >
> >        goto Done;
> >
> >      }
> >
> > -    Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> >
> > -    if (!Status) {
> >
> > +    if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> >
> >        goto Done;
> >
> >      }
> >
> >
> >
> > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> >          //
> >
> >          // Hash of Certificate is found in forbidden database.
> >
> >          //
> >
> > -        IsFound = TRUE;
> >
> > +        Status   = EFI_SUCCESS;
> >
> > +        *IsFound = TRUE;
> >
> >
> >
> >          //
> >
> >          // Return the revocation time.
> >
> > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> >      DbxList  = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> > >SignatureListSize);
> >
> >    }
> >
> >
> >
> > +  Status = EFI_SUCCESS;
> >
> > +
> >
> >  Done:
> >
> >    if (HashCtx != NULL) {
> >
> >      FreePool (HashCtx);
> >
> >    }
> >
> >
> >
> > -  return IsFound;
> >
> > +  return Status;
> >
> >  }
> >
> >
> >
> >  /**
> >
> > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> >  {
> >
> >    EFI_STATUS                Status;
> >
> >    BOOLEAN                   IsForbidden;
> >
> > +  BOOLEAN                   IsFound;
> >
> >    UINT8                     *Data;
> >
> >    UINTN                     DataSize;
> >
> >    EFI_SIGNATURE_LIST        *CertList;
> >
> > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> >      //
> >
> >      CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> >
> >
> >
> > -    if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> *)Data,
> > DataSize, &RevocationTime)) {
> >
> > +    Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > *)Data, DataSize, &RevocationTime, &IsFound);
> >
> > +    if (EFI_ERROR (Status) || IsFound) {
> >
> >        //
> >
> >        // Check the timestamp signature and signing time to determine if the
> image
> > can be trusted.
> >
> >        //
> >
> >        IsForbidden = TRUE;
> >
> > -      if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
> >
> > +      if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime)) {
> >
> >          IsForbidden = FALSE;
> >
> >          //
> >
> >          // Pass DBT check. Continue to check other certs in image signer's cert list
> > against DBX, DBT
> >
> > @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> >  {
> >
> >    EFI_STATUS                Status;
> >
> >    BOOLEAN                   VerifyStatus;
> >
> > +  BOOLEAN                   IsFound;
> >
> >    EFI_SIGNATURE_LIST        *CertList;
> >
> >    EFI_SIGNATURE_DATA        *CertData;
> >
> >    UINTN                     DataSize;
> >
> > @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> >            // The image is signed and its signature is found in 'db'.
> >
> >            //
> >
> >            if (DbxData != NULL) {
> >
> > +            VerifyStatus = FALSE;
> >
> >              //
> >
> >              // Here We still need to check if this RootCert's Hash is revoked
> >
> >              //
> >
> > -            if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> >
> > -              //
> >
> > -              // Check the timestamp signature and signing time to determine if the
> > RootCert can be trusted.
> >
> > -              //
> >
> > -              VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> >
> > -              if (!VerifyStatus) {
> >
> > -                DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> and
> > signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> >
> > -              }
> >
> > +            Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
> >
> > +            if (EFI_ERROR (Status)) {
> >
> > +              goto Done;
> >
> > +            }
> >
> > +
> >
> > +            if (!IsFound) {
> >
> > +              VerifyStatus = TRUE;
> >
> > +              goto Done;
> >
> > +            }
> >
> > +
> >
> > +            //
> >
> > +            // Check the timestamp signature and signing time to determine if the
> > RootCert can be trusted.
> >
> > +            //
> >
> > +            VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> >
> > +            if (!VerifyStatus) {
> >
> > +              DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> and
> > signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> >
> >              }
> >
> >            }
> >
> >
> >
> > --
> > 2.24.0.windows.2


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

View/Reply Online (#54365): https://edk2.groups.io/g/devel/message/54365
Mute This Topic: https://groups.io/mt/71023424/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)

Posted by Yao, Jiewen 4 years, 9 months ago

1) I prefer we do a little bit simple clean up in this series. Just name change. Maybe as patch-10.

2) When PassTimestampCheck() need to be called? Only Dbx is found? Or even the Dbx is broken?

I prefer we need use a consistent rule.

Case 1 in original patch:
if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime)) {

Case 2 in your email:
>               VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
>               if (!VerifyStatus) {

It seems they are not consistent...

Thank you
Yao Jiewen


> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Thursday, February 13, 2020 11:08 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> 
> Jiewen,
> 
> Thanks for the comments.
> 
> 1) You're right. IsCertHashFoundInDatabase is quite general and cause
> confusions between
> db and dbx situation. Since it's not newly introduced in this patch series, do you
> think it's ok
> to fix it in separate patch series later? Or do you prefer fix it in this patch series?
> I'm ok with
> both.
> 
> 2) I checked both code again. I think you're right. Both callings are for dbx, any
> error Status
> should be taken as IsFound(==TRUE). What about following change for the
> second case?
> Please help double check if any logic hole here.
> 
>             Status = IsCertHashFoundInDatabase (...);
>             if (EFI_ERROR (Status) || IsFound) {
>               //
>               // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
>               //
>               VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
>               if (!VerifyStatus) {
>                 DEBUG ((...));
>               }
>             } else  {
>               VerifyStatus = TRUE;
>             }
> 
>              goto Done;
> 
> Regards,
> Jian
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Thursday, February 13, 2020 6:11 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> >
> > Comment below:
> >
> > 1) I think the function name - IsCertHashFoundInDatabase() and the
> > implementation {  DbxList  = SignatureList;   DbxSize  = SignatureListSize; } bring
> > some confusion to me.
> >
> > If this is a *generic* database search function, I recommend we use a generic
> > name - not use DbxList/DbxSize in the function implementation.
> >
> > If the input SignatureList of the function must be *Dbx*, I recommend we use
> > IsCertHashFoundInDbx() as the function name.
> >
> > Either change is OK for me.
> >
> > 2) Now we have to check 2 output: Status and IsFound in
> > IsCertHashFoundInDatabase().
> >
> > I am struggling to understand the different between 2 different ways of error
> > handling:
> >
> > ===========================
> >     Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > *)Data, DataSize, &RevocationTime, &IsFound);
> >     if (EFI_ERROR (Status) || IsFound) {
> >       //
> >       // Check the timestamp signature and signing time to determine if the
> image
> > can be trusted.
> >       //
> >       IsForbidden = TRUE;
> >       if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime)) {
> >         IsForbidden = FALSE;
> > ============================
> >
> > and
> >
> > ============================
> >             VerifyStatus = FALSE;
> >             //
> >             // Here We still need to check if this RootCert's Hash is revoked
> >             //
> >             Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
> >             if (EFI_ERROR (Status)) {
> >               goto Done;
> >             }
> >
> >             if (!IsFound) {
> >               VerifyStatus = TRUE;
> >               goto Done;
> >             }
> >
> >             //
> >             // Check the timestamp signature and signing time to determine if the
> > RootCert can be trusted.
> >             //
> >             VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> >             if (!VerifyStatus) {
> > ===============================
> >
> > I *believe* the logic behind is same. If so, we can use a consistent way to
> check
> > the 2 output and decide if PassTimestampCheck() is required.
> >
> > Or, can we create a one single function to perform such check for both
> > IsCertHashFoundInDatabase() and PassTimestampCheck() ?
> >
> > If I am wrong, there is *difference* between them. Then I think we need much
> > better description to help reviewer to catch the difference.
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > Sent: Thursday, February 6, 2020 10:20 PM
> > > To: devel@edk2.groups.io
> > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > > Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error
> > > and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > >
> > > 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>
> > > ---
> > >  .../DxeImageVerificationLib.c                 | 68 +++++++++++--------
> > >  1 file changed, 41 insertions(+), 27 deletions(-)
> > >
> > > diff --git
> > > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > index 8739d1fa29..a5dfee0f8e 100644
> > > ---
> 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.
> > >
> > >    @param[in]  SignatureListSize Size of Signature List.
> > >
> > >    @param[out] RevocationTime    Return the time that the certificate was
> > > revoked.
> > >
> > > +  @param[out] IsFound           Search result. Only valid if EFI_SUCCESS
> > returned.
> > >
> > >
> > >
> > > -  @return TRUE   The certificate hash is found in the forbidden database.
> > >
> > > -  @return FALSE  The certificate hash is not found in the forbidden database.
> > >
> > > +  @retval EFI_SUCCESS           Finished the search without any error.
> > >
> > > +  @retval Others                Error occurred in the search of database.
> > >
> > >
> > >
> > >  **/
> > >
> > > -BOOLEAN
> > >
> > > +EFI_STATUS
> > >
> > >  IsCertHashFoundInDatabase (
> > >
> > >    IN  UINT8               *Certificate,
> > >
> > >    IN  UINTN               CertSize,
> > >
> > >    IN  EFI_SIGNATURE_LIST  *SignatureList,
> > >
> > >    IN  UINTN               SignatureListSize,
> > >
> > > -  OUT EFI_TIME            *RevocationTime
> > >
> > > +  OUT EFI_TIME            *RevocationTime,
> > >
> > > +  OUT BOOLEAN             *IsFound
> > >
> > >    )
> > >
> > >  {
> > >
> > > -  BOOLEAN             IsFound;
> > >
> > > -  BOOLEAN             Status;
> > >
> > > +  EFI_STATUS          Status;
> > >
> > >    EFI_SIGNATURE_LIST  *DbxList;
> > >
> > >    UINTN               DbxSize;
> > >
> > >    EFI_SIGNATURE_DATA  *CertHash;
> > >
> > > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> > >    UINT8               *TBSCert;
> > >
> > >    UINTN               TBSCertSize;
> > >
> > >
> > >
> > > -  IsFound  = FALSE;
> > >
> > > +  Status   = EFI_ABORTED;
> > >
> > > +  *IsFound = FALSE;
> > >
> > >    DbxList  = SignatureList;
> > >
> > >    DbxSize  = SignatureListSize;
> > >
> > >    HashCtx  = NULL;
> > >
> > >    HashAlg  = HASHALG_MAX;
> > >
> > >
> > >
> > >    if ((RevocationTime == NULL) || (DbxList == NULL)) {
> > >
> > > -    return FALSE;
> > >
> > > +    return EFI_INVALID_PARAMETER;
> > >
> > >    }
> > >
> > >
> > >
> > >    //
> > >
> > >    // Retrieve the TBSCertificate from the X.509 Certificate.
> > >
> > >    //
> > >
> > >    if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> > >
> > > -    return FALSE;
> > >
> > > +    return Status;
> > >
> > >    }
> > >
> > >
> > >
> > >    while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
> > >
> > > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> > >      if (HashCtx == NULL) {
> > >
> > >        goto Done;
> > >
> > >      }
> > >
> > > -    Status = mHash[HashAlg].HashInit (HashCtx);
> > >
> > > -    if (!Status) {
> > >
> > > +    if (!mHash[HashAlg].HashInit (HashCtx)) {
> > >
> > >        goto Done;
> > >
> > >      }
> > >
> > > -    Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> > >
> > > -    if (!Status) {
> > >
> > > +    if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> > >
> > >        goto Done;
> > >
> > >      }
> > >
> > > -    Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> > >
> > > -    if (!Status) {
> > >
> > > +    if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> > >
> > >        goto Done;
> > >
> > >      }
> > >
> > >
> > >
> > > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> > >          //
> > >
> > >          // Hash of Certificate is found in forbidden database.
> > >
> > >          //
> > >
> > > -        IsFound = TRUE;
> > >
> > > +        Status   = EFI_SUCCESS;
> > >
> > > +        *IsFound = TRUE;
> > >
> > >
> > >
> > >          //
> > >
> > >          // Return the revocation time.
> > >
> > > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> > >      DbxList  = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> > > >SignatureListSize);
> > >
> > >    }
> > >
> > >
> > >
> > > +  Status = EFI_SUCCESS;
> > >
> > > +
> > >
> > >  Done:
> > >
> > >    if (HashCtx != NULL) {
> > >
> > >      FreePool (HashCtx);
> > >
> > >    }
> > >
> > >
> > >
> > > -  return IsFound;
> > >
> > > +  return Status;
> > >
> > >  }
> > >
> > >
> > >
> > >  /**
> > >
> > > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> > >  {
> > >
> > >    EFI_STATUS                Status;
> > >
> > >    BOOLEAN                   IsForbidden;
> > >
> > > +  BOOLEAN                   IsFound;
> > >
> > >    UINT8                     *Data;
> > >
> > >    UINTN                     DataSize;
> > >
> > >    EFI_SIGNATURE_LIST        *CertList;
> > >
> > > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> > >      //
> > >
> > >      CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> > >
> > >
> > >
> > > -    if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > *)Data,
> > > DataSize, &RevocationTime)) {
> > >
> > > +    Status = IsCertHashFoundInDatabase (Cert, CertSize,
> (EFI_SIGNATURE_LIST
> > > *)Data, DataSize, &RevocationTime, &IsFound);
> > >
> > > +    if (EFI_ERROR (Status) || IsFound) {
> > >
> > >        //
> > >
> > >        // Check the timestamp signature and signing time to determine if the
> > image
> > > can be trusted.
> > >
> > >        //
> > >
> > >        IsForbidden = TRUE;
> > >
> > > -      if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
> > >
> > > +      if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> AuthDataSize,
> > > &RevocationTime)) {
> > >
> > >          IsForbidden = FALSE;
> > >
> > >          //
> > >
> > >          // Pass DBT check. Continue to check other certs in image signer's cert
> list
> > > against DBX, DBT
> > >
> > > @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> > >  {
> > >
> > >    EFI_STATUS                Status;
> > >
> > >    BOOLEAN                   VerifyStatus;
> > >
> > > +  BOOLEAN                   IsFound;
> > >
> > >    EFI_SIGNATURE_LIST        *CertList;
> > >
> > >    EFI_SIGNATURE_DATA        *CertData;
> > >
> > >    UINTN                     DataSize;
> > >
> > > @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> > >            // The image is signed and its signature is found in 'db'.
> > >
> > >            //
> > >
> > >            if (DbxData != NULL) {
> > >
> > > +            VerifyStatus = FALSE;
> > >
> > >              //
> > >
> > >              // Here We still need to check if this RootCert's Hash is revoked
> > >
> > >              //
> > >
> > > -            if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> > >
> > > -              //
> > >
> > > -              // Check the timestamp signature and signing time to determine if
> the
> > > RootCert can be trusted.
> > >
> > > -              //
> > >
> > > -              VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > >
> > > -              if (!VerifyStatus) {
> > >
> > > -                DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> > and
> > > signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> > >
> > > -              }
> > >
> > > +            Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> &IsFound);
> > >
> > > +            if (EFI_ERROR (Status)) {
> > >
> > > +              goto Done;
> > >
> > > +            }
> > >
> > > +
> > >
> > > +            if (!IsFound) {
> > >
> > > +              VerifyStatus = TRUE;
> > >
> > > +              goto Done;
> > >
> > > +            }
> > >
> > > +
> > >
> > > +            //
> > >
> > > +            // Check the timestamp signature and signing time to determine if
> the
> > > RootCert can be trusted.
> > >
> > > +            //
> > >
> > > +            VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > >
> > > +            if (!VerifyStatus) {
> > >
> > > +              DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> > and
> > > signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> > >
> > >              }
> > >
> > >            }
> > >
> > >
> > >
> > > --
> > > 2.24.0.windows.2


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

View/Reply Online (#54398): https://edk2.groups.io/g/devel/message/54398
Mute This Topic: https://groups.io/mt/71023424/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)

Posted by Wang, Jian J 4 years, 9 months ago

Jiewen,

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Friday, February 14, 2020 8:54 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> 
> 1) I prefer we do a little bit simple clean up in this series. Just name change.
> Maybe as patch-10.
> 

Sure. I'll add it in v2.

> 2) When PassTimestampCheck() need to be called? Only Dbx is found? Or even
> the Dbx is broken?
> 
> I prefer we need use a consistent rule.
> 
> Case 1 in original patch:
> if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime)) {
> 
> Case 2 in your email:
> >               VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> >               if (!VerifyStatus) {
> 
> It seems they are not consistent...
> 

Just talked to Chao privately. He mentioned that RevocationTime might not
be valid if Status != EFI_SUCCESS. So we should only call PassTimestampCheck()
when Status == EFI_SUCCESS and IsFound == TRUE.

Here's my new proposal (for case 1, case 2 is similar).

    Status = IsCertHashFoundInDatabase (...);
    if (EFI_ERROR(Status)) {
      //
      // Error in searching dbx. Consider it as 'found'. RevocationTime might
      // not be valid in such situation.
      //
      IsForbidden = TRUE;
    } else if (IsFound) {
      //
      // Found Cert in dbx successfully. Check the timestamp signature and
      // signing time to determine if the image can be trusted.
      //
      if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
        IsForbidden = FALSE;
        //
        // Pass DBT check. Continue to check other certs in image signer's cert list against DBX, DBT
        //
        continue;
      } else {
        IsForbidden = TRUE;
        DEBUG((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature failed the timestamp check.\n"));
        goto Done;
      }
    }

If no objection, I'll include it in v2.

Regards,
Jian
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Thursday, February 13, 2020 11:08 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> >
> > Jiewen,
> >
> > Thanks for the comments.
> >
> > 1) You're right. IsCertHashFoundInDatabase is quite general and cause
> > confusions between
> > db and dbx situation. Since it's not newly introduced in this patch series, do you
> > think it's ok
> > to fix it in separate patch series later? Or do you prefer fix it in this patch series?
> > I'm ok with
> > both.
> >
> > 2) I checked both code again. I think you're right. Both callings are for dbx, any
> > error Status
> > should be taken as IsFound(==TRUE). What about following change for the
> > second case?
> > Please help double check if any logic hole here.
> >
> >             Status = IsCertHashFoundInDatabase (...);
> >             if (EFI_ERROR (Status) || IsFound) {
> >               //
> >               // Check the timestamp signature and signing time to determine if the
> > RootCert can be trusted.
> >               //
> >               VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> >               if (!VerifyStatus) {
> >                 DEBUG ((...));
> >               }
> >             } else  {
> >               VerifyStatus = TRUE;
> >             }
> >
> >              goto Done;
> >
> > Regards,
> > Jian
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen <jiewen.yao@intel.com>
> > > Sent: Thursday, February 13, 2020 6:11 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>
> > > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > >
> > > Comment below:
> > >
> > > 1) I think the function name - IsCertHashFoundInDatabase() and the
> > > implementation {  DbxList  = SignatureList;   DbxSize  = SignatureListSize; }
> bring
> > > some confusion to me.
> > >
> > > If this is a *generic* database search function, I recommend we use a
> generic
> > > name - not use DbxList/DbxSize in the function implementation.
> > >
> > > If the input SignatureList of the function must be *Dbx*, I recommend we
> use
> > > IsCertHashFoundInDbx() as the function name.
> > >
> > > Either change is OK for me.
> > >
> > > 2) Now we have to check 2 output: Status and IsFound in
> > > IsCertHashFoundInDatabase().
> > >
> > > I am struggling to understand the different between 2 different ways of error
> > > handling:
> > >
> > > ===========================
> > >     Status = IsCertHashFoundInDatabase (Cert, CertSize,
> (EFI_SIGNATURE_LIST
> > > *)Data, DataSize, &RevocationTime, &IsFound);
> > >     if (EFI_ERROR (Status) || IsFound) {
> > >       //
> > >       // Check the timestamp signature and signing time to determine if the
> > image
> > > can be trusted.
> > >       //
> > >       IsForbidden = TRUE;
> > >       if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> AuthDataSize,
> > > &RevocationTime)) {
> > >         IsForbidden = FALSE;
> > > ============================
> > >
> > > and
> > >
> > > ============================
> > >             VerifyStatus = FALSE;
> > >             //
> > >             // Here We still need to check if this RootCert's Hash is revoked
> > >             //
> > >             Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> &IsFound);
> > >             if (EFI_ERROR (Status)) {
> > >               goto Done;
> > >             }
> > >
> > >             if (!IsFound) {
> > >               VerifyStatus = TRUE;
> > >               goto Done;
> > >             }
> > >
> > >             //
> > >             // Check the timestamp signature and signing time to determine if the
> > > RootCert can be trusted.
> > >             //
> > >             VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > >             if (!VerifyStatus) {
> > > ===============================
> > >
> > > I *believe* the logic behind is same. If so, we can use a consistent way to
> > check
> > > the 2 output and decide if PassTimestampCheck() is required.
> > >
> > > Or, can we create a one single function to perform such check for both
> > > IsCertHashFoundInDatabase() and PassTimestampCheck() ?
> > >
> > > If I am wrong, there is *difference* between them. Then I think we need
> much
> > > better description to help reviewer to catch the difference.
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > > Sent: Thursday, February 6, 2020 10:20 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> > > > <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > > > Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > error
> > > > and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > > >
> > > > 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>
> > > > ---
> > > >  .../DxeImageVerificationLib.c                 | 68 +++++++++++--------
> > > >  1 file changed, 41 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git
> > > > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > index 8739d1fa29..a5dfee0f8e 100644
> > > > ---
> > 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.
> > > >
> > > >    @param[in]  SignatureListSize Size of Signature List.
> > > >
> > > >    @param[out] RevocationTime    Return the time that the certificate was
> > > > revoked.
> > > >
> > > > +  @param[out] IsFound           Search result. Only valid if EFI_SUCCESS
> > > returned.
> > > >
> > > >
> > > >
> > > > -  @return TRUE   The certificate hash is found in the forbidden database.
> > > >
> > > > -  @return FALSE  The certificate hash is not found in the forbidden
> database.
> > > >
> > > > +  @retval EFI_SUCCESS           Finished the search without any error.
> > > >
> > > > +  @retval Others                Error occurred in the search of database.
> > > >
> > > >
> > > >
> > > >  **/
> > > >
> > > > -BOOLEAN
> > > >
> > > > +EFI_STATUS
> > > >
> > > >  IsCertHashFoundInDatabase (
> > > >
> > > >    IN  UINT8               *Certificate,
> > > >
> > > >    IN  UINTN               CertSize,
> > > >
> > > >    IN  EFI_SIGNATURE_LIST  *SignatureList,
> > > >
> > > >    IN  UINTN               SignatureListSize,
> > > >
> > > > -  OUT EFI_TIME            *RevocationTime
> > > >
> > > > +  OUT EFI_TIME            *RevocationTime,
> > > >
> > > > +  OUT BOOLEAN             *IsFound
> > > >
> > > >    )
> > > >
> > > >  {
> > > >
> > > > -  BOOLEAN             IsFound;
> > > >
> > > > -  BOOLEAN             Status;
> > > >
> > > > +  EFI_STATUS          Status;
> > > >
> > > >    EFI_SIGNATURE_LIST  *DbxList;
> > > >
> > > >    UINTN               DbxSize;
> > > >
> > > >    EFI_SIGNATURE_DATA  *CertHash;
> > > >
> > > > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> > > >    UINT8               *TBSCert;
> > > >
> > > >    UINTN               TBSCertSize;
> > > >
> > > >
> > > >
> > > > -  IsFound  = FALSE;
> > > >
> > > > +  Status   = EFI_ABORTED;
> > > >
> > > > +  *IsFound = FALSE;
> > > >
> > > >    DbxList  = SignatureList;
> > > >
> > > >    DbxSize  = SignatureListSize;
> > > >
> > > >    HashCtx  = NULL;
> > > >
> > > >    HashAlg  = HASHALG_MAX;
> > > >
> > > >
> > > >
> > > >    if ((RevocationTime == NULL) || (DbxList == NULL)) {
> > > >
> > > > -    return FALSE;
> > > >
> > > > +    return EFI_INVALID_PARAMETER;
> > > >
> > > >    }
> > > >
> > > >
> > > >
> > > >    //
> > > >
> > > >    // Retrieve the TBSCertificate from the X.509 Certificate.
> > > >
> > > >    //
> > > >
> > > >    if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> > > >
> > > > -    return FALSE;
> > > >
> > > > +    return Status;
> > > >
> > > >    }
> > > >
> > > >
> > > >
> > > >    while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
> > > >
> > > > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> > > >      if (HashCtx == NULL) {
> > > >
> > > >        goto Done;
> > > >
> > > >      }
> > > >
> > > > -    Status = mHash[HashAlg].HashInit (HashCtx);
> > > >
> > > > -    if (!Status) {
> > > >
> > > > +    if (!mHash[HashAlg].HashInit (HashCtx)) {
> > > >
> > > >        goto Done;
> > > >
> > > >      }
> > > >
> > > > -    Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> > > >
> > > > -    if (!Status) {
> > > >
> > > > +    if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> > > >
> > > >        goto Done;
> > > >
> > > >      }
> > > >
> > > > -    Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> > > >
> > > > -    if (!Status) {
> > > >
> > > > +    if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> > > >
> > > >        goto Done;
> > > >
> > > >      }
> > > >
> > > >
> > > >
> > > > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> > > >          //
> > > >
> > > >          // Hash of Certificate is found in forbidden database.
> > > >
> > > >          //
> > > >
> > > > -        IsFound = TRUE;
> > > >
> > > > +        Status   = EFI_SUCCESS;
> > > >
> > > > +        *IsFound = TRUE;
> > > >
> > > >
> > > >
> > > >          //
> > > >
> > > >          // Return the revocation time.
> > > >
> > > > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> > > >      DbxList  = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> > > > >SignatureListSize);
> > > >
> > > >    }
> > > >
> > > >
> > > >
> > > > +  Status = EFI_SUCCESS;
> > > >
> > > > +
> > > >
> > > >  Done:
> > > >
> > > >    if (HashCtx != NULL) {
> > > >
> > > >      FreePool (HashCtx);
> > > >
> > > >    }
> > > >
> > > >
> > > >
> > > > -  return IsFound;
> > > >
> > > > +  return Status;
> > > >
> > > >  }
> > > >
> > > >
> > > >
> > > >  /**
> > > >
> > > > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> > > >  {
> > > >
> > > >    EFI_STATUS                Status;
> > > >
> > > >    BOOLEAN                   IsForbidden;
> > > >
> > > > +  BOOLEAN                   IsFound;
> > > >
> > > >    UINT8                     *Data;
> > > >
> > > >    UINTN                     DataSize;
> > > >
> > > >    EFI_SIGNATURE_LIST        *CertList;
> > > >
> > > > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> > > >      //
> > > >
> > > >      CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> > > >
> > > >
> > > >
> > > > -    if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > > *)Data,
> > > > DataSize, &RevocationTime)) {
> > > >
> > > > +    Status = IsCertHashFoundInDatabase (Cert, CertSize,
> > (EFI_SIGNATURE_LIST
> > > > *)Data, DataSize, &RevocationTime, &IsFound);
> > > >
> > > > +    if (EFI_ERROR (Status) || IsFound) {
> > > >
> > > >        //
> > > >
> > > >        // Check the timestamp signature and signing time to determine if the
> > > image
> > > > can be trusted.
> > > >
> > > >        //
> > > >
> > > >        IsForbidden = TRUE;
> > > >
> > > > -      if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime))
> {
> > > >
> > > > +      if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> > AuthDataSize,
> > > > &RevocationTime)) {
> > > >
> > > >          IsForbidden = FALSE;
> > > >
> > > >          //
> > > >
> > > >          // Pass DBT check. Continue to check other certs in image signer's cert
> > list
> > > > against DBX, DBT
> > > >
> > > > @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> > > >  {
> > > >
> > > >    EFI_STATUS                Status;
> > > >
> > > >    BOOLEAN                   VerifyStatus;
> > > >
> > > > +  BOOLEAN                   IsFound;
> > > >
> > > >    EFI_SIGNATURE_LIST        *CertList;
> > > >
> > > >    EFI_SIGNATURE_DATA        *CertData;
> > > >
> > > >    UINTN                     DataSize;
> > > >
> > > > @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> > > >            // The image is signed and its signature is found in 'db'.
> > > >
> > > >            //
> > > >
> > > >            if (DbxData != NULL) {
> > > >
> > > > +            VerifyStatus = FALSE;
> > > >
> > > >              //
> > > >
> > > >              // Here We still need to check if this RootCert's Hash is revoked
> > > >
> > > >              //
> > > >
> > > > -            if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> > > >
> > > > -              //
> > > >
> > > > -              // Check the timestamp signature and signing time to determine if
> > the
> > > > RootCert can be trusted.
> > > >
> > > > -              //
> > > >
> > > > -              VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > &RevocationTime);
> > > >
> > > > -              if (!VerifyStatus) {
> > > >
> > > > -                DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> signed
> > > and
> > > > signature is accepted by DB, but its root cert failed the timestamp
> check.\n"));
> > > >
> > > > -              }
> > > >
> > > > +            Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> > &IsFound);
> > > >
> > > > +            if (EFI_ERROR (Status)) {
> > > >
> > > > +              goto Done;
> > > >
> > > > +            }
> > > >
> > > > +
> > > >
> > > > +            if (!IsFound) {
> > > >
> > > > +              VerifyStatus = TRUE;
> > > >
> > > > +              goto Done;
> > > >
> > > > +            }
> > > >
> > > > +
> > > >
> > > > +            //
> > > >
> > > > +            // Check the timestamp signature and signing time to determine if
> > the
> > > > RootCert can be trusted.
> > > >
> > > > +            //
> > > >
> > > > +            VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > &RevocationTime);
> > > >
> > > > +            if (!VerifyStatus) {
> > > >
> > > > +              DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> > > and
> > > > signature is accepted by DB, but its root cert failed the timestamp
> check.\n"));
> > > >
> > > >              }
> > > >
> > > >            }
> > > >
> > > >
> > > >
> > > > --
> > > > 2.24.0.windows.2


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

View/Reply Online (#54404): https://edk2.groups.io/g/devel/message/54404
Mute This Topic: https://groups.io/mt/71023424/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)

Posted by Yao, Jiewen 4 years, 9 months ago

Fine. Thanks for the update.

> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Friday, February 14, 2020 11:32 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> 
> Jiewen,
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Friday, February 14, 2020 8:54 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> >
> > 1) I prefer we do a little bit simple clean up in this series. Just name change.
> > Maybe as patch-10.
> >
> 
> Sure. I'll add it in v2.
> 
> > 2) When PassTimestampCheck() need to be called? Only Dbx is found? Or even
> > the Dbx is broken?
> >
> > I prefer we need use a consistent rule.
> >
> > Case 1 in original patch:
> > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > > > &RevocationTime)) {
> >
> > Case 2 in your email:
> > >               VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > >               if (!VerifyStatus) {
> >
> > It seems they are not consistent...
> >
> 
> Just talked to Chao privately. He mentioned that RevocationTime might not
> be valid if Status != EFI_SUCCESS. So we should only call PassTimestampCheck()
> when Status == EFI_SUCCESS and IsFound == TRUE.
> 
> Here's my new proposal (for case 1, case 2 is similar).
> 
>     Status = IsCertHashFoundInDatabase (...);
>     if (EFI_ERROR(Status)) {
>       //
>       // Error in searching dbx. Consider it as 'found'. RevocationTime might
>       // not be valid in such situation.
>       //
>       IsForbidden = TRUE;
>     } else if (IsFound) {
>       //
>       // Found Cert in dbx successfully. Check the timestamp signature and
>       // signing time to determine if the image can be trusted.
>       //
>       if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
>         IsForbidden = FALSE;
>         //
>         // Pass DBT check. Continue to check other certs in image signer's cert list
> against DBX, DBT
>         //
>         continue;
>       } else {
>         IsForbidden = TRUE;
>         DEBUG((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but
> signature failed the timestamp check.\n"));
>         goto Done;
>       }
>     }
> 
> If no objection, I'll include it in v2.
> 
> Regards,
> Jian
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > Sent: Thursday, February 13, 2020 11:08 PM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>
> > > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > >
> > > Jiewen,
> > >
> > > Thanks for the comments.
> > >
> > > 1) You're right. IsCertHashFoundInDatabase is quite general and cause
> > > confusions between
> > > db and dbx situation. Since it's not newly introduced in this patch series, do
> you
> > > think it's ok
> > > to fix it in separate patch series later? Or do you prefer fix it in this patch
> series?
> > > I'm ok with
> > > both.
> > >
> > > 2) I checked both code again. I think you're right. Both callings are for dbx,
> any
> > > error Status
> > > should be taken as IsFound(==TRUE). What about following change for the
> > > second case?
> > > Please help double check if any logic hole here.
> > >
> > >             Status = IsCertHashFoundInDatabase (...);
> > >             if (EFI_ERROR (Status) || IsFound) {
> > >               //
> > >               // Check the timestamp signature and signing time to determine if
> the
> > > RootCert can be trusted.
> > >               //
> > >               VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > >               if (!VerifyStatus) {
> > >                 DEBUG ((...));
> > >               }
> > >             } else  {
> > >               VerifyStatus = TRUE;
> > >             }
> > >
> > >              goto Done;
> > >
> > > Regards,
> > > Jian
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen <jiewen.yao@intel.com>
> > > > Sent: Thursday, February 13, 2020 6:11 PM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > > > <lersek@redhat.com>
> > > > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib:
> Differentiate
> > > > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > > >
> > > > Comment below:
> > > >
> > > > 1) I think the function name - IsCertHashFoundInDatabase() and the
> > > > implementation {  DbxList  = SignatureList;   DbxSize  = SignatureListSize; }
> > bring
> > > > some confusion to me.
> > > >
> > > > If this is a *generic* database search function, I recommend we use a
> > generic
> > > > name - not use DbxList/DbxSize in the function implementation.
> > > >
> > > > If the input SignatureList of the function must be *Dbx*, I recommend we
> > use
> > > > IsCertHashFoundInDbx() as the function name.
> > > >
> > > > Either change is OK for me.
> > > >
> > > > 2) Now we have to check 2 output: Status and IsFound in
> > > > IsCertHashFoundInDatabase().
> > > >
> > > > I am struggling to understand the different between 2 different ways of
> error
> > > > handling:
> > > >
> > > > ===========================
> > > >     Status = IsCertHashFoundInDatabase (Cert, CertSize,
> > (EFI_SIGNATURE_LIST
> > > > *)Data, DataSize, &RevocationTime, &IsFound);
> > > >     if (EFI_ERROR (Status) || IsFound) {
> > > >       //
> > > >       // Check the timestamp signature and signing time to determine if the
> > > image
> > > > can be trusted.
> > > >       //
> > > >       IsForbidden = TRUE;
> > > >       if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> > AuthDataSize,
> > > > &RevocationTime)) {
> > > >         IsForbidden = FALSE;
> > > > ============================
> > > >
> > > > and
> > > >
> > > > ============================
> > > >             VerifyStatus = FALSE;
> > > >             //
> > > >             // Here We still need to check if this RootCert's Hash is revoked
> > > >             //
> > > >             Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> > &IsFound);
> > > >             if (EFI_ERROR (Status)) {
> > > >               goto Done;
> > > >             }
> > > >
> > > >             if (!IsFound) {
> > > >               VerifyStatus = TRUE;
> > > >               goto Done;
> > > >             }
> > > >
> > > >             //
> > > >             // Check the timestamp signature and signing time to determine if
> the
> > > > RootCert can be trusted.
> > > >             //
> > > >             VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > &RevocationTime);
> > > >             if (!VerifyStatus) {
> > > > ===============================
> > > >
> > > > I *believe* the logic behind is same. If so, we can use a consistent way to
> > > check
> > > > the 2 output and decide if PassTimestampCheck() is required.
> > > >
> > > > Or, can we create a one single function to perform such check for both
> > > > IsCertHashFoundInDatabase() and PassTimestampCheck() ?
> > > >
> > > > If I am wrong, there is *difference* between them. Then I think we need
> > much
> > > > better description to help reviewer to catch the difference.
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > > > Sent: Thursday, February 6, 2020 10:20 PM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> > > > > <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > > > > Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > > error
> > > > > and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > > > >
> > > > > 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>
> > > > > ---
> > > > >  .../DxeImageVerificationLib.c                 | 68 +++++++++++--------
> > > > >  1 file changed, 41 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git
> > > > >
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > >
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > > index 8739d1fa29..a5dfee0f8e 100644
> > > > > ---
> > > 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.
> > > > >
> > > > >    @param[in]  SignatureListSize Size of Signature List.
> > > > >
> > > > >    @param[out] RevocationTime    Return the time that the certificate
> was
> > > > > revoked.
> > > > >
> > > > > +  @param[out] IsFound           Search result. Only valid if EFI_SUCCESS
> > > > returned.
> > > > >
> > > > >
> > > > >
> > > > > -  @return TRUE   The certificate hash is found in the forbidden database.
> > > > >
> > > > > -  @return FALSE  The certificate hash is not found in the forbidden
> > database.
> > > > >
> > > > > +  @retval EFI_SUCCESS           Finished the search without any error.
> > > > >
> > > > > +  @retval Others                Error occurred in the search of database.
> > > > >
> > > > >
> > > > >
> > > > >  **/
> > > > >
> > > > > -BOOLEAN
> > > > >
> > > > > +EFI_STATUS
> > > > >
> > > > >  IsCertHashFoundInDatabase (
> > > > >
> > > > >    IN  UINT8               *Certificate,
> > > > >
> > > > >    IN  UINTN               CertSize,
> > > > >
> > > > >    IN  EFI_SIGNATURE_LIST  *SignatureList,
> > > > >
> > > > >    IN  UINTN               SignatureListSize,
> > > > >
> > > > > -  OUT EFI_TIME            *RevocationTime
> > > > >
> > > > > +  OUT EFI_TIME            *RevocationTime,
> > > > >
> > > > > +  OUT BOOLEAN             *IsFound
> > > > >
> > > > >    )
> > > > >
> > > > >  {
> > > > >
> > > > > -  BOOLEAN             IsFound;
> > > > >
> > > > > -  BOOLEAN             Status;
> > > > >
> > > > > +  EFI_STATUS          Status;
> > > > >
> > > > >    EFI_SIGNATURE_LIST  *DbxList;
> > > > >
> > > > >    UINTN               DbxSize;
> > > > >
> > > > >    EFI_SIGNATURE_DATA  *CertHash;
> > > > >
> > > > > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> > > > >    UINT8               *TBSCert;
> > > > >
> > > > >    UINTN               TBSCertSize;
> > > > >
> > > > >
> > > > >
> > > > > -  IsFound  = FALSE;
> > > > >
> > > > > +  Status   = EFI_ABORTED;
> > > > >
> > > > > +  *IsFound = FALSE;
> > > > >
> > > > >    DbxList  = SignatureList;
> > > > >
> > > > >    DbxSize  = SignatureListSize;
> > > > >
> > > > >    HashCtx  = NULL;
> > > > >
> > > > >    HashAlg  = HASHALG_MAX;
> > > > >
> > > > >
> > > > >
> > > > >    if ((RevocationTime == NULL) || (DbxList == NULL)) {
> > > > >
> > > > > -    return FALSE;
> > > > >
> > > > > +    return EFI_INVALID_PARAMETER;
> > > > >
> > > > >    }
> > > > >
> > > > >
> > > > >
> > > > >    //
> > > > >
> > > > >    // Retrieve the TBSCertificate from the X.509 Certificate.
> > > > >
> > > > >    //
> > > > >
> > > > >    if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> > > > >
> > > > > -    return FALSE;
> > > > >
> > > > > +    return Status;
> > > > >
> > > > >    }
> > > > >
> > > > >
> > > > >
> > > > >    while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize))
> {
> > > > >
> > > > > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> > > > >      if (HashCtx == NULL) {
> > > > >
> > > > >        goto Done;
> > > > >
> > > > >      }
> > > > >
> > > > > -    Status = mHash[HashAlg].HashInit (HashCtx);
> > > > >
> > > > > -    if (!Status) {
> > > > >
> > > > > +    if (!mHash[HashAlg].HashInit (HashCtx)) {
> > > > >
> > > > >        goto Done;
> > > > >
> > > > >      }
> > > > >
> > > > > -    Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> > > > >
> > > > > -    if (!Status) {
> > > > >
> > > > > +    if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> > > > >
> > > > >        goto Done;
> > > > >
> > > > >      }
> > > > >
> > > > > -    Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> > > > >
> > > > > -    if (!Status) {
> > > > >
> > > > > +    if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> > > > >
> > > > >        goto Done;
> > > > >
> > > > >      }
> > > > >
> > > > >
> > > > >
> > > > > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> > > > >          //
> > > > >
> > > > >          // Hash of Certificate is found in forbidden database.
> > > > >
> > > > >          //
> > > > >
> > > > > -        IsFound = TRUE;
> > > > >
> > > > > +        Status   = EFI_SUCCESS;
> > > > >
> > > > > +        *IsFound = TRUE;
> > > > >
> > > > >
> > > > >
> > > > >          //
> > > > >
> > > > >          // Return the revocation time.
> > > > >
> > > > > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> > > > >      DbxList  = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> > > > > >SignatureListSize);
> > > > >
> > > > >    }
> > > > >
> > > > >
> > > > >
> > > > > +  Status = EFI_SUCCESS;
> > > > >
> > > > > +
> > > > >
> > > > >  Done:
> > > > >
> > > > >    if (HashCtx != NULL) {
> > > > >
> > > > >      FreePool (HashCtx);
> > > > >
> > > > >    }
> > > > >
> > > > >
> > > > >
> > > > > -  return IsFound;
> > > > >
> > > > > +  return Status;
> > > > >
> > > > >  }
> > > > >
> > > > >
> > > > >
> > > > >  /**
> > > > >
> > > > > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> > > > >  {
> > > > >
> > > > >    EFI_STATUS                Status;
> > > > >
> > > > >    BOOLEAN                   IsForbidden;
> > > > >
> > > > > +  BOOLEAN                   IsFound;
> > > > >
> > > > >    UINT8                     *Data;
> > > > >
> > > > >    UINTN                     DataSize;
> > > > >
> > > > >    EFI_SIGNATURE_LIST        *CertList;
> > > > >
> > > > > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> > > > >      //
> > > > >
> > > > >      CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> > > > >
> > > > >
> > > > >
> > > > > -    if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > > > *)Data,
> > > > > DataSize, &RevocationTime)) {
> > > > >
> > > > > +    Status = IsCertHashFoundInDatabase (Cert, CertSize,
> > > (EFI_SIGNATURE_LIST
> > > > > *)Data, DataSize, &RevocationTime, &IsFound);
> > > > >
> > > > > +    if (EFI_ERROR (Status) || IsFound) {
> > > > >
> > > > >        //
> > > > >
> > > > >        // Check the timestamp signature and signing time to determine if the
> > > > image
> > > > > can be trusted.
> > > > >
> > > > >        //
> > > > >
> > > > >        IsForbidden = TRUE;
> > > > >
> > > > > -      if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime))
> > {
> > > > >
> > > > > +      if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> > > AuthDataSize,
> > > > > &RevocationTime)) {
> > > > >
> > > > >          IsForbidden = FALSE;
> > > > >
> > > > >          //
> > > > >
> > > > >          // Pass DBT check. Continue to check other certs in image signer's
> cert
> > > list
> > > > > against DBX, DBT
> > > > >
> > > > > @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> > > > >  {
> > > > >
> > > > >    EFI_STATUS                Status;
> > > > >
> > > > >    BOOLEAN                   VerifyStatus;
> > > > >
> > > > > +  BOOLEAN                   IsFound;
> > > > >
> > > > >    EFI_SIGNATURE_LIST        *CertList;
> > > > >
> > > > >    EFI_SIGNATURE_DATA        *CertData;
> > > > >
> > > > >    UINTN                     DataSize;
> > > > >
> > > > > @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> > > > >            // The image is signed and its signature is found in 'db'.
> > > > >
> > > > >            //
> > > > >
> > > > >            if (DbxData != NULL) {
> > > > >
> > > > > +            VerifyStatus = FALSE;
> > > > >
> > > > >              //
> > > > >
> > > > >              // Here We still need to check if this RootCert's Hash is revoked
> > > > >
> > > > >              //
> > > > >
> > > > > -            if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> > > > >
> > > > > -              //
> > > > >
> > > > > -              // Check the timestamp signature and signing time to determine
> if
> > > the
> > > > > RootCert can be trusted.
> > > > >
> > > > > -              //
> > > > >
> > > > > -              VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > > &RevocationTime);
> > > > >
> > > > > -              if (!VerifyStatus) {
> > > > >
> > > > > -                DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> > signed
> > > > and
> > > > > signature is accepted by DB, but its root cert failed the timestamp
> > check.\n"));
> > > > >
> > > > > -              }
> > > > >
> > > > > +            Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> > > &IsFound);
> > > > >
> > > > > +            if (EFI_ERROR (Status)) {
> > > > >
> > > > > +              goto Done;
> > > > >
> > > > > +            }
> > > > >
> > > > > +
> > > > >
> > > > > +            if (!IsFound) {
> > > > >
> > > > > +              VerifyStatus = TRUE;
> > > > >
> > > > > +              goto Done;
> > > > >
> > > > > +            }
> > > > >
> > > > > +
> > > > >
> > > > > +            //
> > > > >
> > > > > +            // Check the timestamp signature and signing time to determine
> if
> > > the
> > > > > RootCert can be trusted.
> > > > >
> > > > > +            //
> > > > >
> > > > > +            VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > > &RevocationTime);
> > > > >
> > > > > +            if (!VerifyStatus) {
> > > > >
> > > > > +              DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> signed
> > > > and
> > > > > signature is accepted by DB, but its root cert failed the timestamp
> > check.\n"));
> > > > >
> > > > >              }
> > > > >
> > > > >            }
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > 2.24.0.windows.2


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

View/Reply Online (#54405): https://edk2.groups.io/g/devel/message/54405
Mute This Topic: https://groups.io/mt/71023424/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-