[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

> > > +             if (!(active & alg_to_mask(hash_alg)))
> > > +                     continue;
> > > +             switch (hash_alg) {
> > > +             case TPM2_ALG_SHA1:
> >
> > SHA1 is known to be unsafe. Why would we support it?
> 
> Basically I agree with removing SHA1 support.
> This efi_tcg2.c implementation aims to support TCG v2, so there is no
> reason to keep SHA1.
> Anyway, SHA1 is supported in tcg2_create_digest() for the measurement
> other than PE/COFF image. Do we also remove SHA1 from
> tcg2_create_digest()?
> 

The hardware dictates what kind of SHAxxx you are supposed to add in the
EventLog and the PCRs. Why would we remove the functionality?  If someone
considers SHA1 unsafe, he can just disable it from his hardware and remove it
from the active algorithms.


Cheers
/Ilias

> For other comments, I will modify the code and send v2 patch.
> 
> Thanks,
> Masahisa
> 
> 
> On Wed, 21 Apr 2021 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 4/15/21 3:30 PM, Masahisa Kojima wrote:
> > > "TCG PC Client Platform Firmware Profile Specification"
> > > requires to measure every attempt to load and execute
> > > a OS Loader(a UEFI application) into PCR[4].
> > > This commit adds the PE/COFF image measurement, extends PCR,
> > > and appends measurement into Event Log.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > >   include/efi_loader.h              |   4 +
> > >   include/efi_tcg2.h                |  10 ++
> > >   include/tpm-v2.h                  |   1 +
> > >   lib/efi_loader/efi_image_loader.c |   7 ++
> > >   lib/efi_loader/efi_tcg2.c         | 187 ++++++++++++++++++++++++++++--
> > >   5 files changed, 199 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index de1a496a97..b02bc93c8e 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void);
> > >   efi_status_t efi_rng_register(void);
> > >   /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
> > >   efi_status_t efi_tcg2_register(void);
> > > +/* measure the pe-coff image, extend PCR and add Event Log */
> > > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
> > > +                                struct efi_loaded_image_obj *handle,
> > > +                                struct efi_loaded_image *loaded_image_info);
> > >   /* Create handles and protocols for the partitions of a block device */
> > >   int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> > >                              const char *if_typename, int diskid,
> > > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> > > index 40e241ce31..f8d46c5fd2 100644
> > > --- a/include/efi_tcg2.h
> > > +++ b/include/efi_tcg2.h
> > > @@ -9,6 +9,8 @@
> > >   #if !defined _EFI_TCG2_PROTOCOL_H_
> > >   #define _EFI_TCG2_PROTOCOL_H_
> > >
> > > +#include <efi.h>
> >
> > This include is already included in efi_api.h.
> >
> > > +#include <efi_api.h>
> > >   #include <tpm-v2.h>
> > >
> > >   #define EFI_TCG2_PROTOCOL_GUID \
> > > @@ -53,6 +55,14 @@ struct efi_tcg2_event {
> > >       u8 event[];
> > >   } __packed;
> > >
> > > +struct uefi_image_load_event {
> > > +     efi_physical_addr_t image_location_in_memory;
> > > +     u64 image_length_in_memory;
> > > +     u64 image_link_time_address;
> > > +     u64 length_of_device_path;
> > > +     struct efi_device_path device_path[];
> >
> > A device path is not an array of struct efi_device_path. But the first
> > element is of this type. So ok.
> >
> > > +} __packed;
> >
> > Why should this be __packed? You don't use arrays of this structure and
> > it is naturally packed.
> >
> > > +
> > >   struct efi_tcg2_boot_service_capability {
> > >       u8 size;
> > >       struct efi_tcg2_version structure_version;
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index df67a196cf..ab9c04dc0a 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -61,6 +61,7 @@ struct udevice;
> > >   #define EV_S_CRTM_VERSION   ((u32)0x00000008)
> > >   #define EV_CPU_MICROCODE    ((u32)0x00000009)
> > >   #define EV_TABLE_OF_DEVICES ((u32)0x0000000B)
> >
> > Please, add a comment here that the following values are defined in the
> > "TCG EFI Platform Specification".
> >
> > > +#define EV_EFI_BOOT_SERVICES_APPLICATION     ((u32)0x80000003)
> >
> > Please, add all EV_EFI_* constants.
> >
> > >
> > >   /* TPMS_TAGGED_PROPERTY Structure */
> > >   struct tpms_tagged_property {
> > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > > index 2c35cb5651..b032ec5dd8 100644
> > > --- a/lib/efi_loader/efi_image_loader.c
> > > +++ b/lib/efi_loader/efi_image_loader.c
> > > @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
> > >               goto err;
> > >       }
> > >
> > > +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL)
> > > +     /* Measure an PE/COFF image */
> > > +     if (tcg2_measure_pe_image(efi, efi_size, handle,
> > > +                               loaded_image_info))
> > > +             log_err("PE image measurement failed\n");
> > > +#endif
> > > +
> > >       /* Copy PE headers */
> > >       memcpy(efi_reloc, efi,
> > >              sizeof(*dos)
> > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > index ed86a220fb..9fab07605f 100644
> > > --- a/lib/efi_loader/efi_tcg2.c
> > > +++ b/lib/efi_loader/efi_tcg2.c
> > > @@ -13,8 +13,10 @@
> > >   #include <efi_loader.h>
> > >   #include <efi_tcg2.h>
> > >   #include <log.h>
> > > +#include <malloc.h>
> > >   #include <version.h>
> > >   #include <tpm-v2.h>
> > > +#include <u-boot/rsa.h>
> > >   #include <u-boot/sha1.h>
> > >   #include <u-boot/sha256.h>
> > >   #include <u-boot/sha512.h>
> > > @@ -709,6 +711,172 @@ out:
> > >       return EFI_EXIT(ret);
> > >   }
> > >
> > > +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
> > > +                                    struct tpml_digest_values *digest_list)
> > > +{
> > > +     IMAGE_NT_HEADERS32 *nt;
> > > +     WIN_CERTIFICATE *wincerts = NULL;
> > > +     size_t wincerts_len;
> > > +     struct efi_image_regions *regs = NULL;
> > > +     void *new_efi = NULL;
> > > +     size_t new_efi_size;
> > > +     u8 hash[TPM2_SHA512_DIGEST_SIZE];
> > > +     efi_status_t ret;
> > > +     u32 active;
> > > +     int i;
> > > +
> > > +     ret = efi_check_pe(efi, efi_size, (void **)&nt);
> > Why are you calling this function? It is already called in efi_load_pe().
> >
> > > +     if (ret != EFI_SUCCESS) {
> > > +             log_err("Not a valid PE-COFF file\n");
> > > +             return EFI_INVALID_PARAMETER;
> > > +     }
> > > +
> > > +     /*
> > > +      * Size must be 8-byte aligned and the trailing bytes must be
> > > +      * zero'ed. Otherwise hash value may be incorrect.
> > > +      */
> > > +     if (!IS_ALIGNED(efi_size, 8)) {
> > > +             new_efi_size = ALIGN(efi_size, 8);
> > > +             new_efi = calloc(new_efi_size, 1);
> > > +             if (!new_efi)
> > > +                     return EFI_OUT_OF_RESOURCES;
> > > +             memcpy(new_efi, efi, efi_size);
> > > +             efi = new_efi;
> > > +             efi_size = new_efi_size;
> > > +     }
> > > +
> > > +     if (!efi_image_parse(efi, efi_size, &regs, &wincerts,
> > > +                          &wincerts_len)) {
> > > +             log_err("Parsing PE executable image failed\n");
> > > +             ret = EFI_INVALID_PARAMETER;
> > > +             goto out;
> > > +     }
> >
> > Please, don't duplicate code from efi_image_authenticate(). Extract a
> > common function instead.
> >
> > > +
> > > +     ret = __get_active_pcr_banks(&active);
> > > +     if (ret != EFI_SUCCESS) {
> > > +             ret = EFI_DEVICE_ERROR;
> > > +             goto out;
> > > +     }
> > > +
> > > +     digest_list->count = 0;
> > > +     for (i = 0; i < MAX_HASH_COUNT; i++) {
> > > +             u16 hash_alg = hash_algo_list[i].hash_alg;
> > > +
> > > +             if (!(active & alg_to_mask(hash_alg)))
> > > +                     continue;
> > > +             switch (hash_alg) {
> > > +             case TPM2_ALG_SHA1:
> >
> > SHA1 is known to be unsafe. Why would we support it?
> >
> > > +                     hash_calculate("sha1", regs->reg, regs->num, hash);
> > > +                     digest_list->count++;
> >
> > Why do you repeat this line in every case of the switch statement?
> > Please, put it below.
> >
> > > +                     break;
> > > +             case TPM2_ALG_SHA256:
> > > +                     hash_calculate("sha256", regs->reg, regs->num, hash);
> > > +                     digest_list->count++;
> > > +                     break;
> > > +             case TPM2_ALG_SHA384:
> > > +                     hash_calculate("sha384", regs->reg, regs->num, hash);
> > > +                     digest_list->count++;
> > > +                     break;
> > > +             case TPM2_ALG_SHA512:
> > > +                     hash_calculate("sha512", regs->reg, regs->num, hash);
> > > +                     digest_list->count++;
> > > +                     break;
> > > +             default:
> > > +                     EFI_PRINT("Unsupported algorithm %x\n", hash_alg);
> > > +                     return EFI_INVALID_PARAMETER;
> > > +             }
> > > +             digest_list->digests[i].hash_alg = hash_alg;
> > > +             memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg));
> > > +     }
> > > +
> > > +out:
> > > +     free(new_efi);
> > > +     free(regs);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
> > > +                                struct efi_loaded_image_obj *handle,
> > > +                                struct efi_loaded_image *loaded_image)
> > > +{
> > > +     struct tpml_digest_values digest_list;
> > > +     efi_status_t ret;
> > > +     struct udevice *dev;
> > > +     u32 pcr_index, event_type, event_size;
> > > +     struct uefi_image_load_event *image_load_event;
> > > +     u8 *event;
> >
> > The variable event is not needed. You can use image_load_event directly.
> >
> > > +     struct efi_device_path *device_path;
> > > +     u32 device_path_length;
> > > +     IMAGE_DOS_HEADER *dos;
> > > +     IMAGE_NT_HEADERS32 *nt;
> > > +
> > > +     ret = platform_get_tpm2_device(&dev);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> >
> > We should measure drivers too. Cf.
> > EV_EFI_BOOT_SERVICES_DRIVER, EV_EFI_RUNTIME_SERVICES_DRIVER.
> >
> > > +             pcr_index = 4;
> > > +             event_type = EV_EFI_BOOT_SERVICES_APPLICATION;
> > > +     } else {
> > > +             return EFI_UNSUPPORTED;
> > > +     }
> > > +
> > > +     ret = tcg2_hash_pe_image(efi, efi_size, &digest_list);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     loaded_image->system_table->boottime->handle_protocol(&handle->header,
> > > +                                     &efi_guid_loaded_image_device_path,
> > > +                                     (void **)&device_path);
> >
> > You would have to use EFI_CALL() here.
> >
> > Please, use efi_search_protocol() instead of all this indirection.
> >
> > Best regards
> >
> > Heinrich
> >
> > > +     device_path_length = efi_dp_size(device_path);
> > > +     if (device_path_length > 0) {
> > > +             /* add end node size */
> > > +             device_path_length += sizeof(struct efi_device_path);
> > > +     }
> > > +     event_size = sizeof(struct uefi_image_load_event) + device_path_length;
> > > +     event = malloc(event_size);
> > > +     if (!event)
> > > +             return EFI_OUT_OF_RESOURCES;
> > > +
> > > +     image_load_event = (struct uefi_image_load_event *)event;
> > > +     image_load_event->image_location_in_memory = (efi_physical_addr_t)efi;
> > > +     image_load_event->image_length_in_memory = efi_size;
> > > +     image_load_event->length_of_device_path = device_path_length;
> > > +
> > > +     dos = (IMAGE_DOS_HEADER *)efi;
> > > +     nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew);
> > > +     if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> > > +             IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt;
> > > +
> > > +             image_load_event->image_link_time_address =
> > > +                             nt64->OptionalHeader.ImageBase;
> > > +     } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> > > +             image_load_event->image_link_time_address =
> > > +                             nt->OptionalHeader.ImageBase;
> > > +     } else {
> > > +             ret = EFI_INVALID_PARAMETER;
> > > +             goto out;
> > > +     }
> > > +
> > > +     if (device_path_length > 0) {
> > > +             memcpy(image_load_event->device_path, device_path,
> > > +                    device_path_length);
> > > +     }
> > > +
> > > +     ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
> > > +                                 event_size, event);
> > > +
> > > +out:
> > > +     free(event);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >   /**
> > >    * efi_tcg2_hash_log_extend_event() - extend and optionally log events
> > >    *
> > > @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags,
> > >       /*
> > >        * if PE_COFF_IMAGE is set we need to make sure the image is not
> > >        * corrupted, verify it and hash the PE/COFF image in accordance with
> > > -      * the  procedure  specified  in  "Calculating  the  PE  Image  Hash"
> > > -      * section  of the "Windows Authenticode Portable Executable Signature
> > > +      * the procedure specified in "Calculating the PE Image Hash"
> > > +      * section of the "Windows Authenticode Portable Executable Signature
> > >        * Format"
> > > -      * Not supported for now
> > >        */
> > >       if (flags & PE_COFF_IMAGE) {
> > > -             ret = EFI_UNSUPPORTED;
> > > -             goto out;
> > > +             ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len,
> > > +                                      &digest_list);
> > > +     } else {
> > > +             ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len,
> > > +                                      &digest_list);
> > >       }
> > > +     if (ret != EFI_SUCCESS)
> > > +             goto out;
> > >
> > >       pcr_index = efi_tcg_event->header.pcr_index;
> > >       event_type = efi_tcg_event->header.event_type;
> > >
> > > -     ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len,
> > > -                              &digest_list);
> > > -     if (ret != EFI_SUCCESS)
> > > -             goto out;
> > > -
> > >       ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> > >       if (ret != EFI_SUCCESS)
> > >               goto out;
> > >
> >