Liming Sun, Can you explain why you cannot use PcdFmpDevicePkcs7CertBufferXdr for your use case? I want to understand the use case to see if that feature can be applied or if a minor enhancement to this feature can work.
Using the UEFI Secure Boot DB for anything other than authentication of UEFI boot loaders is not recommended. Thanks, Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On > Behalf Of Liming Sun > Sent: Wednesday, July 1, 2020 9:27 AM > To: Jiang, Guomin <guomin.ji...@intel.com>; > devel@edk2.groups.io; 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 > > >> But if your customer indeed want it, you can add it > to your customization code. > Thanks. Yes, this is a behavior customer expects. This > change just tries to provide a handy way to enroll > initial keys. > So the initial keys could be carried in the capsule > itself. > It also has "PcdFmpDeviceAllowSecureBootKeys" disabled > by default, so it behaves the same as before. > > We'll try to use customization code instead as > suggested. > > Thanks, > Liming > > > -----Original Message----- > > From: Jiang, Guomin <guomin.ji...@intel.com> > > Sent: Tuesday, June 30, 2020 8:56 PM > > To: Liming Sun <l...@mellanox.com>; > devel@edk2.groups.io; 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 want to ask your one question: are you sure that > every mother board which deliver to customer will enable > the secure boot mode? > > > > I just emphasize that I want to make sure that the > device firmware come from the device vendor. > > > > Thanks for your effort, the patch is good, I just > think it is not suitable for common solution. > > > > But if your customer indeed want it, you can add it to > your customization code. > > > > Thanks > > Guomin > > > > > -----Original Message----- > > > From: Liming Sun <l...@mellanox.com> > > > Sent: Tuesday, June 30, 2020 8:47 PM > > > To: devel@edk2.groups.io; Jiang, Guomin > <guomin.ji...@intel.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 > > > > > > Thanks Guomin. > > > > > > I still have one question. Let's assume we're the > device vendor and we let > > > customer to enroll their keys. Once the keys are > enrolled, the device will be > > > in secure boot mode. Are you saying that the end > user could "have the ability > > > to enroll their DB without too many effort" even > after the secure boot has > > > been enabled already? > > > > > > Please correct me if I misunderstood it. > > > > > > - Liming > > > > > > > -----Original Message----- > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> > On Behalf Of > > > Guomin > > > > Jiang via groups.io > > > > Sent: Tuesday, June 30, 2020 3:33 AM > > > > 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 > > > > > > > > Liming, > > > > > > > > The end user have the ability to enroll their DB > without too many effort. > > > > > > > > And I think some end user also have the ability to > get insecure firmware > > > which not from the device vendor. > > > > > > > > I suggest that tell the device vendor that it is > critical that set the > > > PcdFmpDevicePkcs7CertBufferXdr rather than decrease > the security. > > > > > > > > Best Regards > > > > Guomin > > > > > > > > > -----Original Message----- > > > > > From: devel@edk2.groups.io > <devel@edk2.groups.io> On Behalf Of > > > > > Liming Sun > > > > > Sent: Tuesday, June 30, 2020 11:33 AM > > > > > To: Jiang, Guomin <guomin.ji...@intel.com>; > devel@edk2.groups.io; > > > > > 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 > > > > > > > > > > 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.PcdFmpDeviceAllowSecureBootK > eys|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.PcdFmpDevicePkcs7CertBufferX > dr > > > > > > > ## CONSUMES > > > > > > > > > > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceTestKeySha256Dig > est > > > > > > > ## CONSUMES > > > > > > > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceImageTypeIdGuid > > > > > > > ## CONSUMES > > > > > > > + > > > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceAllowSecureBootK > eys > > > > > > > ## 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.PcdFmpDevicePkcs7CertBufferX > dr > > > > > > > ## CONSUMES > > > > > > > > > > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceTestKeySha256Dig > est > > > > > > > ## CONSUMES > > > > > > > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceImageTypeIdGuid > > > > > > > ## CONSUMES > > > > > > > + > > > > gFmpDevicePkgTokenSpaceGuid.PcdFmpDeviceAllowSecureBootK > eys > > > > > > > ## CONSUMES > > > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed > > > ## > > > > > > > SOMETIMES_PRODUCES > > > > > > > > > > > > > > [Depex] > > > > > > > -- > > > > > > > 1.8.3.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61919): https://edk2.groups.io/g/devel/message/61919 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] -=-=-=-=-=-=-=-=-=-=-=-