From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web09.7423.1581606455625595026 for ; Thu, 13 Feb 2020 07:07:35 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: jian.j.wang@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2020 07:07:35 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,437,1574150400"; d="scan'208";a="313750118" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga001.jf.intel.com with ESMTP; 13 Feb 2020 07:07:34 -0800 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 07:07:34 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 07:07:34 -0800 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.46]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.98]) with mapi id 14.03.0439.000; Thu, 13 Feb 2020 23:07:32 +0800 From: "Wang, Jian J" To: "Yao, Jiewen" , "devel@edk2.groups.io" CC: "Zhang, Chao B" , Laszlo Ersek Subject: Re: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575) Thread-Topic: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575) Thread-Index: AQHV3Ph7rsdlqfMv1UaUc05X+oTmrqgY64tAgABSGWA= Date: Thu, 13 Feb 2020 15:07:31 +0000 Message-ID: References: <20200206141933.356-1-jian.j.wang@intel.com> <20200206141933.356-7-jian.j.wang@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503F92CC3F@shsmsx102.ccr.corp.intel.com> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503F92CC3F@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTEwOTZlMzktOGNhYi00M2MxLTkzZDItNWYxNDkyZDY1ZmQ1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiaFNPR2tpZ2pVQlp0emZcL3Y0V0ZFMVRhSzlsdkhUaUViTkhJXC9zVStoSWpYaUx5RHVHZDlXNGcxMFZaVHE0dnFWIn0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jian.j.wang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Jiewen, Thanks for the comments. 1) You're right. IsCertHashFoundInDatabase is quite general and cause confu= sions 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 p= atch series? I'm ok with both. 2) I checked both code again. I think you're right. Both callings are for d= bx, any error Status should be taken as IsFound(=3D=3DTRUE). What about following change for the= second case? Please help double check if any logic hole here. Status =3D IsCertHashFoundInDatabase (...); if (EFI_ERROR (Status) || IsFound) { // // Check the timestamp signature and signing time to determin= e if the RootCert can be trusted. // VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSize, = &RevocationTime); if (!VerifyStatus) { DEBUG ((...)); } } else { VerifyStatus =3D TRUE; } goto Done; Regards, Jian > -----Original Message----- > From: Yao, Jiewen > Sent: Thursday, February 13, 2020 6:11 PM > To: Wang, Jian J ; devel@edk2.groups.io > Cc: Zhang, Chao B ; Laszlo Ersek > > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentia= te > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575) >=20 > Comment below: >=20 > 1) I think the function name - IsCertHashFoundInDatabase() and the > implementation { DbxList =3D SignatureList; DbxSize =3D SignatureLis= tSize; } bring > some confusion to me. >=20 > If this is a *generic* database search function, I recommend we use a gen= eric > name - not use DbxList/DbxSize in the function implementation. >=20 > If the input SignatureList of the function must be *Dbx*, I recommend we = use > IsCertHashFoundInDbx() as the function name. >=20 > Either change is OK for me. >=20 > 2) Now we have to check 2 output: Status and IsFound in > IsCertHashFoundInDatabase(). >=20 > I am struggling to understand the different between 2 different ways of e= rror > handling: >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > Status =3D 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 t= he image > can be trusted. > // > IsForbidden =3D TRUE; > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSi= ze, > &RevocationTime)) { > IsForbidden =3D FALSE; > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D >=20 > and >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > VerifyStatus =3D FALSE; > // > // Here We still need to check if this RootCert's Hash is rev= oked > // > Status =3D IsCertHashFoundInDatabase (RootCert, RootCertSize, > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound); > if (EFI_ERROR (Status)) { > goto Done; > } >=20 > if (!IsFound) { > VerifyStatus =3D TRUE; > goto Done; > } >=20 > // > // Check the timestamp signature and signing time to determin= e if the > RootCert can be trusted. > // > VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSize, > &RevocationTime); > if (!VerifyStatus) { > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D >=20 > 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. >=20 > Or, can we create a one single function to perform such check for both > IsCertHashFoundInDatabase() and PassTimestampCheck() ? >=20 > If I am wrong, there is *difference* between them. Then I think we need m= uch > better description to help reviewer to catch the difference. >=20 > Thank you > Yao Jiewen >=20 >=20 > > -----Original Message----- > > From: Wang, Jian J > > Sent: Thursday, February 6, 2020 10:20 PM > > To: devel@edk2.groups.io > > Cc: Yao, Jiewen ; Zhang, Chao B > > ; Laszlo Ersek > > 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=3D1608 > > > > 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 > > Cc: Chao Zhang > > Signed-off-by: Jian J Wang > > Signed-off-by: Laszlo Ersek > > --- > > .../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/DxeImageVerificationL= ib.c > > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL= ib.c > > @@ -822,22 +822,23 @@ AddImageExeInfo ( > > @param[in] SignatureList Pointer to the Signature List in forbi= dden > database. > > > > @param[in] SignatureListSize Size of Signature List. > > > > @param[out] RevocationTime Return the time that the certificate w= as > > revoked. > > > > + @param[out] IsFound Search result. Only valid if EFI_SUCCE= SS > returned. > > > > > > > > - @return TRUE The certificate hash is found in the forbidden databa= se. > > > > - @return FALSE The certificate hash is not found in the forbidden da= tabase. > > > > + @retval EFI_SUCCESS Finished the search without any error. > > > > + @retval Others Error occurred in the search of databa= se. > > > > > > > > **/ > > > > -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 =3D FALSE; > > > > + Status =3D EFI_ABORTED; > > > > + *IsFound =3D FALSE; > > > > DbxList =3D SignatureList; > > > > DbxSize =3D SignatureListSize; > > > > HashCtx =3D NULL; > > > > HashAlg =3D HASHALG_MAX; > > > > > > > > if ((RevocationTime =3D=3D NULL) || (DbxList =3D=3D 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 >=3D DbxList->SignatureLi= stSize)) { > > > > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase ( > > if (HashCtx =3D=3D NULL) { > > > > goto Done; > > > > } > > > > - Status =3D mHash[HashAlg].HashInit (HashCtx); > > > > - if (!Status) { > > > > + if (!mHash[HashAlg].HashInit (HashCtx)) { > > > > goto Done; > > > > } > > > > - Status =3D mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSiz= e); > > > > - if (!Status) { > > > > + if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) { > > > > goto Done; > > > > } > > > > - Status =3D 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 =3D TRUE; > > > > + Status =3D EFI_SUCCESS; > > > > + *IsFound =3D TRUE; > > > > > > > > // > > > > // Return the revocation time. > > > > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase ( > > DbxList =3D (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList- > > >SignatureListSize); > > > > } > > > > > > > > + Status =3D EFI_SUCCESS; > > > > + > > > > Done: > > > > if (HashCtx !=3D 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 =3D CertPtr + sizeof (UINT32) + CertSize; > > > > > > > > - if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST > *)Data, > > DataSize, &RevocationTime)) { > > > > + Status =3D IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATU= RE_LIST > > *)Data, DataSize, &RevocationTime, &IsFound); > > > > + if (EFI_ERROR (Status) || IsFound) { > > > > // > > > > // Check the timestamp signature and signing time to determine i= f the > image > > can be trusted. > > > > // > > > > IsForbidden =3D TRUE; > > > > - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)= ) { > > > > + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDat= aSize, > > &RevocationTime)) { > > > > IsForbidden =3D FALSE; > > > > // > > > > // Pass DBT check. Continue to check other certs in image sign= er'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 !=3D NULL) { > > > > + VerifyStatus =3D 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 det= ermine if the > > RootCert can be trusted. > > > > - // > > > > - VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataS= ize, > > &RevocationTime); > > > > - if (!VerifyStatus) { > > > > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is= signed > and > > signature is accepted by DB, but its root cert failed the timestamp che= ck.\n")); > > > > - } > > > > + Status =3D IsCertHashFoundInDatabase (RootCert, RootCertSi= ze, > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound); > > > > + if (EFI_ERROR (Status)) { > > > > + goto Done; > > > > + } > > > > + > > > > + if (!IsFound) { > > > > + VerifyStatus =3D TRUE; > > > > + goto Done; > > > > + } > > > > + > > > > + // > > > > + // Check the timestamp signature and signing time to deter= mine if the > > RootCert can be trusted. > > > > + // > > > > + VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSiz= e, > > &RevocationTime); > > > > + if (!VerifyStatus) { > > > > + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is s= igned > and > > signature is accepted by DB, but its root cert failed the timestamp che= ck.\n")); > > > > } > > > > } > > > > > > > > -- > > 2.24.0.windows.2