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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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