From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web09.4405.1581588675437582896 for ; Thu, 13 Feb 2020 02:11:15 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: jiewen.yao@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2020 02:11:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,436,1574150400"; d="scan'208";a="281485393" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by FMSMGA003.fm.intel.com with ESMTP; 13 Feb 2020 02:11:14 -0800 Received: from fmsmsx163.amr.corp.intel.com (10.18.125.72) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 02:11:14 -0800 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx163.amr.corp.intel.com (10.18.125.72) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 02:11:14 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.126]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.97]) with mapi id 14.03.0439.000; Thu, 13 Feb 2020 18:11:12 +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+oTmrqgY64tA Date: Thu, 13 Feb 2020 10:11:11 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F92CC3F@shsmsx102.ccr.corp.intel.com> References: <20200206141933.356-1-jian.j.wang@intel.com> <20200206141933.356-7-jian.j.wang@intel.com> In-Reply-To: <20200206141933.356-7-jian.j.wang@intel.com> 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 Comment below: 1) I think the function name - IsCertHashFoundInDatabase() and the implemen= tation { DbxList =3D SignatureList; DbxSize =3D SignatureListSize; } b= ring some confusion to me. If this is a *generic* database search function, I recommend we use a gener= ic name - not use DbxList/DbxSize in the function implementation. If the input SignatureList of the function must be *Dbx*, I recommend we us= e IsCertHashFoundInDbx() as the function name. Either change is OK for me. 2) Now we have to check 2 output: Status and IsFound in IsCertHashFoundInDa= tabase(). I am struggling to understand the different between 2 different ways of err= or 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_LI= ST *)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 revok= ed // Status =3D IsCertHashFoundInDatabase (RootCert, RootCertSize, (= 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 determine = if the RootCert can be trusted. // VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSize, &R= evocationTime); 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 both IsCe= rtHashFoundInDatabase() and PassTimestampCheck() ? If I am wrong, there is *difference* between them. Then I think we need muc= h 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: Differentiate e= rror > and search result in IsCertHashFoundInDatabase(CVE-2019-14575) >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1608 >=20 > 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. >=20 > 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(-) >=20 > 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 forbidd= en database. >=20 > @param[in] SignatureListSize Size of Signature List. >=20 > @param[out] RevocationTime Return the time that the certificate was > revoked. >=20 > + @param[out] IsFound Search result. Only valid if EFI_SUCCESS= returned. >=20 >=20 >=20 > - @return TRUE The certificate hash is found in the forbidden database= . >=20 > - @return FALSE The certificate hash is not found in the forbidden data= base. >=20 > + @retval EFI_SUCCESS Finished the search without any error. >=20 > + @retval Others Error occurred in the search of database= . >=20 >=20 >=20 > **/ >=20 > -BOOLEAN >=20 > +EFI_STATUS >=20 > IsCertHashFoundInDatabase ( >=20 > IN UINT8 *Certificate, >=20 > IN UINTN CertSize, >=20 > IN EFI_SIGNATURE_LIST *SignatureList, >=20 > IN UINTN SignatureListSize, >=20 > - OUT EFI_TIME *RevocationTime >=20 > + OUT EFI_TIME *RevocationTime, >=20 > + OUT BOOLEAN *IsFound >=20 > ) >=20 > { >=20 > - BOOLEAN IsFound; >=20 > - BOOLEAN Status; >=20 > + EFI_STATUS Status; >=20 > EFI_SIGNATURE_LIST *DbxList; >=20 > UINTN DbxSize; >=20 > EFI_SIGNATURE_DATA *CertHash; >=20 > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase ( > UINT8 *TBSCert; >=20 > UINTN TBSCertSize; >=20 >=20 >=20 > - IsFound =3D FALSE; >=20 > + Status =3D EFI_ABORTED; >=20 > + *IsFound =3D FALSE; >=20 > DbxList =3D SignatureList; >=20 > DbxSize =3D SignatureListSize; >=20 > HashCtx =3D NULL; >=20 > HashAlg =3D HASHALG_MAX; >=20 >=20 >=20 > if ((RevocationTime =3D=3D NULL) || (DbxList =3D=3D NULL)) { >=20 > - return FALSE; >=20 > + return EFI_INVALID_PARAMETER; >=20 > } >=20 >=20 >=20 > // >=20 > // Retrieve the TBSCertificate from the X.509 Certificate. >=20 > // >=20 > if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) { >=20 > - return FALSE; >=20 > + return Status; >=20 > } >=20 >=20 >=20 > while ((DbxSize > 0) && (SignatureListSize >=3D DbxList->SignatureList= Size)) { >=20 > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase ( > if (HashCtx =3D=3D NULL) { >=20 > goto Done; >=20 > } >=20 > - Status =3D mHash[HashAlg].HashInit (HashCtx); >=20 > - if (!Status) { >=20 > + if (!mHash[HashAlg].HashInit (HashCtx)) { >=20 > goto Done; >=20 > } >=20 > - Status =3D mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)= ; >=20 > - if (!Status) { >=20 > + if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) { >=20 > goto Done; >=20 > } >=20 > - Status =3D mHash[HashAlg].HashFinal (HashCtx, CertDigest); >=20 > - if (!Status) { >=20 > + if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) { >=20 > goto Done; >=20 > } >=20 >=20 >=20 > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase ( > // >=20 > // Hash of Certificate is found in forbidden database. >=20 > // >=20 > - IsFound =3D TRUE; >=20 > + Status =3D EFI_SUCCESS; >=20 > + *IsFound =3D TRUE; >=20 >=20 >=20 > // >=20 > // Return the revocation time. >=20 > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase ( > DbxList =3D (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList- > >SignatureListSize); >=20 > } >=20 >=20 >=20 > + Status =3D EFI_SUCCESS; >=20 > + >=20 > Done: >=20 > if (HashCtx !=3D NULL) { >=20 > FreePool (HashCtx); >=20 > } >=20 >=20 >=20 > - return IsFound; >=20 > + return Status; >=20 > } >=20 >=20 >=20 > /** >=20 > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx ( > { >=20 > EFI_STATUS Status; >=20 > BOOLEAN IsForbidden; >=20 > + BOOLEAN IsFound; >=20 > UINT8 *Data; >=20 > UINTN DataSize; >=20 > EFI_SIGNATURE_LIST *CertList; >=20 > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx ( > // >=20 > CertPtr =3D CertPtr + sizeof (UINT32) + CertSize; >=20 >=20 >=20 > - if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *= )Data, > DataSize, &RevocationTime)) { >=20 > + Status =3D IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE= _LIST > *)Data, DataSize, &RevocationTime, &IsFound); >=20 > + if (EFI_ERROR (Status) || IsFound) { >=20 > // >=20 > // Check the timestamp signature and signing time to determine if = the image > can be trusted. >=20 > // >=20 > IsForbidden =3D TRUE; >=20 > - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) = { >=20 > + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataS= ize, > &RevocationTime)) { >=20 > IsForbidden =3D FALSE; >=20 > // >=20 > // Pass DBT check. Continue to check other certs in image signer= 's cert list > against DBX, DBT >=20 > @@ -1392,6 +1396,7 @@ IsAllowedByDb ( > { >=20 > EFI_STATUS Status; >=20 > BOOLEAN VerifyStatus; >=20 > + BOOLEAN IsFound; >=20 > EFI_SIGNATURE_LIST *CertList; >=20 > EFI_SIGNATURE_DATA *CertData; >=20 > UINTN DataSize; >=20 > @@ -1495,17 +1500,26 @@ IsAllowedByDb ( > // The image is signed and its signature is found in 'db'. >=20 > // >=20 > if (DbxData !=3D NULL) { >=20 > + VerifyStatus =3D FALSE; >=20 > // >=20 > // Here We still need to check if this RootCert's Hash is re= voked >=20 > // >=20 > - if (IsCertHashFoundInDatabase (RootCert, RootCertSize, > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) { >=20 > - // >=20 > - // Check the timestamp signature and signing time to deter= mine if the > RootCert can be trusted. >=20 > - // >=20 > - VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSiz= e, > &RevocationTime); >=20 > - if (!VerifyStatus) { >=20 > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is s= igned and > signature is accepted by DB, but its root cert failed the timestamp check= .\n")); >=20 > - } >=20 > + Status =3D IsCertHashFoundInDatabase (RootCert, RootCertSize= , > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound); >=20 > + if (EFI_ERROR (Status)) { >=20 > + goto Done; >=20 > + } >=20 > + >=20 > + if (!IsFound) { >=20 > + VerifyStatus =3D TRUE; >=20 > + goto Done; >=20 > + } >=20 > + >=20 > + // >=20 > + // Check the timestamp signature and signing time to determi= ne if the > RootCert can be trusted. >=20 > + // >=20 > + VerifyStatus =3D PassTimestampCheck (AuthData, AuthDataSize, > &RevocationTime); >=20 > + if (!VerifyStatus) { >=20 > + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is sig= ned and > signature is accepted by DB, but its root cert failed the timestamp check= .\n")); >=20 > } >=20 > } >=20 >=20 >=20 > -- > 2.24.0.windows.2