Fine. Thanks for the update.
> -----Original Message-----
> From: Wang, Jian J <jian.j.w...@intel.com>
> Sent: Friday, February 14, 2020 11:32 AM
> 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,
>
> > -----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 (#54405): https://edk2.groups.io/g/devel/message/54405
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]
-=-=-=-=-=-=-=-=-=-=-=-
