Topics

[PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys

Liming Sun
 

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@...>
---
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

Guomin Jiang
 

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@...>; Gao, Liming <liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Liming Sun <lsun@...>; devel@edk2.groups.io; Sean Brogan
<sean.brogan@...>
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@...>
---
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


Liming Sun
 

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@...>
Sent: Sunday, June 28, 2020 11:18 PM
To: devel@edk2.groups.io; Liming Sun <lsun@...>; Xu, Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Gao, Liming <liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Liming Sun <lsun@...>; devel@edk2.groups.io; Sean Brogan
<sean.brogan@...>
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@...>
---
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


Guomin Jiang
 

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@...>; devel@edk2.groups.io; Xu,
Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>
Sent: Sunday, June 28, 2020 11:18 PM
To: devel@edk2.groups.io; Liming Sun <lsun@...>; Xu, Wei6
<wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Gao, Liming
<liming.gao@...>; Kinney, Michael D
<michael.d.kinney@...>
Cc: Liming Sun <lsun@...>; devel@edk2.groups.io; Sean
Brogan <sean.brogan@...>
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@...>
---
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



Liming Sun
 

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@...>; Xu, Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; devel@edk2.groups.io; Xu,
Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>
Sent: Sunday, June 28, 2020 11:18 PM
To: devel@edk2.groups.io; Liming Sun <lsun@...>; Xu, Wei6
<wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Gao, Liming
<liming.gao@...>; Kinney, Michael D
<michael.d.kinney@...>
Cc: Liming Sun <lsun@...>; devel@edk2.groups.io; Sean
Brogan <sean.brogan@...>
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@...>
---
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




Guomin Jiang
 

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@...>
Sent: Tuesday, June 30, 2020 8:47 PM
To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@...>; Xu,
Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Xu, Wei6
<wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; devel@edk2.groups.io;
Xu,
Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>
Sent: Sunday, June 28, 2020 11:18 PM
To: devel@edk2.groups.io; Liming Sun <lsun@...>; Xu, Wei6
<wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Gao, Liming
<liming.gao@...>; Kinney, Michael D
<michael.d.kinney@...>
Cc: Liming Sun <lsun@...>; devel@edk2.groups.io; Sean
Brogan <sean.brogan@...>
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@...>
---
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




Liming Sun
 

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@...>
Sent: Tuesday, June 30, 2020 8:56 PM
To: Liming Sun <lsun@...>; devel@edk2.groups.io; Xu, Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>
Sent: Tuesday, June 30, 2020 8:47 PM
To: devel@edk2.groups.io; Jiang, Guomin <guomin.jiang@...>; Xu,
Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Xu, Wei6
<wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; devel@edk2.groups.io;
Xu,
Wei6 <wei6.xu@...>; Gao, Liming <liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>
Sent: Sunday, June 28, 2020 11:18 PM
To: devel@edk2.groups.io; Liming Sun <lsun@...>; Xu, Wei6
<wei6.xu@...>; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Gao, Liming
<liming.gao@...>; Kinney, Michael D
<michael.d.kinney@...>
Cc: Liming Sun <lsun@...>; devel@edk2.groups.io; Sean
Brogan <sean.brogan@...>
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@...>
---
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




Michael D Kinney
 

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@...>;
devel@edk2.groups.io; Xu, Wei6 <wei6.xu@...>; Gao,
Liming <liming.gao@...>; Kinney, Michael D
<michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>
Sent: Tuesday, June 30, 2020 8:56 PM
To: Liming Sun <lsun@...>;
devel@edk2.groups.io; Xu, Wei6 <wei6.xu@...>; Gao,
Liming <liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>
Sent: Tuesday, June 30, 2020 8:47 PM
To: devel@edk2.groups.io; Jiang, Guomin
<guomin.jiang@...>; Xu,
Wei6 <wei6.xu@...>; Gao, Liming
<liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Xu, Wei6
<wei6.xu@...>; Gao, Liming
<liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>;
devel@edk2.groups.io;
Xu,
Wei6 <wei6.xu@...>; Gao, Liming
<liming.gao@...>;
Kinney, Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>
Sent: Sunday, June 28, 2020 11:18 PM
To: devel@edk2.groups.io; Liming Sun
<lsun@...>; Xu, Wei6
<wei6.xu@...>; Gao, Liming
<liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Cc: Sean Brogan <sean.brogan@...>
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@...>; Gao,
Liming
<liming.gao@...>; Kinney, Michael D
<michael.d.kinney@...>
Cc: Liming Sun <lsun@...>;
devel@edk2.groups.io; Sean
Brogan <sean.brogan@...>
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@...>
---
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