From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web11.3475.1581666438318520863 for ; Thu, 13 Feb 2020 23:47:18 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: jiewen.yao@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2020 23:47:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,439,1574150400"; d="scan'208";a="228402173" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga008.fm.intel.com with ESMTP; 13 Feb 2020 23:47:18 -0800 Received: from fmsmsx162.amr.corp.intel.com (10.18.125.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 23:47:17 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx162.amr.corp.intel.com (10.18.125.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 13 Feb 2020 23:47:17 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.126]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.98]) with mapi id 14.03.0439.000; Fri, 14 Feb 2020 15:47:15 +0800 From: "Yao, Jiewen" To: "Wang, Jian J" , "devel@edk2.groups.io" CC: "Zhang, Chao B" , Laszlo Ersek Subject: Re: [PATCH v2 06/10] SecurityPkg/DxeImageVerificationLib: Differentiate error/search result (1)(CVE-2019-14575) Thread-Topic: [PATCH v2 06/10] SecurityPkg/DxeImageVerificationLib: Differentiate error/search result (1)(CVE-2019-14575) Thread-Index: AQHV4whHMN61E15OYkyy6+RtqFGD9agaT9Dw Date: Fri, 14 Feb 2020 07:47:15 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F92F8EA@shsmsx102.ccr.corp.intel.com> References: <20200214072745.1570-1-jian.j.wang@intel.com> <20200214072745.1570-7-jian.j.wang@intel.com> In-Reply-To: <20200214072745.1570-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 Reviewed-by: Jiewen Yao > -----Original Message----- > From: Wang, Jian J > Sent: Friday, February 14, 2020 3:28 PM > To: devel@edk2.groups.io > Cc: Yao, Jiewen ; Zhang, Chao B > ; Laszlo Ersek > Subject: [PATCH v2 06/10] SecurityPkg/DxeImageVerificationLib: Differenti= ate > error/search result (1)(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 | 64 ++++++++++++------- > 1 file changed, 42 insertions(+), 22 deletions(-) >=20 > diff --git > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index 8739d1fa29..85261ba7f2 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,20 +1347,29 @@ 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)) { >=20 > // >=20 > - // Check the timestamp signature and signing time to determine if = the image > can be trusted. >=20 > + // Error in searching dbx. Consider it as 'found'. RevocationTime = might >=20 > + // not be valid in such situation. >=20 > // >=20 > IsForbidden =3D TRUE; >=20 > + } else if (IsFound) { >=20 > + // >=20 > + // Found Cert in dbx successfully. Check the timestamp signature a= nd >=20 > + // signing time to determine if the image can be trusted. >=20 > + // >=20 > if (PassTimestampCheck (AuthData, AuthDataSize, &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 > // >=20 > continue; >=20 > + } else { >=20 > + IsForbidden =3D TRUE; >=20 > + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed bu= t > signature failed the timestamp check.\n")); >=20 > + goto Done; >=20 > } >=20 > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but > signature failed the timestamp check.\n")); >=20 > - goto Done; >=20 > } >=20 >=20 >=20 > } >=20 > @@ -1392,6 +1404,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 > @@ -1498,7 +1511,14 @@ IsAllowedByDb ( > // >=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 > + Status =3D IsCertHashFoundInDatabase (RootCert, RootCertSize= , > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound); >=20 > + if (EFI_ERROR (Status)) { >=20 > + // >=20 > + // Error in searching dbx. Consider it as 'found'. Revocat= ionTime might >=20 > + // not be valid in such situation. >=20 > + // >=20 > + VerifyStatus =3D FALSE; >=20 > + } else if (IsFound) { >=20 > // >=20 > // Check the timestamp signature and signing time to deter= mine if the > RootCert can be trusted. >=20 > // >=20 > -- > 2.24.0.windows.2