[PATCH 6/8] efi: capsule: Add support for uefi capsule authentication

[Akashi Takahiro <takahiro.akashi@linaro.org>]

On Sun, May 10, 2020 at 04:56:21PM +0530, Sughosh Ganu wrote:
> On Fri, 8 May 2020 at 06:12, Akashi Takahiro <takahiro.akashi@linaro.org>
> wrote:
> 
> > On Thu, May 07, 2020 at 05:20:35PM +0530, Sughosh Ganu wrote:
> > > On Thu, 7 May 2020 at 13:49, Akashi Takahiro <takahiro.akashi@linaro.org
> > >
> > > wrote:
> > >
> > > > Sughosh,
> > > >
> > > > On Thu, Apr 30, 2020 at 11:06:28PM +0530, Sughosh Ganu wrote:
> > > > > Add support for authenticating uefi capsules. Most of the signature
> > > > > verification functionality is shared with the uefi secure boot
> > > > > feature.
> > > > >
> > > > > The root certificate containing the public key used for the signature
> > > > > verification is stored as an efi variable, similar to the variables
> > > > > used for uefi secure boot. The root certificate is stored as an efi
> > > > > signature list(esl) file -- this file contains the x509 certificate
> > > > > which is the root certificate.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >  include/efi_api.h              |  17 +++++
> > > > >  include/efi_loader.h           |   8 ++-
> > > > >  lib/efi_loader/Kconfig         |  16 +++++
> > > > >  lib/efi_loader/efi_capsule.c   | 126
> > +++++++++++++++++++++++++++++++++
> > > > >  lib/efi_loader/efi_signature.c |   4 +-
> > > > >  5 files changed, 167 insertions(+), 4 deletions(-)
> > > > >
> > > >
> > >
> > > <snip>
> > >
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > index ec2976ceba..245deabbed 100644
> > > > > --- a/lib/efi_loader/Kconfig
> > > > > +++ b/lib/efi_loader/Kconfig
> > > > > @@ -110,6 +110,22 @@ config EFI_CAPSULE_FIT_DEVICE
> > > > >       help
> > > > >         Define storage device, say 1:1, for storing FIT image
> > > > >
> > > > > +config EFI_CAPSULE_AUTHENTICATE
> > > > > +     bool "Update Capsule authentication"
> > > > > +     depends on EFI_HAVE_CAPSULE_SUPPORT
> > > > > +     depends on EFI_CAPSULE_ON_DISK
> > > > > +     depends on EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> > > >
> > > > Do we need those two configurations?
> > > >
> > >
> > > Right, I think we can just do with the EFI_CAPSULE_ON_DISK. Will change.
> >
> > Actually I don't think we need EFI_CAPSULE_ON_DISK neither.
> >
> 
> We at least need EFI_HAVE_CAPSULE_SUPPORT, isn't it.

Ah, yes of course.

