From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR04-VI1-obe.outbound.protection.outlook.com (EUR04-VI1-obe.outbound.protection.outlook.com [40.107.8.55]) by mx.groups.io with SMTP id smtpd.web12.7884.1593488008540100138 for ; Mon, 29 Jun 2020 20:33:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@mellanox.com header.s=selector1 header.b=kOQtJ0HA; spf=pass (domain: mellanox.com, ip: 40.107.8.55, mailfrom: lsun@mellanox.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=aelglm7Zvgp5utTN9EuEOcosZmCWi5tPmA7bmjrYyix9f7XGyDCButphN+5n9jryaV1JmYYfKEzkRXGqVXWVgtfoIMnXlvzBwVqWV0Q+uSNlLncCjRD5YgUjUWsAjsANPsNAZkSOUUxCboMubR1LFPvU5wnvZdghIJPlrApAB5WlmrJsM4uZV+mNBd4E/kK4MgseVEUArwmG8rXrDhre/E544j7prWQX6ml9/akSRnMdBatjSoFlpXLMmCvQKhzglm0sjXXoKpYHAN2b8Jsy6oPZI7NndSET+ZEJvnAR6IOzOaQPaebsM7YddyShdCTmfvWqOs2rnPQ5q8fx+4KjlA== 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=+Bt3XrzdgV3b8FwiJtqt9OPoy34XJAmZ22yZqTwck9I=; b=PL9XTzLgcGj2K/2ke/qTclq0CjjRbXJC/JY4kHlazuz6HvFL2/mLWpQlHx7+2Hg+arlnN78HMwMZA1okQzf4yYeTbqS7GfD2cXKepjiBYGH7UeOP5wakvTFAqqf2Ug/UufUO58li8dSF8+R08SlcVOEojOJ+x//zCm4uXjJmqT2XjVi+6hFZVaeby2lTYMmZRULz+aD0Ppg43ieqv6KWB+HdM+9JT281m9uysfVi4OvcvyV5oeZVr61WY9CapXtjSrh581sg5QRuyYyxWix/jNkcY/cIDVRmmEWT/QKpwiaro+/KFx8P7SwfCIyOflecknPf426rDtgux0STp8Lqxw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=mellanox.com; dmarc=pass action=none header.from=mellanox.com; dkim=pass header.d=mellanox.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=+Bt3XrzdgV3b8FwiJtqt9OPoy34XJAmZ22yZqTwck9I=; b=kOQtJ0HA2xoIVvGfjF1GWwWNag2yB2Vf2Fw8kye0SgByK7bvuiCcKZBnqnd80JTg9uHfF1yRG4VJRSjVSijklu9+HDDMsLzpj7aUr8BV6c14lpT/Aa5VV8VgNNX5opWtBUdYmJHHZQkzsclKhoirgT5+wVU7aLWiEN1KvdeX3KA= Received: from DB6PR05MB3223.eurprd05.prod.outlook.com (2603:10a6:6:20::21) by DB6PR05MB3494.eurprd05.prod.outlook.com (2603:10a6:6:1e::33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3131.20; Tue, 30 Jun 2020 03:33:25 +0000 Received: from DB6PR05MB3223.eurprd05.prod.outlook.com ([fe80::3878:38d1:7487:f23a]) by DB6PR05MB3223.eurprd05.prod.outlook.com ([fe80::3878:38d1:7487:f23a%3]) with mapi id 15.20.3131.027; Tue, 30 Jun 2020 03:33:25 +0000 From: "Liming Sun" To: "Jiang, Guomin" , "devel@edk2.groups.io" , "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: AQHWRmHfzsoxRpvKo0Kc2JNYV1wdaaju+xgAgAGNuZA= Date: Tue, 30 Jun 2020 03:33:25 +0000 Message-ID: References: <5b42e8e089fb961766c639b733284413ccf03272.1592587621.git.lsun@mellanox.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=mellanox.com; x-originating-ip: [173.76.169.242] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: e23216ba-b293-490a-22aa-08d81ca658d3 x-ms-traffictypediagnostic: DB6PR05MB3494: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:538; x-forefront-prvs: 0450A714CB x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 6wVKq+WYJS/G9yuaBw4HdpcctgbyasEqa11p6i+9N/rIC+xQr4+m1bWo5iC2QF0UPPPluq4u50p9ZmTXUMMkDKomDAHc/FQkJtTB2pqn++a8ZXBxEvTc0KvFNZ5ZrDqbTyz+4V1bq797s/wu4PXixsT+knJHuH/jKcmbgQV8LnC7fyzbC22isDwQdPwlWJBX6vMYM2hTloM0InIh2aEQGTnxF30Jp7VZU90jEmQyFkusQPidm/DwR3lMPFs9+dPxlxSPVrywFi7q2bjXcRBRNK6BEPWfhHOtP71nrcZ0sRDv5aJp2UlLWEOv7ZrTqwIVOXeWv597YDkvNS5BwoRQ5oWmYXMhDydIr2zQFahsMk1lt5ZWr4M/do2rWZ9zbh5OV4xmonM0164gp4voyHnBhw== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DB6PR05MB3223.eurprd05.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(396003)(136003)(376002)(346002)(39860400002)(366004)(478600001)(52536014)(71200400001)(8676002)(2906002)(19627235002)(66946007)(83380400001)(76116006)(7696005)(66446008)(64756008)(66556008)(8936002)(6506007)(186003)(86362001)(66476007)(55016002)(15650500001)(966005)(30864003)(4326008)(316002)(45080400002)(9686003)(5660300002)(26005)(53546011)(33656002)(110136005);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata: S/7rMqocTYMlvLq20Vn0mKif6sQ7xhT4SpeuzW3H0ynj6gr/mUISRhep/K5cbINOCQUzSKlPWpafsmhEBzWxqXmBHhl9vv2n/khiYqg3BifXm0W3qNPp+XK5Uest5OlW2DAUOkkFhJubhhxMeHJDSWQ+r0r5WF+qnDc7Ru57WbIm3bh+XcYqqF6/KFpHJAuTqUXYlr005RzdQWndpJOnJRsOKWK3v3O0n8uhcaTah14XqgSPXmqSJHMnB26C/wsD4+G+/9m7yZ1yXhXEGPeGQK4S55O4XQwTmM8enrI9uL4i1gp4oc4HQffjkTeTmmWleSe5oA5oW/9RvKZYoTutOdd9LcuPNoJc/uxEnyrb+gVocuyNG/YUr7t1oXaMHj288Zr3z4W4+ezvqkmIgg4GO8RjjjNqE3P/4++PQX2drTaJCTzDsng9Rs/ijDJzBQUMlW8tWsU8jw3okFJZZsg5RPBNT+Ig85XBLMTYqcZK7MCWy2/tTKDycnI27pnSi/E3 x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DB6PR05MB3223.eurprd05.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: e23216ba-b293-490a-22aa-08d81ca658d3 X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Jun 2020 03:33:25.2136 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: C6GUcTF+83/SaBCCTXFFo7HwvqylbPGGn7NyiHzhp/xOYWdy+C1q1LHkyCTvnVjOE/Z9zvvQe5+tttEBUD4v0g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR05MB3494 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks Guomin for the comments! Below is the main scenario for the proposed change: - Device Manufacturer provides the devices with UEFI preinstalled in non-s= ecure state and no hard-coded keys ( PcdFmpDevicePkcs7CertBufferXdr). - Customer (not End-User) enrolls their own keys in trusted environment be= fore delivering to End User. This capsule approach can be used for large deployment without involving a= ny private keys. Yes, I do agree that once it's delivered to End User it won't be considere= d secure. Thanks, Liming > -----Original Message----- > From: Jiang, Guomin > Sent: Sunday, June 28, 2020 11:18 PM > To: devel@edk2.groups.io; Liming Sun ; Xu, Wei6 ; Gao, Liming ; > Kinney, Michael D > Cc: Sean Brogan > Subject: RE: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verifica= tion with secure boot keys >=20 > I think it have some vulnerability, the case as below. >=20 > 1. Untrusted End User enroll the new DB key -> sign the untrusted device= firmware -> flash the untrusted device firmware -> the > system will become unsafe. >=20 > I think the end user is untrusted and we need to make sure only few pers= on can have the privilege. >=20 > Best Regards > Guomin >=20 > > -----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 verificati= on > > with secure boot keys > > > > 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; > > > > One purpose for this change is to auto-deploy the UEFI secure boot key= s > > with UEFI capsule. Initially it's done in trusted environment. > > Once secure boot is enabled, the same keys will be used to verify the = signed > > capsules as well for further updates. > > > > 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(-) > > > > 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 > > > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceImageTypeIdGuid|{0}|VOID > > *|0x40000010 > > > > + ## 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 = capsule > > # update image. Encoded using the Variable-Length Opaque Data for= mat > > 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; > > } > > > > +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_ERRO= R > > + (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_SIGNAT= URE_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 + Cer= tList- > > >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. > > > > @@ -728,6 +824,7 @@ CheckTheImage ( > > UINT8 *PublicKeyDataXdrEnd; > > EFI_FIRMWARE_IMAGE_DEP *Dependencies; > > UINT32 DependenciesSize; > > + UINT8 AllowSecureBootKeys; > > > > Status =3D EFI_SUCCESS; > > RawSize =3D 0; > > @@ -782,9 +879,15 @@ CheckTheImage ( > > PublicKeyDataXdr =3D PcdGetPtr (PcdFmpDevicePkcs7CertBufferXdr); > > PublicKeyDataXdrEnd =3D PublicKeyDataXdr + PcdGetSize > > (PcdFmpDevicePkcs7CertBufferXdr); > > > > - if (PublicKeyDataXdr =3D=3D NULL || (PublicKeyDataXdr =3D=3D > > PublicKeyDataXdrEnd)) { > > - DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Invalid certificate, skipping i= t.\n", > > mImageIdName)); > > - Status =3D EFI_ABORTED; > > + if (PublicKeyDataXdr =3D=3D NULL || (PublicKeyDataXdrEnd - PublicKe= yDataXdr > > < 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 > > > > 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 @@ > > > > [Guids] > > gEfiEndOfDxeEventGroupGuid > > + gEfiCertX509Guid > > + gEfiImageSecurityDatabaseGuid > > > > [Protocols] > > gEdkiiVariableLockProtocolGuid ## CONSUMES > > @@ -74,6 +76,7 @@ > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDevicePkcs7CertBufferXdr > > ## CONSUMES > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceTestKeySha256Digest > > ## CONSUMES > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceImageTypeIdGuid > > ## CONSUMES > > + gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceAllowSecureBootKeys > > ## CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed = ## > > SOMETIMES_PRODUCES > > > > [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 > > > > [Depex] > > -- > > 1.8.3.1 > > > > > >=20