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

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

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