From: "Guomin Jiang" <guomin.jiang@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"lsun@mellanox.com" <lsun@mellanox.com>,
"Xu, Wei6" <wei6.xu@intel.com>,
"Gao, Liming" <liming.gao@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Subject: Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
Date: Mon, 29 Jun 2020 03:18:16 +0000 [thread overview]
Message-ID: <DM6PR11MB29550F772DA91E4F37DCD16A9D6E0@DM6PR11MB2955.namprd11.prod.outlook.com> (raw)
In-Reply-To: <5b42e8e089fb961766c639b733284413ccf03272.1592587621.git.lsun@mellanox.com>
I think it have some vulnerability, the case as below.
1. Untrusted End User enroll the new DB key -> sign the untrusted device firmware -> flash the untrusted device firmware -> the system will become unsafe.
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 <devel@edk2.groups.io> On Behalf Of Liming
> Sun
> Sent: Saturday, June 20, 2020 1:48 AM
> To: Xu, Wei6 <wei6.xu@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Liming Sun <lsun@mellanox.com>; devel@edk2.groups.io; Sean Brogan
> <sean.brogan@microsoft.com>
> Subject: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification
> 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 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 signed
> capsules as well for further updates.
>
> Signed-off-by: Liming Sun <lsun@mellanox.com>
> ---
> 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 format
> 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 = GetVariable2 (Name, Guid, &Data, &Length); if (EFI_ERROR
> + (Status)) {
> + return EFI_NOT_FOUND;
> + }
> +
> + CertList = (EFI_SIGNATURE_LIST *) Data; while ((Length > 0) &&
> + (Length >= CertList->SignatureListSize)) {
> + if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
> + CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList +
> + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
> + CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) -
> + CertList->SignatureHeaderSize) / CertList->SignatureSize;
> +
> + for (Index = 0; Index < CertCount; Index++) {
> + Status = AuthenticateFmpImage (
> + (EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image,
> + ImageSize,
> + CertData->SignatureData,
> + CertList->SignatureSize - sizeof (EFI_GUID)
> + );
> + if (!EFI_ERROR (Status))
> + goto Done;
> +
> + CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList-
> >SignatureSize);
> + }
> + }
> +
> + Length -= CertList->SignatureListSize;
> + CertList = (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 = CheckTheImageWithSecureBootVariable(
> + EFI_PLATFORM_KEY_NAME,
> + &gEfiGlobalVariableGuid,
> + Image,
> + ImageSize
> + );
> + if (!EFI_ERROR (Status) || Status == 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 = 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 = 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 = EFI_SUCCESS;
> RawSize = 0;
> @@ -782,9 +879,15 @@ CheckTheImage (
> PublicKeyDataXdr = PcdGetPtr (PcdFmpDevicePkcs7CertBufferXdr);
> PublicKeyDataXdrEnd = PublicKeyDataXdr + PcdGetSize
> (PcdFmpDevicePkcs7CertBufferXdr);
>
> - if (PublicKeyDataXdr == NULL || (PublicKeyDataXdr ==
> PublicKeyDataXdrEnd)) {
> - DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Invalid certificate, skipping it.\n",
> mImageIdName));
> - Status = EFI_ABORTED;
> + if (PublicKeyDataXdr == NULL || (PublicKeyDataXdrEnd - PublicKeyDataXdr
> < sizeof (UINT32))) {
> + AllowSecureBootKeys = PcdGet8 (PcdFmpDeviceAllowSecureBootKeys);
> + if (AllowSecureBootKeys) {
> + DEBUG ((DEBUG_INFO, "FmpDxe: Use secure boot certs.\n"));
> + Status = CheckTheImageWithSecureBootKeys (Image, ImageSize);
> + } else {
> + DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Invalid certificate, skipping
> it.\n", mImageIdName));
> + Status = 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 <Protocol/FirmwareManagement.h> #include
> <Protocol/FirmwareManagementProgress.h>
> #include <Protocol/VariableLock.h>
> +#include <Guid/ImageAuthentication.h>
> #include <Guid/SystemResourceTable.h>
> #include <Guid/EventGroup.h>
>
> 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
>
>
>
next prev parent reply other threads:[~2020-06-29 3:18 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-19 17:48 [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys Liming Sun
2020-06-29 3:18 ` Guomin Jiang [this message]
2020-06-30 3:33 ` [edk2-devel] " Liming Sun
2020-06-30 7:32 ` Guomin Jiang
2020-06-30 12:47 ` Liming Sun
2020-07-01 0:56 ` Guomin Jiang
2020-07-01 16:26 ` Liming Sun
2020-07-01 17:42 ` Michael D Kinney
2020-07-06 20:58 ` Liming Sun
2020-07-07 15:42 ` Michael D Kinney
2020-07-22 0:37 ` Guomin Jiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM6PR11MB29550F772DA91E4F37DCD16A9D6E0@DM6PR11MB2955.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox