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

["Wang, Jian J" <jian.j.wang@intel.com>]

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