From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web10.11317.1593400700294289949 for ; Sun, 28 Jun 2020 20:18:20 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=MW2wubTG; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: guomin.jiang@intel.com) IronPort-SDR: ywASWPUDsiJdfw00ieRITLHELLlG1hRW1h2ZOD4SO7z15SSIwTP6ebrubS3uHGyB1TkDZk7GVI RT+s1AhTn4Vw== X-IronPort-AV: E=McAfee;i="6000,8403,9666"; a="163914508" X-IronPort-AV: E=Sophos;i="5.75,293,1589266800"; d="scan'208";a="163914508" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jun 2020 20:18:19 -0700 IronPort-SDR: Pjm3TXeodeNca1lZjjBrYUFmU4OCoWrlkMdaHIMe75ylKnn2S8GaQQCH5cz7bHBjrhP8M2NTjS gxXU3BhtXzng== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,293,1589266800"; d="scan'208";a="386271835" Received: from orsmsx108.amr.corp.intel.com ([10.22.240.6]) by fmsmga001.fm.intel.com with ESMTP; 28 Jun 2020 20:18:19 -0700 Received: from orsmsx601.amr.corp.intel.com (10.22.229.14) by ORSMSX108.amr.corp.intel.com (10.22.240.6) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 28 Jun 2020 20:18:19 -0700 Received: from orsmsx609.amr.corp.intel.com (10.22.229.22) by ORSMSX601.amr.corp.intel.com (10.22.229.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Sun, 28 Jun 2020 20:18:18 -0700 Received: from ORSEDG001.ED.cps.intel.com (10.7.248.4) by orsmsx609.amr.corp.intel.com (10.22.229.22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Sun, 28 Jun 2020 20:18:18 -0700 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.171) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 28 Jun 2020 20:18:18 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=aL4oT1nzW6XmANOVYpdwlWb8jd9mKyqvY+iZ21POJzUBiojT8AuZWQvEwBXNqw7jD21pqR2DBVoa9TDiyFTKhZu+6eTkzKGRa9s9giu8ofIo+kltjt2ngSMS2gOwK/w9J9hLvNi/MnrphCd+ah9VMHqonyvEn6R/5Ey0iGYLOhO6KulUTv3ZGorpd++35jrsmcoIxOvjZ/9LKHh+/BiHrYCmrRQJPRCg6WPhdOnL8dPySJtvMHdT1QVqJ1i39KBbM88+0AUegOb3m5SdUCAlv93pwlFbetBuY0dGU4ez1pQjvzmS7fGECcgw4AHkzp6P1UjtXSSZEKaEQ/eDsdjCKQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Id1TktpL6WIjEzuNv2H9vmWZvBVYgsHN27FJrWr6UL4=; b=hZALPdKWUhu4Uuk5BnLnUc0qZrdUHVgb4Hw4NnI3WZi6QGKwMHG/63KvIKD5vAtiq8XBZlEW3+cFTe+qXLDz1mrjg290H8g5u9aLUrzF45raGJ2hOk3CZubQ2/OXrXQBp8SEXtMQYMGXkZ7ChQnCX0nejzBrtge2SZnuobuhxsoBcXO/CfI2bcJ8G/yPzHhj3e7jVwYp/a0MLsQ4LdB2Fxb7LdlKZ7QlEP2ndwqSCepteDsLC7+Jr0ORb40becTlWAlwNnpBHC9mvBoLPgoJ9ke4MoNfEUsT1jJ1/TnrQm48MtDgbha/dJStCcXFbpX0yXHhAoVAatFkvewamtf2kQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Id1TktpL6WIjEzuNv2H9vmWZvBVYgsHN27FJrWr6UL4=; b=MW2wubTGeipaz6pcA5/QgY+ZKPFJzalRxIT3I5rSvtvrVRr8QPyKfSQwNhYlbXmHUKYIA/zE8VR3gXDvidQOjb+ipqz8uteK7kNtmYclnc/3/rTF/QJPFidWRFA1uDI4Taa2C+9OakPyH65ttqsdGtJbq+bURLE7QXu2J5mkCUw= Received: from DM6PR11MB2955.namprd11.prod.outlook.com (2603:10b6:5:65::31) by DM6PR11MB3689.namprd11.prod.outlook.com (2603:10b6:5:143::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3131.20; Mon, 29 Jun 2020 03:18:16 +0000 Received: from DM6PR11MB2955.namprd11.prod.outlook.com ([fe80::e916:c766:fc41:b51d]) by DM6PR11MB2955.namprd11.prod.outlook.com ([fe80::e916:c766:fc41:b51d%5]) with mapi id 15.20.3131.026; Mon, 29 Jun 2020 03:18:16 +0000 From: "Guomin Jiang" To: "devel@edk2.groups.io" , "lsun@mellanox.com" , "Xu, Wei6" , "Gao, Liming" , "Kinney, Michael D" CC: Sean Brogan Subject: Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys Thread-Topic: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys Thread-Index: AQHWRmLn4qcSWkWl3EKmf7xe2byxn6ju+QaQ Date: Mon, 29 Jun 2020 03:18:16 +0000 Message-ID: References: <5b42e8e089fb961766c639b733284413ccf03272.1592587621.git.lsun@mellanox.com> In-Reply-To: <5b42e8e089fb961766c639b733284413ccf03272.1592587621.git.lsun@mellanox.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-version: 11.2.0.6 dlp-product: dlpe-windows dlp-reaction: no-action authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.198.147.196] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: a583b4e0-ca31-4a60-701e-08d81bdb10e8 x-ms-traffictypediagnostic: DM6PR11MB3689: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:275; x-forefront-prvs: 044968D9E1 x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: TUGNHIgENRIS2lCIgvnZZ9rC6p6EHktkZgmPMw4LdwTBz7eI93zAFNZrAVaAnEx97+dkriSRUdu7FKw1tSLKaXzp/xxXiK0FLDSF6KAYbwiKwGQp21g487PwkOotfl3w0PFjsyoTc9xL6taB1D2HLETXUz7a5Hrq/bAOzFdWOkcRjznaaJpetqS+/gxkmrZ7pkcZmer4w4CzbRj/wT65P1bmd1WAvZDS62GR2WSj0nMODBI1Sq9AzWJS45YuRT9bpRPsZRbrcFMWZTY+PKVpsh9BMcnl2RLcTaRF5eKnZPu/z2HDUqPPVkU4CrMtcj/WgKw6M61IJXIZZiYrUOR1KYZo1NISGFyEm7r9FuCaEiArZiZES8z8WXXIAUd9YBbnpB8/ABy4vMgQCf3llwtPuw== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM6PR11MB2955.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(136003)(346002)(39860400002)(366004)(376002)(396003)(9686003)(52536014)(83380400001)(33656002)(5660300002)(71200400001)(86362001)(66446008)(66556008)(66476007)(76116006)(64756008)(66946007)(19627235002)(186003)(6636002)(110136005)(55016002)(26005)(2906002)(53546011)(6506007)(8936002)(966005)(7696005)(316002)(15650500001)(8676002)(4326008)(478600001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: tUzSmN5t8YThf4wABJdlRLN4XUOAGz2uvYR8rIHv1QHqh+YG0et8+ECMHIam9iGqsE/DX1ZNrN0kLPU/NhUgE3V9yNt5L+lqRCYRw+/kbc04LxUx9VcsL/K4Ec+0Ckojs9YwAiN2w18huTUBvgoQqf25cbHm/tDpCKVZCc4cZSXTMZkrzgc/WaznzM2pwFNd7OsdFO4M7u7kjC1faxK23LBcZpJRigi7Sh5ElZxudxYYfbOinjpB4UUf2rqC5xpQOltYn564XUFWGV+J5YyNRIKg8CV2L4TLwfTWlcRLzkiUFAFh8P3VMF3k42fAo+6c9FL894OgAWk7p9+6XAf4DoTFsMEXCGlxBF123AEYYmKe7AdEe/yfIxXddMpbGOeeHVY+aCEO3mCjoRkIszVt5eUSmAw7exAV8zC22LMJtF7RS+gh3jwDpLHLkZJDi20CuQWpiXweo85/lza6uo0chbxiCw+XOn7kwH/fs37l2QYIIKh4MHgERxzJH604EqCz MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB2955.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: a583b4e0-ca31-4a60-701e-08d81bdb10e8 X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jun 2020 03:18:16.6932 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: OffP5xDosK7i6VAmES/LOBKSx7743VYXiCdbK54ezP5MjsV6LoFCgzgupDHzXlwcokP3QnWMv/Rr7vHF6I4E1g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB3689 Return-Path: guomin.jiang@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I think it have some vulnerability, the case as below. 1. Untrusted End User enroll the new DB key -> sign the untrusted device f= irmware -> flash the untrusted device firmware -> the system will become un= safe. I think the end user is untrusted and we need to make sure only few person= can have the privilege. Best Regards Guomin > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Liming > Sun > Sent: Saturday, June 20, 2020 1:48 AM > To: Xu, Wei6 ; Gao, Liming ; > Kinney, Michael D > Cc: Liming Sun ; devel@edk2.groups.io; Sean Brogan > > Subject: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification > with secure boot keys >=20 > This commit enhances the FmpDevicePkg package to optionally verify > capsule with the secure boot keys when PcdFmpDevicePkcs7CertBufferXdr is > not set and the new PCD variable PcdFmpDeviceAllowSecureBootKeys is > configured. Below is the check logic: > - Pass if verified with PK key, or PK key not set yet; > - Deny if verified with the DBX keys; > - Verified it against the DB keys; >=20 > One purpose for this change is to auto-deploy the UEFI secure boot keys > with UEFI capsule. Initially it's done in trusted environment. > Once secure boot is enabled, the same keys will be used to verify the si= gned > capsules as well for further updates. >=20 > Signed-off-by: Liming Sun > --- > FmpDevicePkg/FmpDevicePkg.dec | 6 +++ > FmpDevicePkg/FmpDxe/FmpDxe.c | 109 > ++++++++++++++++++++++++++++++++++++-- > FmpDevicePkg/FmpDxe/FmpDxe.h | 1 + > FmpDevicePkg/FmpDxe/FmpDxe.inf | 3 ++ > FmpDevicePkg/FmpDxe/FmpDxeLib.inf | 1 + > 5 files changed, 117 insertions(+), 3 deletions(-) >=20 > diff --git a/FmpDevicePkg/FmpDevicePkg.dec > b/FmpDevicePkg/FmpDevicePkg.dec index cab63f5..3aeb89c 100644 > --- a/FmpDevicePkg/FmpDevicePkg.dec > +++ b/FmpDevicePkg/FmpDevicePkg.dec > @@ -126,6 +126,12 @@ > # @Prompt Firmware Device Image Type ID >=20 > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceImageTypeIdGuid|{0}|VOID > *|0x40000010 >=20 > + ## This option is used to verify the capsule using secure boot keys > + if the # PcdFmpDevicePkcs7CertBufferXdr is not configured. In such > + case, the check # will pass if secure boot hasn't been enabled yet. > + # @A flag to tell whether to use secure boot keys when > PcdFmpDevicePkcs7CertBufferXdr is not set. > + > + > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceAllowSecureBootKeys|0x0| > UINT8| > + 0x40000012 > + > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] > ## One or more PKCS7 certificates used to verify a firmware device ca= psule > # update image. Encoded using the Variable-Length Opaque Data forma= t > of RFC diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c > b/FmpDevicePkg/FmpDxe/FmpDxe.c index 5884177..6f82aee 100644 > --- a/FmpDevicePkg/FmpDxe/FmpDxe.c > +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c > @@ -682,6 +682,102 @@ GetAllHeaderSize ( > return CalculatedSize; > } >=20 > +EFI_STATUS > +CheckTheImageWithSecureBootVariable ( > + IN CONST CHAR16 *Name, > + IN CONST EFI_GUID *Guid, > + IN CONST VOID *Image, > + IN UINTN ImageSize > + ) > +{ > + EFI_STATUS Status; > + VOID *Data; > + UINTN Length; > + EFI_SIGNATURE_LIST *CertList; > + EFI_SIGNATURE_DATA *CertData; > + UINTN CertCount; > + UINTN Index; > + > + Status =3D GetVariable2 (Name, Guid, &Data, &Length); if (EFI_ERROR > + (Status)) { > + return EFI_NOT_FOUND; > + } > + > + CertList =3D (EFI_SIGNATURE_LIST *) Data; while ((Length > 0) && > + (Length >=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_SIGNATUR= E_LIST) - > + CertList->SignatureHeaderSize) / CertList->SignatureSize; > + > + for (Index =3D 0; Index < CertCount; Index++) { > + Status =3D AuthenticateFmpImage ( > + (EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image, > + ImageSize, > + CertData->SignatureData, > + CertList->SignatureSize - sizeof (EFI_GUID) > + ); > + if (!EFI_ERROR (Status)) > + goto Done; > + > + CertData =3D (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertL= ist- > >SignatureSize); > + } > + } > + > + Length -=3D CertList->SignatureListSize; > + CertList =3D (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + > + CertList->SignatureListSize); } > + > +Done: > + FreePool (Data); > + return Status; > +} > + > +EFI_STATUS > +CheckTheImageWithSecureBootKeys ( > + IN CONST VOID *Image, > + IN UINTN ImageSize > + ) > +{ > + EFI_STATUS Status; > + > + // PK check. > + Status =3D CheckTheImageWithSecureBootVariable( > + EFI_PLATFORM_KEY_NAME, > + &gEfiGlobalVariableGuid, > + Image, > + ImageSize > + ); > + if (!EFI_ERROR (Status) || Status =3D=3D EFI_NOT_FOUND) { > + // Return SUCCESS if verified by PK key or PK key not configured. > + DEBUG ((DEBUG_INFO, "FmpDxe: Verified capsule with PK key.\n")); > + return EFI_SUCCESS; > + } > + > + // DBX check. > + Status =3D CheckTheImageWithSecureBootVariable( > + EFI_IMAGE_SECURITY_DATABASE1, > + &gEfiImageSecurityDatabaseGuid, > + Image, > + ImageSize > + ); > + if (!EFI_ERROR (Status)) { > + DEBUG ((DEBUG_INFO, "FmpDxe: Reject capsule with DBX key.\n")); > + return EFI_SECURITY_VIOLATION; > + } > + > + // DB check. > + DEBUG ((DEBUG_INFO, "FmpDxe: Verify capsule with DB key.\n")); > + Status =3D CheckTheImageWithSecureBootVariable( > + EFI_IMAGE_SECURITY_DATABASE, > + &gEfiImageSecurityDatabaseGuid, > + Image, > + ImageSize > + ); > + return Status; > +} > + > /** > Checks if the firmware image is valid for the device. >=20 > @@ -728,6 +824,7 @@ CheckTheImage ( > UINT8 *PublicKeyDataXdrEnd; > EFI_FIRMWARE_IMAGE_DEP *Dependencies; > UINT32 DependenciesSize; > + UINT8 AllowSecureBootKeys; >=20 > Status =3D EFI_SUCCESS; > RawSize =3D 0; > @@ -782,9 +879,15 @@ CheckTheImage ( > PublicKeyDataXdr =3D PcdGetPtr (PcdFmpDevicePkcs7CertBufferXdr); > PublicKeyDataXdrEnd =3D PublicKeyDataXdr + PcdGetSize > (PcdFmpDevicePkcs7CertBufferXdr); >=20 > - if (PublicKeyDataXdr =3D=3D NULL || (PublicKeyDataXdr =3D=3D > PublicKeyDataXdrEnd)) { > - DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Invalid certificate, skipping it.= \n", > mImageIdName)); > - Status =3D EFI_ABORTED; > + if (PublicKeyDataXdr =3D=3D NULL || (PublicKeyDataXdrEnd - PublicKeyD= ataXdr > < sizeof (UINT32))) { > + AllowSecureBootKeys =3D PcdGet8 (PcdFmpDeviceAllowSecureBootKeys); > + if (AllowSecureBootKeys) { > + DEBUG ((DEBUG_INFO, "FmpDxe: Use secure boot certs.\n")); > + Status =3D CheckTheImageWithSecureBootKeys (Image, ImageSize); > + } else { > + DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Invalid certificate, skipping > it.\n", mImageIdName)); > + Status =3D EFI_ABORTED; > + } > } else { > // > // Try each key from PcdFmpDevicePkcs7CertBufferXdr diff --git > a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h > index 30754de..72a6ce6 100644 > --- a/FmpDevicePkg/FmpDxe/FmpDxe.h > +++ b/FmpDevicePkg/FmpDxe/FmpDxe.h > @@ -34,6 +34,7 @@ > #include #include > > #include > +#include > #include > #include >=20 > diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.inf > b/FmpDevicePkg/FmpDxe/FmpDxe.inf index eeb904a..60b02d4 100644 > --- a/FmpDevicePkg/FmpDxe/FmpDxe.inf > +++ b/FmpDevicePkg/FmpDxe/FmpDxe.inf > @@ -58,6 +58,8 @@ >=20 > [Guids] > gEfiEndOfDxeEventGroupGuid > + gEfiCertX509Guid > + gEfiImageSecurityDatabaseGuid >=20 > [Protocols] > gEdkiiVariableLockProtocolGuid ## CONSUMES > @@ -74,6 +76,7 @@ > gFmpDevicePkgTokenSpaceGuid.PcdFmpDevicePkcs7CertBufferXdr > ## CONSUMES > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceTestKeySha256Digest > ## CONSUMES > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceImageTypeIdGuid > ## CONSUMES > + gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceAllowSecureBootKeys > ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed = ## > SOMETIMES_PRODUCES >=20 > [Depex] > diff --git a/FmpDevicePkg/FmpDxe/FmpDxeLib.inf > b/FmpDevicePkg/FmpDxe/FmpDxeLib.inf > index 9a93b5e..1308cae 100644 > --- a/FmpDevicePkg/FmpDxe/FmpDxeLib.inf > +++ b/FmpDevicePkg/FmpDxe/FmpDxeLib.inf > @@ -74,6 +74,7 @@ > gFmpDevicePkgTokenSpaceGuid.PcdFmpDevicePkcs7CertBufferXdr > ## CONSUMES > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceTestKeySha256Digest > ## CONSUMES > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceImageTypeIdGuid > ## CONSUMES > + gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceAllowSecureBootKeys > ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed = ## > SOMETIMES_PRODUCES >=20 > [Depex] > -- > 1.8.3.1 >=20 >=20 >=20