* [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
@ 2020-06-19 17:48 Liming Sun
2020-06-29 3:18 ` [edk2-devel] " Guomin Jiang
0 siblings, 1 reply; 11+ messages in thread
From: Liming Sun @ 2020-06-19 17:48 UTC (permalink / raw)
To: Wei6 Xu, Liming Gao, Michael D Kinney; +Cc: Liming Sun, devel, Sean Brogan
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-06-19 17:48 [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys Liming Sun
@ 2020-06-29 3:18 ` Guomin Jiang
2020-06-30 3:33 ` Liming Sun
0 siblings, 1 reply; 11+ messages in thread
From: Guomin Jiang @ 2020-06-29 3:18 UTC (permalink / raw)
To: devel@edk2.groups.io, lsun@mellanox.com, Xu, Wei6, Gao, Liming,
Kinney, Michael D
Cc: Sean Brogan
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
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-06-29 3:18 ` [edk2-devel] " Guomin Jiang
@ 2020-06-30 3:33 ` Liming Sun
2020-06-30 7:32 ` Guomin Jiang
0 siblings, 1 reply; 11+ messages in thread
From: Liming Sun @ 2020-06-30 3:33 UTC (permalink / raw)
To: Jiang, Guomin, devel@edk2.groups.io, Xu, Wei6, Gao, Liming,
Kinney, Michael D
Cc: Sean Brogan
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.jiang@intel.com>
> Sent: Sunday, June 28, 2020 11:18 PM
> To: devel@edk2.groups.io; Liming Sun <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
>
> 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
> >
> >
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-06-30 3:33 ` Liming Sun
@ 2020-06-30 7:32 ` Guomin Jiang
2020-06-30 12:47 ` Liming Sun
0 siblings, 1 reply; 11+ messages in thread
From: Guomin Jiang @ 2020-06-30 7:32 UTC (permalink / raw)
To: devel@edk2.groups.io, lsun@mellanox.com, Xu, Wei6, Gao, Liming,
Kinney, Michael D
Cc: Sean Brogan
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.jiang@intel.com>; devel@edk2.groups.io; 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
>
> 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.jiang@intel.com>
> > Sent: Sunday, June 28, 2020 11:18 PM
> > To: devel@edk2.groups.io; Liming Sun <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
> >
> > 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
> > >
> > >
> > >
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-06-30 7:32 ` Guomin Jiang
@ 2020-06-30 12:47 ` Liming Sun
2020-07-01 0:56 ` Guomin Jiang
0 siblings, 1 reply; 11+ messages in thread
From: Liming Sun @ 2020-06-30 12:47 UTC (permalink / raw)
To: devel@edk2.groups.io, guomin.jiang@intel.com, Xu, Wei6,
Gao, Liming, Kinney, Michael D
Cc: Sean Brogan
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 <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
>
> 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.jiang@intel.com>; devel@edk2.groups.io; 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
> >
> > 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.jiang@intel.com>
> > > Sent: Sunday, June 28, 2020 11:18 PM
> > > To: devel@edk2.groups.io; Liming Sun <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
> > >
> > > 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
> > > >
> > > >
> > > >
> >
> >
> >
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-06-30 12:47 ` Liming Sun
@ 2020-07-01 0:56 ` Guomin Jiang
2020-07-01 16:26 ` Liming Sun
0 siblings, 1 reply; 11+ messages in thread
From: Guomin Jiang @ 2020-07-01 0:56 UTC (permalink / raw)
To: Liming Sun, devel@edk2.groups.io, Xu, Wei6, Gao, Liming,
Kinney, Michael D
Cc: Sean Brogan
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 <lsun@mellanox.com>
> Sent: Tuesday, June 30, 2020 8:47 PM
> To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.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
>
> 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 <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
> >
> > 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.jiang@intel.com>; devel@edk2.groups.io;
> > > 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
> > >
> > > 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.jiang@intel.com>
> > > > Sent: Sunday, June 28, 2020 11:18 PM
> > > > To: devel@edk2.groups.io; Liming Sun <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
> > > >
> > > > 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
> > > > >
> > > > >
> > > > >
> > >
> > >
> > >
> >
> >
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-07-01 0:56 ` Guomin Jiang
@ 2020-07-01 16:26 ` Liming Sun
2020-07-01 17:42 ` Michael D Kinney
0 siblings, 1 reply; 11+ messages in thread
From: Liming Sun @ 2020-07-01 16:26 UTC (permalink / raw)
To: Jiang, Guomin, devel@edk2.groups.io, Xu, Wei6, Gao, Liming,
Kinney, Michael D
Cc: Sean Brogan
>> 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.jiang@intel.com>
> Sent: Tuesday, June 30, 2020 8:56 PM
> To: Liming Sun <lsun@mellanox.com>; devel@edk2.groups.io; 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
>
> 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 <lsun@mellanox.com>
> > Sent: Tuesday, June 30, 2020 8:47 PM
> > To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.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
> >
> > 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 <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
> > >
> > > 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.jiang@intel.com>; devel@edk2.groups.io;
> > > > 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
> > > >
> > > > 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.jiang@intel.com>
> > > > > Sent: Sunday, June 28, 2020 11:18 PM
> > > > > To: devel@edk2.groups.io; Liming Sun <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
> > > > >
> > > > > 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
> > > > > >
> > > > > >
> > > > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-07-01 16:26 ` Liming Sun
@ 2020-07-01 17:42 ` Michael D Kinney
2020-07-06 20:58 ` Liming Sun
0 siblings, 1 reply; 11+ messages in thread
From: Michael D Kinney @ 2020-07-01 17:42 UTC (permalink / raw)
To: devel@edk2.groups.io, lsun@mellanox.com, Jiang, Guomin, Xu, Wei6,
Gao, Liming, Kinney, Michael D
Cc: Sean Brogan
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.jiang@intel.com>;
> devel@edk2.groups.io; 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
>
> >> 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.jiang@intel.com>
> > Sent: Tuesday, June 30, 2020 8:56 PM
> > To: Liming Sun <lsun@mellanox.com>;
> devel@edk2.groups.io; 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
> >
> > 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 <lsun@mellanox.com>
> > > Sent: Tuesday, June 30, 2020 8:47 PM
> > > To: devel@edk2.groups.io; Jiang, Guomin
> <guomin.jiang@intel.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
> > >
> > > 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
> <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
> > > >
> > > > 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.jiang@intel.com>;
> devel@edk2.groups.io;
> > > > > 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
> > > > >
> > > > > 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.jiang@intel.com>
> > > > > > Sent: Sunday, June 28, 2020 11:18 PM
> > > > > > To: devel@edk2.groups.io; Liming Sun
> <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
> > > > > >
> > > > > > 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.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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-07-01 17:42 ` Michael D Kinney
@ 2020-07-06 20:58 ` Liming Sun
2020-07-07 15:42 ` Michael D Kinney
0 siblings, 1 reply; 11+ messages in thread
From: Liming Sun @ 2020-07-06 20:58 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io, Jiang, Guomin, Xu, Wei6,
Gao, Liming
Cc: Sean Brogan
Thanks Michael. Below is the use case:
- Device vendor provides devices with UEFI preinstalled;
- Customer gets the device in non-secure-boot mode by default, and would like to enroll the secure boot keys themselves in some automatic way (such as using capsule).
PcdFmpDevicePkcs7CertBufferXdr is not used for two reasons for this use case:
1. Simplicity. So vendor doesn't need to be involved in the key management, and customer could create and sign the capsule themselves.
2. Secure reasons. Once customer fully own the device and put it into secure-boot mode, even the capsule from the device vendor couldn't be applied without being signed by customer. (The hardcoded PcdFmpDevicePkcs7CertBufferXdr couldn't achieve this goal).
Thanks,
Liming
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, July 1, 2020 1:43 PM
> To: devel@edk2.groups.io; Liming Sun <lsun@mellanox.com>; Jiang, Guomin <guomin.jiang@intel.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
>
> 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.jiang@intel.com>;
> > devel@edk2.groups.io; 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
> >
> > >> 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.jiang@intel.com>
> > > Sent: Tuesday, June 30, 2020 8:56 PM
> > > To: Liming Sun <lsun@mellanox.com>;
> > devel@edk2.groups.io; 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
> > >
> > > 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 <lsun@mellanox.com>
> > > > Sent: Tuesday, June 30, 2020 8:47 PM
> > > > To: devel@edk2.groups.io; Jiang, Guomin
> > <guomin.jiang@intel.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
> > > >
> > > > 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
> > <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
> > > > >
> > > > > 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.jiang@intel.com>;
> > devel@edk2.groups.io;
> > > > > > 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
> > > > > >
> > > > > > 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.jiang@intel.com>
> > > > > > > Sent: Sunday, June 28, 2020 11:18 PM
> > > > > > > To: devel@edk2.groups.io; Liming Sun
> > <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
> > > > > > >
> > > > > > > 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.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
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> >
> >
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-07-06 20:58 ` Liming Sun
@ 2020-07-07 15:42 ` Michael D Kinney
2020-07-22 0:37 ` Guomin Jiang
0 siblings, 1 reply; 11+ messages in thread
From: Michael D Kinney @ 2020-07-07 15:42 UTC (permalink / raw)
To: devel@edk2.groups.io, lsun@mellanox.com, Jiang, Guomin, Xu, Wei6,
Gao, Liming, Ni, Ray, Zimmer, Vincent, Rothman, Michael A,
Kinney, Michael D
Cc: Sean Brogan
Hi Liming Sun,
Thank you for providing the additional details.
The use case description is very brief and appears that it
may not follow some of the UEFI Specification requirements.
I want to make sure we have a clear understanding of the use
cases with some of the UEFI Secure Boot and Firmware Updates
experts.
We have a TianoCore design meeting that is hosted by Ray Ni.
Can you please work with Ray to get onto the agenda for
that meeting where you can present your ideas?
Thanks,
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Liming Sun
> Sent: Monday, July 6, 2020 1:59 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io; Jiang, Guomin
> <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>;
> Gao, Liming <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance
> capsule verification with secure boot keys
>
> Thanks Michael. Below is the use case:
>
> - Device vendor provides devices with UEFI preinstalled;
> - Customer gets the device in non-secure-boot mode by
> default, and would like to enroll the secure boot keys
> themselves in some automatic way (such as using
> capsule).
>
> PcdFmpDevicePkcs7CertBufferXdr is not used for two
> reasons for this use case:
> 1. Simplicity. So vendor doesn't need to be involved in
> the key management, and customer could create and sign
> the capsule themselves.
> 2. Secure reasons. Once customer fully own the device
> and put it into secure-boot mode, even the capsule from
> the device vendor couldn't be applied without being
> signed by customer. (The hardcoded
> PcdFmpDevicePkcs7CertBufferXdr couldn't achieve this
> goal).
>
> Thanks,
> Liming
>
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Wednesday, July 1, 2020 1:43 PM
> > To: devel@edk2.groups.io; Liming Sun
> <lsun@mellanox.com>; Jiang, Guomin
> <guomin.jiang@intel.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
> >
> > 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.jiang@intel.com>;
> > > devel@edk2.groups.io; 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
> > >
> > > >> 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.jiang@intel.com>
> > > > Sent: Tuesday, June 30, 2020 8:56 PM
> > > > To: Liming Sun <lsun@mellanox.com>;
> > > devel@edk2.groups.io; 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
> > > >
> > > > 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 <lsun@mellanox.com>
> > > > > Sent: Tuesday, June 30, 2020 8:47 PM
> > > > > To: devel@edk2.groups.io; Jiang, Guomin
> > > <guomin.jiang@intel.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
> > > > >
> > > > > 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
> > > <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
> > > > > >
> > > > > > 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.jiang@intel.com>;
> > > devel@edk2.groups.io;
> > > > > > > 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
> > > > > > >
> > > > > > > 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.jiang@intel.com>
> > > > > > > > Sent: Sunday, June 28, 2020 11:18 PM
> > > > > > > > To: devel@edk2.groups.io; Liming Sun
> > > <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
> > > > > > > >
> > > > > > > > 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.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
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > >
> > >
> > >
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys
2020-07-07 15:42 ` Michael D Kinney
@ 2020-07-22 0:37 ` Guomin Jiang
0 siblings, 0 replies; 11+ messages in thread
From: Guomin Jiang @ 2020-07-22 0:37 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io, lsun@mellanox.com,
Xu, Wei6, Gao, Liming, Ni, Ray, Zimmer, Vincent,
Rothman, Michael A
Cc: Sean Brogan
The topic is done or dropped?
Any status update or decision making?
Thanks
Guomin
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Tuesday, July 7, 2020 11:42 PM
> To: devel@edk2.groups.io; lsun@mellanox.com; Jiang, Guomin
> <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Zimmer, Vincent
> <vincent.zimmer@intel.com>; Rothman, Michael A
> <michael.a.rothman@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
>
> Hi Liming Sun,
>
> Thank you for providing the additional details.
>
> The use case description is very brief and appears that it may not follow
> some of the UEFI Specification requirements.
> I want to make sure we have a clear understanding of the use cases with
> some of the UEFI Secure Boot and Firmware Updates experts.
>
> We have a TianoCore design meeting that is hosted by Ray Ni.
>
> Can you please work with Ray to get onto the agenda for that meeting where
> you can present your ideas?
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming
> > Sun
> > Sent: Monday, July 6, 2020 1:59 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@intel.com>; Xu,
> Wei6
> > <wei6.xu@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Subject: Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule
> > verification with secure boot keys
> >
> > Thanks Michael. Below is the use case:
> >
> > - Device vendor provides devices with UEFI preinstalled;
> > - Customer gets the device in non-secure-boot mode by default, and
> > would like to enroll the secure boot keys themselves in some automatic
> > way (such as using capsule).
> >
> > PcdFmpDevicePkcs7CertBufferXdr is not used for two reasons for this
> > use case:
> > 1. Simplicity. So vendor doesn't need to be involved in the key
> > management, and customer could create and sign the capsule themselves.
> > 2. Secure reasons. Once customer fully own the device and put it into
> > secure-boot mode, even the capsule from the device vendor couldn't be
> > applied without being signed by customer. (The hardcoded
> > PcdFmpDevicePkcs7CertBufferXdr couldn't achieve this goal).
> >
> > Thanks,
> > Liming
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Sent: Wednesday, July 1, 2020 1:43 PM
> > > To: devel@edk2.groups.io; Liming Sun
> > <lsun@mellanox.com>; Jiang, Guomin
> > <guomin.jiang@intel.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
> > >
> > > 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.jiang@intel.com>; devel@edk2.groups.io;
> > > > 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
> > > >
> > > > >> 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.jiang@intel.com>
> > > > > Sent: Tuesday, June 30, 2020 8:56 PM
> > > > > To: Liming Sun <lsun@mellanox.com>;
> > > > devel@edk2.groups.io; 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
> > > > >
> > > > > 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 <lsun@mellanox.com>
> > > > > > Sent: Tuesday, June 30, 2020 8:47 PM
> > > > > > To: devel@edk2.groups.io; Jiang, Guomin
> > > > <guomin.jiang@intel.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
> > > > > >
> > > > > > 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
> > > > <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
> > > > > > >
> > > > > > > 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.jiang@intel.com>;
> > > > devel@edk2.groups.io;
> > > > > > > > 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
> > > > > > > >
> > > > > > > > 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.jiang@intel.com>
> > > > > > > > > Sent: Sunday, June 28, 2020 11:18 PM
> > > > > > > > > To: devel@edk2.groups.io; Liming Sun
> > > > <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
> > > > > > > > >
> > > > > > > > > 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.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
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > >
> > > >
> > > >
> >
> >
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-07-22 0:38 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-19 17:48 [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys Liming Sun
2020-06-29 3:18 ` [edk2-devel] " Guomin Jiang
2020-06-30 3:33 ` 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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox