From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web11.1651.1581651112671542727 for ; Thu, 13 Feb 2020 19:31:52 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: jian.j.wang@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2020 19:31:51 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,439,1574150400"; d="scan'208";a="267407449" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 13 Feb 2020 19:31:51 -0800 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 19:31:51 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 19:31:50 -0800 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.46]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.196]) with mapi id 14.03.0439.000; Fri, 14 Feb 2020 11:31:49 +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+oTmrqgY64tAgABSGWCAAKiyAIAAKExQ Date: Fri, 14 Feb 2020 03:31:48 +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> <74D8A39837DF1E4DA445A8C0B3885C503F92EC03@shsmsx102.ccr.corp.intel.com> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503F92EC03@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTVjNzk5NzAtMWZjOS00MGUwLWI1YmYtODFjYjJmNjYzYTEzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoieVI5UElUOHN2NGpDRHZVdU9vOHdBb1EraFc3Y1dReXBYYnBRTmMxN0pOMlNEVHJFZVwvRkJsUUM4NlRKY2F5MXoifQ== 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, > -----Original Message----- > From: Yao, Jiewen > Sent: Friday, February 14, 2020 8:54 AM > 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 > 1) I prefer we do a little bit simple clean up in this series. Just name = change. > Maybe as patch-10. >=20 Sure. I'll add it in v2. > 2) When PassTimestampCheck() need to be called? Only Dbx is found? Or eve= n > the Dbx is broken? >=20 > I prefer we need use a consistent rule. >=20 > Case 1 in original patch: > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize, > > > &RevocationTime)) { >=20 > Case 2 in your email: > > VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSi= ze, > > &RevocationTime); > > if (!VerifyStatus) { >=20 > It seems they are not consistent... >=20 Just talked to Chao privately. He mentioned that RevocationTime might not be valid if Status !=3D EFI_SUCCESS. So we should only call PassTimestampCh= eck() when Status =3D=3D EFI_SUCCESS and IsFound =3D=3D TRUE. Here's my new proposal (for case 1, case 2 is similar). Status =3D IsCertHashFoundInDatabase (...); if (EFI_ERROR(Status)) { // // Error in searching dbx. Consider it as 'found'. RevocationTime mig= ht // not be valid in such situation. // IsForbidden =3D 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 =3D FALSE; // // Pass DBT check. Continue to check other certs in image signer's = cert list against DBX, DBT // continue; } else { IsForbidden =3D TRUE; DEBUG((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but si= gnature failed the timestamp check.\n")); goto Done; } } If no objection, I'll include it in v2. Regards, Jian > Thank you > Yao Jiewen >=20 >=20 > > -----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: Different= iate > > 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 ser= ies, do you > > think it's ok > > to fix it in separate patch series later? Or do you prefer fix it in th= is patch series? > > I'm ok with > > both. > > > > 2) I checked both code again. I think you're right. Both callings are f= or dbx, 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 dete= rmine if the > > RootCert can be trusted. > > // > > VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSi= ze, > > &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: Differe= ntiate > > > 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 Signatur= eListSize; } > 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: > > > > > > =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 the > > image > > > can be trusted. > > > // > > > IsForbidden =3D TRUE; > > > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, > AuthDataSize, > > > &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= revoked > > > // > > > Status =3D IsCertHashFoundInDatabase (RootCert, RootCertS= ize, > > > (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 dete= rmine if the > > > RootCert can be trusted. > > > // > > > VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSi= ze, > > > &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 = 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 bot= h > > > IsCertHashFoundInDatabase() and PassTimestampCheck() ? > > > > > > If I am wrong, there is *difference* between them. Then I think we ne= ed > 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: Different= iate > > 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 functi= on > > > > will know exactly if a failure is caused by a black list hit or > > > > other error happening, and enforce a more secure operation to preve= nt > > > > 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/DxeImageVerificationL= ib.c > > > > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL= ib.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 f= orbidden > > > database. > > > > > > > > @param[in] SignatureListSize Size of Signature List. > > > > > > > > @param[out] RevocationTime Return the time that the certifica= te was > > > > revoked. > > > > > > > > + @param[out] IsFound Search result. Only valid if EFI_S= UCCESS > > > returned. > > > > > > > > > > > > > > > > - @return TRUE The certificate hash is found in the forbidden da= tabase. > > > > > > > > - @return FALSE The certificate hash is not found in the forbidde= n > database. > > > > > > > > + @retval EFI_SUCCESS Finished the search without any er= ror. > > > > > > > > + @retval Others Error occurred in the search of da= tabase. > > > > > > > > > > > > > > > > **/ > > > > > > > > -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, &TBSCertSi= ze)) { > > > > > > > > - return FALSE; > > > > > > > > + return Status; > > > > > > > > } > > > > > > > > > > > > > > > > while ((DbxSize > 0) && (SignatureListSize >=3D DbxList->Signatu= reListSize)) { > > > > > > > > @@ -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, TBSCer= tSize); > > > > > > > > - 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 + DbxLi= st- > > > > >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_SIGNATURE_LIST > > > > *)Data, DataSize, &RevocationTime, &IsFound); > > > > > > > > + if (EFI_ERROR (Status) || IsFound) { > > > > > > > > // > > > > > > > > // Check the timestamp signature and signing time to determi= ne if the > > > image > > > > can be trusted. > > > > > > > > // > > > > > > > > IsForbidden =3D TRUE; > > > > > > > > - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationT= ime)) > { > > > > > > > > + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, > > AuthDataSize, > > > > &RevocationTime)) { > > > > > > > > IsForbidden =3D 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 !=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= determine if > > the > > > > RootCert can be trusted. > > > > > > > > - // > > > > > > > > - VerifyStatus =3D PassTimestampCheck (AuthData, AuthD= ataSize, > > > > &RevocationTime); > > > > > > > > - if (!VerifyStatus) { > > > > > > > > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Imag= e is > signed > > > and > > > > signature is accepted by DB, but its root cert failed the timestamp > check.\n")); > > > > > > > > - } > > > > > > > > + Status =3D IsCertHashFoundInDatabase (RootCert, RootCe= rtSize, > > > > (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 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 > check.\n")); > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > -- > > > > 2.24.0.windows.2