Thanks Guomin for the comments!
Below is the main scenario for the proposed change:
- Device Manufacturer provides the devices with UEFI preinstalled in non-secure
state and no hard-coded keys ( PcdFmpDevicePkcs7CertBufferXdr).
- Customer (not End-User) enrolls their own keys in trusted environment before
delivering to End User.
This capsule approach can be used for large deployment without involving any
private keys.
Yes, I do agree that once it's delivered to End User it won't be considered
secure.
Thanks,
Liming
> -----Original Message-----
> From: Jiang, Guomin <guomin.ji...@intel.com>
> Sent: Sunday, June 28, 2020 11:18 PM
> To: devel@edk2.groups.io; Liming Sun <l...@mellanox.com>; Xu, Wei6
> <wei6...@intel.com>; Gao, Liming <liming....@intel.com>;
> Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Sean Brogan <sean.bro...@microsoft.com>
> Subject: RE: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification
> with secure boot keys
>
> 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...@intel.com>; Gao, Liming <liming....@intel.com>;
> > Kinney, Michael D <michael.d.kin...@intel.com>
> > Cc: Liming Sun <l...@mellanox.com>; devel@edk2.groups.io; Sean Brogan
> > <sean.bro...@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 <l...@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
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61821): https://edk2.groups.io/g/devel/message/61821
Mute This Topic: https://groups.io/mt/74985160/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
