Re: [edk2-devel] [PATCH] FmpDevicePkg: Enhance capsule verification with secure boot keys

["Liming Sun" <lsun@mellanox.com>]

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