-Takahiro Akashi
> 
> >
> > >
> > > >
> > > > > +     select SHA256
> > > > > +     select RSA
> > > > > +     select RSA_VERIFY
> > > > > +     select RSA_VERIFY_WITH_PKEY
> > > > > +     select X509_CERTIFICATE_PARSER
> > > > > +     select PKCS7_MESSAGE_PARSER
> > > > > +     default n
> > > > > +     help
> > > > > +       Select this option if you want to enable capsule
> > > > > +       authentication
> > > > > +
> > > > >  config EFI_DEVICE_PATH_TO_TEXT
> > > > >       bool "Device path to text protocol"
> > > > >       default y
> > > > > diff --git a/lib/efi_loader/efi_capsule.c
> > b/lib/efi_loader/efi_capsule.c
> > > > > index 931d363edc..a265d36ff0 100644
> > > > > --- a/lib/efi_loader/efi_capsule.c
> > > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > > @@ -12,6 +12,10 @@
> > > > >  #include <malloc.h>
> > > > >  #include <mapmem.h>
> > > > >  #include <sort.h>
> > > > > +#include "../lib/crypto/pkcs7_parser.h"
> > > > > +
> > > >
> > > > As you might notice, the location was changed by
> > > > my recent patch.
> > > >
> > >
> > > Will check and update.
> > >
> > >
> > > >
> > > > > +#include <crypto/pkcs7.h>
> > > > > +#include <linux/err.h>
> > > > >
> > > > >  const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
> > > > >  static const efi_guid_t efi_guid_firmware_management_capsule_id =
> > > > > @@ -245,6 +249,128 @@ out:
> > > > >
> > > > >       return ret;
> > > > >  }
> > > > > +
> > > > > +#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> > > > > +
> > > > > +const efi_guid_t efi_guid_capsule_root_cert_guid =
> > > > > +     EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > > > > +
> > > > > +__weak u16 *efi_get_truststore_name(void)
> > > > > +{
> > > > > +     return L"CRT";
> > > >
> > > > This variable is not defined by UEFI specification, isn't it?
> > > > How can this variable be protected?
> > > >
> > >
> > > This is not part of the uefi specification. The uefi specification does
> > not
> > > mandate any particular method for storing the root certificate to be used
> > > for capsule authentication. The edk2 code gets the root certificate
> > through
> > > a pcd. I had tried using KEK variable for storing the root certificate,
> > and
> > > had also come up with an implementation. However, the addition/deletion
> > and
> > > update of the secure variables is very closely tied with the secure boot
> > > feature and the secure boot state of the system, which is the reason why
> > i
> > > dropped that solution and used a separate variable, which can be defined
> > by
> > > the vendor to store the root certificate.
> >
> > My concern is that, without any protection, the value of this variable
> > can be modified by a mal-user.
> > (One simple solution would be to set this variable read-only.)
> >
> 
> Yes, that is one solution. This will also be taken care of in a scenario
> where the platform is booted in a trusted chain, and the env is also part
> of the image which gets verified by the previous stage(e.g BL2) before
> jumping to the u-boot image. If there is any change in the u-boot image,
> that would result in a boot failure -- but that would need the env to be
> part of the u-boot image which gets verified. I will explore making this
> variable read-only.
> 
> 
> > >
> > > >
> > > > > +}
> > > > > +
> > > > > +__weak const efi_guid_t *efi_get_truststore_vendor(void)
> > > > > +{
> > > > > +
> > > > > +     return &efi_guid_capsule_root_cert_guid;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * efi_capsule_authenticate() - Authenticate a uefi capsule
> > > > > + *
> > > > > + * @capsule:         Capsule file with the authentication
> > > > > + *                   header
> > > > > + * @capsule_size:    Size of the entire capsule
> > > > > + * @image:           pointer to the image payload minus the
> > > > > + *                   authentication header
> > > > > + * @image_size:              size of the image payload
> > > > > + *
> > > > > + * Authenticate the capsule signature with the public key contained
> > > > > + * in the root certificate stored as an efi environment variable
> > > > > + *
> > > > > + * Return: EFI_SUCCESS on successfull authentication or
> > > > > + *      EFI_SECURITY_VIOLATION on authentication failure
> > > > > + */
> > > > > +efi_status_t efi_capsule_authenticate(const void *capsule,
> > > > > +                                   efi_uintn_t capsule_size,
> > > > > +                                   void **image, efi_uintn_t
> > > > *image_size)
> > > > > +{
> > > > > +     uint64_t monotonic_count;
> > > > > +     struct efi_signature_store *truststore;
> > > > > +     struct pkcs7_message *capsule_sig;
> > > > > +     struct efi_image_regions *regs;
> > > > > +     struct efi_firmware_image_authentication *auth_hdr;
> > > > > +     efi_status_t status;
> > > > > +
> > > > > +     status = EFI_SECURITY_VIOLATION;
> > > > > +     capsule_sig = NULL;
> > > > > +     truststore = NULL;
> > > > > +     regs = NULL;
> > > > > +
> > > > > +     /* Sanity checks */
> > > > > +     if (capsule == NULL || capsule_size == 0)
> > > > > +             goto out;
> > > > > +
> > > > > +     auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> > > > > +     if (capsule_size < sizeof(*auth_hdr))
> > > > > +             goto out;
> > > > > +
> > > > > +     if (auth_hdr->auth_info.hdr.dwLength <=
> > > > > +         offsetof(struct win_certificate_uefi_guid, cert_data))
> > > > > +             goto out;
> > > > > +
> > > > > +     if (guidcmp(&auth_hdr->auth_info.cert_type,
> > > > &efi_guid_cert_type_pkcs7))
> > > > > +             goto out;
> > > > > +
> > > > > +     *image = (uint8_t *)capsule +
> > sizeof(auth_hdr->monotonic_count) +
> > > > > +             auth_hdr->auth_info.hdr.dwLength;
> > > > > +     *image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> > > > > +
> >  sizeof(auth_hdr->monotonic_count);
> > > > > +     memcpy(&monotonic_count, &auth_hdr->monotonic_count,
> > > > > +            sizeof(monotonic_count));
> > > > > +
> > > > > +     /* data to be digested */
> > > > > +     regs = calloc(sizeof(*regs) + sizeof(struct image_region) * 2,
> > 1);
> > > > > +     if (!regs)
> > > > > +             goto out;
> > > > > +
> > > > > +     regs->max = 2;
> > > > > +     efi_image_region_add(regs, (uint8_t *)*image,
> > > > > +                          (uint8_t *)*image + *image_size, 1);
> > > > > +
> > > > > +     efi_image_region_add(regs, (uint8_t *)&monotonic_count,
> > > > > +                          (uint8_t *)&monotonic_count +
> > > > sizeof(monotonic_count),
> > > > > +                          1);
> > > >
> > > > Is the order of regions to be calculated correct?
> > > > It seems that 'monotonic_count' precedes 'image' itself
> > > > in a capsule header.
> > > >
> > >
> > > Does that have any impact on the hash value computed.
> >
> > Not 100% sure, but if it doesn't, it cannot guarantee uniqueness
> > of digest values.
> >
> 
> Will check this.
> 
> -sughosh
> 
> 
> >
> > Thanks,
> > -Takahiro Akashi
> >
> > > I did not see any
> > > difference in the hash value computed due to the order in which the
> > regions
> > > are added. But that could be because of the way the hash value gets
> > > computed at the time of building the capsule. I will check this out.
> > >
> > >
> > > >
> > > > > +
> > > > > +     capsule_sig =
> > efi_parse_pkcs7_header(auth_hdr->auth_info.cert_data,
> > > > > +
> > > > auth_hdr->auth_info.hdr.dwLength
> > > > > +                                          -
> > > > sizeof(auth_hdr->auth_info));
> > > > > +     if (IS_ERR(capsule_sig)) {
> > > >
> > > > As Patrick reported, ex-efi_variable_parse_signature()
> > > > returns NULL on error cases, this check should be "!capsule_sig."
> > > >
> > >
> > > Will change.
> > >
> > > -sughosh
> >