From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web09.485.1581641660782296004 for ; Thu, 13 Feb 2020 16:54:20 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: jiewen.yao@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2020 16:54:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,438,1574150400"; d="scan'208";a="238212217" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga006.jf.intel.com with ESMTP; 13 Feb 2020 16:54:19 -0800 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 16:54:19 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 16:54:18 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.126]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.222]) with mapi id 14.03.0439.000; Fri, 14 Feb 2020 08:54:17 +0800 From: "Yao, Jiewen" To: "Wang, Jian J" , "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+oTmrqgY64tAgABSGWCAAKiyAA== Date: Fri, 14 Feb 2020 00:54:16 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F92EC03@shsmsx102.ccr.corp.intel.com> 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: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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: jiewen.yao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 1) I prefer we do a little bit simple clean up in this series. Just name ch= ange. 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 =3D PassTimestampCheck (AuthData, AuthDataSize= , > &RevocationTime); > if (!VerifyStatus) { It seems they are not consistent... Thank you Yao Jiewen > -----Original Message----- > From: Wang, Jian J > Sent: Thursday, February 13, 2020 11:08 PM > To: Yao, Jiewen ; 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 > Jiewen, >=20 > Thanks for the comments. >=20 > 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 serie= s, 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. >=20 > 2) I checked both code again. I think you're right. Both callings are for= dbx, any > error Status > should be taken as IsFound(=3D=3DTRUE). What about following change for t= he > second case? > Please help double check if any logic hole here. >=20 > Status =3D IsCertHashFoundInDatabase (...); > if (EFI_ERROR (Status) || IsFound) { > // > // Check the timestamp signature and signing time to determ= ine if the > RootCert can be trusted. > // > VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSize= , > &RevocationTime); > if (!VerifyStatus) { > DEBUG ((...)); > } > } else { > VerifyStatus =3D TRUE; > } >=20 > goto Done; >=20 > Regards, > Jian >=20 > > -----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: Different= iate > > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575) > > > > Comment below: > > > > 1) I think the function name - IsCertHashFoundInDatabase() and the > > implementation { DbxList =3D SignatureList; DbxSize =3D SignatureL= istSize; } bring > > some confusion to me. > > > > If this is a *generic* database search function, I recommend we use a g= eneric > > name - not use DbxList/DbxSize in the function implementation. > > > > If the input SignatureList of the function must be *Dbx*, I recommend w= e 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: > > > > =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_SIGNATUR= E_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 =3D TRUE; > > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthData= Size, > > &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 > > > > and > > > > =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 r= evoked > > // > > Status =3D IsCertHashFoundInDatabase (RootCert, RootCertSiz= e, > > (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 determ= ine 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 > > > > I *believe* the logic behind is same. If so, we can use a consistent wa= y 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 > > > 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: Differentia= te > 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/DxeImageVerificationLib.c > > > +++ > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > > > @@ -822,22 +822,23 @@ AddImageExeInfo ( > > > @param[in] SignatureList Pointer to the Signature List in for= bidden > > 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_SUC= CESS > > returned. > > > > > > > > > > > > - @return TRUE The certificate hash is found in the forbidden data= base. > > > > > > - @return FALSE The certificate hash is not found in the forbidden = database. > > > > > > + @retval EFI_SUCCESS Finished the search without any erro= r. > > > > > > + @retval Others Error occurred in the search of data= base. > > > > > > > > > > > > **/ > > > > > > -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->Signature= ListSize)) { > > > > > > @@ -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, TBSCertS= ize); > > > > > > - 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_LI= ST > > *)Data, > > > DataSize, &RevocationTime)) { > > > > > > + 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 the > > image > > > can be trusted. > > > > > > // > > > > > > IsForbidden =3D TRUE; > > > > > > - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTim= e)) { > > > > > > + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, > AuthDataSize, > > > &RevocationTime)) { > > > > > > IsForbidden =3D FALSE; > > > > > > // > > > > > > // Pass DBT check. Continue to check other certs in image si= gner'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 i= s revoked > > > > > > // > > > > > > - if (IsCertHashFoundInDatabase (RootCert, RootCertSize, > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) { > > > > > > - // > > > > > > - // Check the timestamp signature and signing time to d= etermine if > the > > > RootCert can be trusted. > > > > > > - // > > > > > > - VerifyStatus =3D PassTimestampCheck (AuthData, AuthDat= aSize, > > > &RevocationTime); > > > > > > - if (!VerifyStatus) { > > > > > > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image = is signed > > and > > > signature is accepted by DB, but its root cert failed the timestamp c= heck.\n")); > > > > > > - } > > > > > > + Status =3D IsCertHashFoundInDatabase (RootCert, RootCert= Size, > > > (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 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 c= heck.\n")); > > > > > > } > > > > > > } > > > > > > > > > > > > -- > > > 2.24.0.windows.2