Reviewed-by: Jiewen Yao <jiewen....@intel.com> > -----Original Message----- > From: Wang, Jian J <jian.j.w...@intel.com> > Sent: Friday, February 14, 2020 3:28 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 v2 06/10] SecurityPkg/DxeImageVerificationLib: Differentiate > error/search result (1)(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 | 64 ++++++++++++------- > 1 file changed, 42 insertions(+), 22 deletions(-) > > diff --git > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index 8739d1fa29..85261ba7f2 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,20 +1347,29 @@ 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)) { > > // > > - // Check the timestamp signature and signing time to determine if the > image > can be trusted. > > + // 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; > > } > > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but > signature failed the timestamp check.\n")); > > - goto Done; > > } > > > > } > > @@ -1392,6 +1404,7 @@ IsAllowedByDb ( > { > > EFI_STATUS Status; > > BOOLEAN VerifyStatus; > > + BOOLEAN IsFound; > > EFI_SIGNATURE_LIST *CertList; > > EFI_SIGNATURE_DATA *CertData; > > UINTN DataSize; > > @@ -1498,7 +1511,14 @@ IsAllowedByDb ( > // > > // Here We still need to check if this RootCert's Hash is revoked > > // > > - if (IsCertHashFoundInDatabase (RootCert, RootCertSize, > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) { > > + Status = IsCertHashFoundInDatabase (RootCert, RootCertSize, > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound); > > + if (EFI_ERROR (Status)) { > > + // > > + // Error in searching dbx. Consider it as 'found'. > RevocationTime might > > + // not be valid in such situation. > > + // > > + VerifyStatus = FALSE; > > + } else if (IsFound) { > > // > > // Check the timestamp signature and signing time to determine > if the > RootCert can be trusted. > > // > > -- > 2.24.0.windows.2
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54428): https://edk2.groups.io/g/devel/message/54428 Mute This Topic: https://groups.io/mt/71264904/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
