Jiewen, > -----Original Message----- > From: Yao, Jiewen <jiewen....@intel.com> > Sent: Friday, February 14, 2020 8:54 AM > To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io > Cc: Zhang, Chao B <chao.b.zh...@intel.com>; Laszlo Ersek > <ler...@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.w...@intel.com> > > Sent: Thursday, February 13, 2020 11:08 PM > > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io > > Cc: Zhang, Chao B <chao.b.zh...@intel.com>; Laszlo Ersek > > <ler...@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....@intel.com> > > > Sent: Thursday, February 13, 2020 6:11 PM > > > To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io > > > Cc: Zhang, Chao B <chao.b.zh...@intel.com>; Laszlo Ersek > > > <ler...@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.w...@intel.com> > > > > Sent: Thursday, February 6, 2020 10:20 PM > > > > To: devel@edk2.groups.io > > > > Cc: Yao, Jiewen <jiewen....@intel.com>; Zhang, Chao B > > > > <chao.b.zh...@intel.com>; Laszlo Ersek <ler...@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....@intel.com> > > > > Cc: Chao Zhang <chao.b.zh...@intel.com> > > > > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > > > > Signed-off-by: Laszlo Ersek <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-