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.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 (#61871): https://edk2.groups.io/g/devel/message/61871 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] -=-=-=-=-=-=-=-=-=-=-=-