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

["Guomin Jiang" <guomin.jiang@intel.com>]

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