From nobody Mon Feb 9 03:13:21 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of groups.io designates 66.175.222.12 as permitted sender) client-ip=66.175.222.12; envelope-from=bounce+27952+54421+1787277+3901457@groups.io; helo=web01.groups.io; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of groups.io designates 66.175.222.12 as permitted sender) smtp.mailfrom=bounce+27952+54421+1787277+3901457@groups.io; dmarc=fail(p=none dis=none) header.from=intel.com Received: from web01.groups.io (web01.groups.io [66.175.222.12]) by mx.zohomail.com with SMTPS id 1581665273083622.2576468848389; Thu, 13 Feb 2020 23:27:53 -0800 (PST) Return-Path: X-Received: by 127.0.0.2 with SMTP id z77aYY1788612x8kj8hQMil2; Thu, 13 Feb 2020 23:27:52 -0800 X-Received: from mga04.intel.com (mga04.intel.com []) by mx.groups.io with SMTP id smtpd.web11.3343.1581665266656025367 for ; Thu, 13 Feb 2020 23:27:51 -0800 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2020 23:27:51 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,439,1574150400"; d="scan'208";a="347904221" X-Received: from shwdeopensfp777.ccr.corp.intel.com ([10.239.158.78]) by fmsmga001.fm.intel.com with ESMTP; 13 Feb 2020 23:27:49 -0800 From: "Wang, Jian J" To: devel@edk2.groups.io Cc: Jiewen Yao , Chao Zhang Subject: [edk2-devel] [PATCH v2 05/10] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code(CVE-2019-14575) Date: Fri, 14 Feb 2020 15:27:40 +0800 Message-Id: <20200214072745.1570-6-jian.j.wang@intel.com> In-Reply-To: <20200214072745.1570-1-jian.j.wang@intel.com> References: <20200214072745.1570-1-jian.j.wang@intel.com> MIME-Version: 1.0 Precedence: Bulk List-Unsubscribe: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,jian.j.wang@intel.com X-Gm-Message-State: vuEDbBtxR25M0Ocdk6eAZ04Vx1787277AA= Content-Transfer-Encoding: quoted-printable DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=groups.io; q=dns/txt; s=20140610; t=1581665272; bh=WsTBWH8El36+qO8TGxoxBsTsvfNz7XAKIzYhJWrpMAQ=; h=Cc:Date:From:Reply-To:Subject:To; b=qbScwep/n7Bb8iZPJ4Vfr9eL1o2eCVuZvU0+7KZ0y8rUUhr67bD6Z5So1VRdlEwUZ0r hZzchFo4+QV85zJzCN6+OaIvXmf0dMMkJE4hm3b+pf+Q7vCL1fTgRpgsfQIQvw54svNZ8 2r0v283pK3pFNkzUHWitIYaTCV3ZYbRRYfg= X-ZohoMail-DKIM: pass (identity @groups.io) Content-Type: text/plain; charset="utf-8" REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1608 The dbx fetching code inside the while/for-loop causes code hard to understand. Since there's no need to get dbx more than once, this patch simplify the code logic by moving related code to be outside the while- loop. db fetching code is also refined accordingly to reduce the indent level of code. More comments are also added or refined to explain more details. Cc: Jiewen Yao Cc: Chao Zhang Signed-off-by: Jian J Wang Reviewed-by: Jiewen Yao --- .../DxeImageVerificationLib.c | 144 ++++++++++-------- 1 file changed, 83 insertions(+), 61 deletions(-) diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificati= onLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL= ib.c index ed5dbf26b0..8739d1fa29 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c @@ -1412,76 +1412,92 @@ IsAllowedByDb ( RootCertSize =3D 0; VerifyStatus =3D FALSE; =20 + // + // Fetch 'db' content. If 'db' doesn't exist or encounters problem to ge= t the + // data, return not-allowed-by-db (FALSE). + // DataSize =3D 0; Status =3D gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSe= curityDatabaseGuid, NULL, &DataSize, NULL); - if (Status =3D=3D EFI_BUFFER_TOO_SMALL) { - Data =3D (UINT8 *) AllocateZeroPool (DataSize); - if (Data =3D=3D NULL) { - return VerifyStatus; + ASSERT (EFI_ERROR (Status)); + if (Status !=3D EFI_BUFFER_TOO_SMALL) { + return VerifyStatus; + } + + Data =3D (UINT8 *) AllocateZeroPool (DataSize); + if (Data =3D=3D NULL) { + return VerifyStatus; + } + + Status =3D gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecu= rityDatabaseGuid, NULL, &DataSize, (VOID *) Data); + if (EFI_ERROR (Status)) { + goto Done; + } + + // + // Fetch 'dbx' content. If 'dbx' doesn't exist, continue to check 'db'. + // If any other errors occured, no need to check 'db' but just return + // not-allowed-by-db (FALSE) to avoid bypass. + // + DbxDataSize =3D 0; + Status =3D gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiIma= geSecurityDatabaseGuid, NULL, &DbxDataSize, NULL); + ASSERT (EFI_ERROR (Status)); + if (Status !=3D EFI_BUFFER_TOO_SMALL) { + if (Status !=3D EFI_NOT_FOUND) { + goto Done; + } + // + // 'dbx' does not exist. Continue to check 'db'. + // + } else { + // + // 'dbx' exists. Get its content. + // + DbxData =3D (UINT8 *) AllocateZeroPool (DbxDataSize); + if (DbxData =3D=3D NULL) { + goto Done; } =20 - Status =3D gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSe= curityDatabaseGuid, NULL, &DataSize, (VOID *) Data); + Status =3D gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageS= ecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData); if (EFI_ERROR (Status)) { goto Done; } + } =20 - // - // Find X509 certificate in Signature List to verify the signature in = pkcs7 signed data. - // - CertList =3D (EFI_SIGNATURE_LIST *) Data; - while ((DataSize > 0) && (DataSize >=3D CertList->SignatureListSize)) { - if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) { - CertData =3D (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof = (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize); - CertCount =3D (CertList->SignatureListSize - sizeof (EFI_SIGNATURE= _LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize; + // + // Find X509 certificate in Signature List to verify the signature in pk= cs7 signed data. + // + CertList =3D (EFI_SIGNATURE_LIST *) Data; + while ((DataSize > 0) && (DataSize >=3D CertList->SignatureListSize)) { + if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) { + CertData =3D (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (E= FI_SIGNATURE_LIST) + CertList->SignatureHeaderSize); + CertCount =3D (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_L= IST) - CertList->SignatureHeaderSize) / CertList->SignatureSize; =20 - for (Index =3D 0; Index < CertCount; Index++) { - // - // Iterate each Signature Data Node within this CertList for ver= ify. - // - RootCert =3D CertData->SignatureData; - RootCertSize =3D CertList->SignatureSize - sizeof (EFI_GUID); + for (Index =3D 0; Index < CertCount; Index++) { + // + // Iterate each Signature Data Node within this CertList for verif= y. + // + RootCert =3D CertData->SignatureData; + RootCertSize =3D CertList->SignatureSize - sizeof (EFI_GUID); =20 + // + // Call AuthenticodeVerify library to Verify Authenticode struct. + // + VerifyStatus =3D AuthenticodeVerify ( + AuthData, + AuthDataSize, + RootCert, + RootCertSize, + mImageDigest, + mImageDigestSize + ); + if (VerifyStatus) { // - // Call AuthenticodeVerify library to Verify Authenticode struct. + // The image is signed and its signature is found in 'db'. // - VerifyStatus =3D AuthenticodeVerify ( - AuthData, - AuthDataSize, - RootCert, - RootCertSize, - mImageDigest, - mImageDigestSize - ); - if (VerifyStatus) { + if (DbxData !=3D NULL) { // // Here We still need to check if this RootCert's Hash is revo= ked // - DbxDataSize =3D 0; - Status =3D gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &= gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL); - if (Status !=3D EFI_BUFFER_TOO_SMALL) { - if (Status !=3D EFI_NOT_FOUND) { - VerifyStatus =3D FALSE; - } - goto Done; - } - DbxData =3D (UINT8 *) AllocateZeroPool (DbxDataSize); - if (DbxData =3D=3D NULL) { - // - // Force not-allowed-by-db to avoid bypass - // - VerifyStatus =3D FALSE; - goto Done; - } - - Status =3D gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gE= fiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData); - if (EFI_ERROR (Status)) { - // - // Force not-allowed-by-db to avoid bypass - // - VerifyStatus =3D FALSE; - goto Done; - } - if (IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SI= GNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) { // // Check the timestamp signature and signing time to determi= ne if the RootCert can be trusted. @@ -1491,17 +1507,23 @@ IsAllowedByDb ( DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is sig= ned and signature is accepted by DB, but its root cert failed the timestamp= check.\n")); } } - - goto Done; } =20 - CertData =3D (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertLi= st->SignatureSize); + // + // There's no 'dbx' to check revocation time against (must-be pa= ss), + // or, there's revocation time found in 'dbx' and checked againt= 'dbt' + // (maybe pass or fail, depending on timestamp compare result). = Either + // way the verification job has been completed at this point. + // + goto Done; } - } =20 - DataSize -=3D CertList->SignatureListSize; - CertList =3D (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList->= SignatureListSize); + CertData =3D (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList= ->SignatureSize); + } } + + DataSize -=3D CertList->SignatureListSize; + CertList =3D (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList->Si= gnatureListSize); } =20 Done: --=20 2.24.0.windows.2 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54421): https://edk2.groups.io/g/devel/message/54421 Mute This Topic: https://groups.io/mt/71264903/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-