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.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 (#54398): https://edk2.groups.io/g/devel/message/54398
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]
-=-=-=-=-=-=-=-=-=-=-=-
