On Thu, Jan 23, 2020 at 06:41:53PM +0100, Heinrich Schuchardt wrote:
> On 1/22/20 8:42 AM, AKASHI Takahiro wrote:
> >Heinrich,
> >
> >On Wed, Jan 22, 2020 at 10:13:48AM +0900, AKASHI Takahiro wrote:
> >>On Tue, Jan 21, 2020 at 08:15:06AM +0100, Heinrich Schuchardt wrote:
> >>>On 1/21/20 7:12 AM, AKASHI Takahiro wrote:
> >>>>On Fri, Jan 17, 2020 at 06:51:50AM +0100, Heinrich Schuchardt wrote:
> >>>>>On 1/17/20 6:11 AM, AKASHI Takahiro wrote:
> >>>>>>On Thu, Jan 09, 2020 at 12:55:17AM +0100, Heinrich Schuchardt wrote:
> >>>>>>>On 12/18/19 1:45 AM, AKASHI Takahiro wrote:
> >>>>>>>>With this commit, image validation can be enforced, as UEFI
> >>>>>>>>specification
> >>>>>>>>section 32.5 describes, if CONFIG_EFI_SECURE_BOOT is enabled.
> >>>>>>>>
> >>>>>>>>Currently we support
> >>>>>>>>* authentication based on db and dbx,
> >>>>>>>> so dbx-validated image will always be rejected.
> >>>>>>>>* following signature types:
> >>>>>>>> EFI_CERT_SHA256_GUID (SHA256 digest for unsigned images)
> >>>>>>>> EFI_CERT_X509_GUID (x509 certificate for signed images)
> >>>>>>>>Timestamp-based certificate revocation is not supported here.
> >>>>>>>>
> >>>>>>>>Internally, authentication data is stored in one of certificates
> >>>>>>>>tables
> >>>>>>>>of PE image (See efi_image_parse()) and will be verified by
> >>>>>>>>efi_image_authenticate() before loading a given image.
> >>>>>>>>
> >>>>>>>>It seems that UEFI specification defines the verification process
> >>>>>>>>in a bit ambiguous way. I tried to implement it as closely to as
> >>>>>>>>EDK2 does.
> >>>>>>>>
> >>>>>>>>Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> >>>>>>>>---
> >>>>>>>> include/efi_loader.h | 7 +-
> >>>>>>>> lib/efi_loader/efi_boottime.c | 2 +-
> >>>>>>>> lib/efi_loader/efi_image_loader.c | 454
> >>>>>>>> +++++++++++++++++++++++++++++-
> >>>>>>>> 3 files changed, 449 insertions(+), 14 deletions(-)
> >>>>>>>>
> >>>>>>>>diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>>>>>>>index 1f88caf86709..e12b49098fb0 100644
> >>>>>>>>--- a/include/efi_loader.h
> >>>>>>>>+++ b/include/efi_loader.h
> >>>>>>>>@@ -11,6 +11,7 @@
> >>>>>>>> #include <common.h>
> >>>>>>>> #include <part_efi.h>
> >>>>>>>> #include <efi_api.h>
> >>>>>>>>+#include <pe.h>
> >>>>>>>>
> >>>>>>>> static inline int guidcmp(const void *g1, const void *g2)
> >>>>>>>> {
> >>>>>>>>@@ -398,7 +399,8 @@ efi_status_t efi_set_watchdog(unsigned long
> >>>>>>>>timeout);
> >>>>>>>> /* Called from places to check whether a timer expired */
> >>>>>>>> void efi_timer_check(void);
> >>>>>>>> /* PE loader implementation */
> >>>>>>>>-efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void
> >>>>>>>>*efi,
> >>>>>>>>+efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
> >>>>>>>>+ void *efi, size_t efi_size,
> >>>>>>>> struct efi_loaded_image *loaded_image_info);
> >>>>>>>> /* Called once to store the pristine gd pointer */
> >>>>>>>> void efi_save_gd(void);
> >>>>>>>>@@ -726,6 +728,9 @@ void efi_sigstore_free(struct efi_signature_store
> >>>>>>>>*sigstore);
> >>>>>>>> struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name);
> >>>>>>>>
> >>>>>>>> bool efi_secure_boot_enabled(void);
> >>>>>>>>+
> >>>>>>>>+bool efi_image_parse(void *efi, size_t len, struct efi_image_regions
> >>>>>>>>**regp,
> >>>>>>>>+ WIN_CERTIFICATE **auth, size_t *auth_len);
> >>>>>>>> #endif /* CONFIG_EFI_SECURE_BOOT */
> >>>>>>>>
> >>>>>>>> #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> >>>>>>>>diff --git a/lib/efi_loader/efi_boottime.c
> >>>>>>>>b/lib/efi_loader/efi_boottime.c
> >>>>>>>>index 493d906c641d..311681764034 100644
> >>>>>>>>--- a/lib/efi_loader/efi_boottime.c
> >>>>>>>>+++ b/lib/efi_loader/efi_boottime.c
> >>>>>>>>@@ -1879,7 +1879,7 @@ efi_status_t EFIAPI efi_load_image(bool
> >>>>>>>>boot_policy,
> >>>>>>>> efi_dp_split_file_path(file_path, &dp, &fp);
> >>>>>>>> ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
> >>>>>>>> if (ret == EFI_SUCCESS)
> >>>>>>>>- ret = efi_load_pe(*image_obj, dest_buffer, info);
> >>>>>>>>+ ret = efi_load_pe(*image_obj, dest_buffer, source_size,
> >>>>>>>>info);
> >>>>>>>> if (!source_buffer)
> >>>>>>>> /* Release buffer to which file was loaded */
> >>>>>>>> efi_free_pages((uintptr_t)dest_buffer,
> >>>>>>>>diff --git a/lib/efi_loader/efi_image_loader.c
> >>>>>>>>b/lib/efi_loader/efi_image_loader.c
> >>>>>>>>index 13541cfa7a28..939758e61e3c 100644
> >>>>>>>>--- a/lib/efi_loader/efi_image_loader.c
> >>>>>>>>+++ b/lib/efi_loader/efi_image_loader.c
> >>>>>>>>@@ -9,7 +9,9 @@
> >>>>>>>>
> >>>>>>>> #include <common.h>
> >>>>>>>> #include <efi_loader.h>
> >>>>>>>>+#include <malloc.h>
> >>>>>>>> #include <pe.h>
> >>>>>>>>+#include "../lib/crypto/pkcs7_parser.h"
> >>>>>>>>
> >>>>>>>> const efi_guid_t efi_global_variable_guid =
> >>>>>>>> EFI_GLOBAL_VARIABLE_GUID;
> >>>>>>>> const efi_guid_t efi_guid_device_path =
> >>>>>>>> EFI_DEVICE_PATH_PROTOCOL_GUID;
> >>>>>>>>@@ -205,6 +207,367 @@ static void efi_set_code_and_data_type(
> >>>>>>>> }
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>>+#ifdef CONFIG_EFI_SECURE_BOOT
> >>>>>>>>+/**
> >>>>>>>>+ * efi_image_parse - parse a PE image
> >>>>>>>>+ * @efi: Pointer to image
> >>>>>>>>+ * @len: Size of @efi
> >>>>>>>>+ * @regs: Pointer to a list of regions
> >>>>>>>>+ * @auth: Pointer to a pointer to authentication data in PE
> >>>>>>>>+ * @auth_len: Size of @auth
> >>>>>>>>+ *
> >>>>>>>>+ * Parse image binary in PE32(+) format, assuming that sanity of PE
> >>>>>>>>image
> >>>>>>>>+ * has been checked by a caller.
> >>>>>>>>+ * On success, an address of authentication data in @efi and its
> >>>>>>>>size will
> >>>>>>>>+ * be returned in @auth and @auth_len, respectively.
> >>>>>>>>+ *
> >>>>>>>>+ * Return: true on success, false on error
> >>>>>>>>+ */
> >>>>>>>>+bool efi_image_parse(void *efi, size_t len, struct efi_image_regions
> >>>>>>>>**regp,
> >>>>>>>>+ WIN_CERTIFICATE **auth, size_t *auth_len)
> >>>>>>>
> >>>>>>>
> >>>>>>>This function is way too long (> 100 lines). Please, cut it into
> >>>>>>>logical
> >>>>>>>units.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>>+{
> >>>>>>>>+ struct efi_image_regions *regs;
> >>>>>>>>+ IMAGE_DOS_HEADER *dos;
> >>>>>>>>+ IMAGE_NT_HEADERS32 *nt;
> >>>>>>>>+ IMAGE_SECTION_HEADER *sections, **sorted;
> >>>>>>>>+ int num_regions, num_sections, i, j;
> >>>>>>>>+ int ctidx = IMAGE_DIRECTORY_ENTRY_SECURITY;
> >>>>>>>>+ u32 align, size, authsz, authoff;
> >>>>>>>>+ size_t bytes_hashed;
> >>>>>>>>+
> >>>>>>>>+ dos = (void *)efi;
> >>>>>>>>+ nt = (void *)(efi + dos->e_lfanew);
> >>>>>>>>+
> >>>>>>>>+ /*
> >>>>>>>>+ * Count maximum number of regions to be digested.
> >>>>>>>>+ * We don't have to have an exact number here.
> >>>>>>>>+ * See efi_image_region_add()'s in parsing below.
> >>>>>>>>+ */
> >>>>>>>>+ num_regions = 3; /* for header */
> >>>>>>>>+ num_regions += nt->FileHeader.NumberOfSections;
> >>>>>>>>+ num_regions++; /* for extra */
> >>>>>>>>+
> >>>>>>>>+ regs = calloc(sizeof(*regs) + sizeof(struct image_region) *
> >>>>>>>>num_regions,
> >>>>>>>>+ 1);
> >>>>>>>>+ if (!regs)
> >>>>>>>>+ goto err;
> >>>>>>>>+ regs->max = num_regions;
> >>>>>>>>+
> >>>>>>>>+ /*
> >>>>>>>>+ * Collect data regions for hash calculation
> >>>>>>>>+ * 1. File headers
> >>>>>>>>+ */
> >>>>>>>>+ if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> >>>>>>>>+ IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
> >>>>>>>>+ IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
> >>>>>>>>+
> >>>>>>>>+ /* Skip CheckSum */
> >>>>>>>>+ efi_image_region_add(regs, efi, &opt->CheckSum, 0);
> >>>>>>>>+ if (nt64->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
> >>>>>>>>+ efi_image_region_add(regs,
> >>>>>>>>+ &opt->CheckSum + 1,
> >>>>>>>>+ efi + opt->SizeOfHeaders,
> >>>>>>>>0);
> >>>>>>>>+ } else {
> >>>>>>>>+ /* Skip Certificates Table */
> >>>>>>>>+ efi_image_region_add(regs,
> >>>>>>>>+ &opt->CheckSum + 1,
> >>>>>>>>+
> >>>>>>>>&opt->DataDirectory[ctidx], 0);
> >>>>>>>>+ efi_image_region_add(regs,
> >>>>>>>>+ &opt->DataDirectory[ctidx]
> >>>>>>>>+ 1,
> >>>>>>>>+ efi + opt->SizeOfHeaders,
> >>>>>>>>0);
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ bytes_hashed = opt->SizeOfHeaders;
> >>>>>>>>+ align = opt->FileAlignment;
> >>>>>>>>+ authoff = opt->DataDirectory[ctidx].VirtualAddress;
> >>>>>>>>+ authsz = opt->DataDirectory[ctidx].Size;
> >>>>>>>>+ } else if (nt->OptionalHeader.Magic ==
> >>>>>>>>IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> >>>>>>>>+ IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
> >>>>>>>>+
> >>>>>>>>+ efi_image_region_add(regs, efi, &opt->CheckSum, 0);
> >>>>>>>>+ efi_image_region_add(regs, &opt->CheckSum + 1,
> >>>>>>>>+ &opt->DataDirectory[ctidx], 0);
> >>>>>>>>+ efi_image_region_add(regs, &opt->DataDirectory[ctidx] +
> >>>>>>>>1,
> >>>>>>>>+ efi + opt->SizeOfHeaders, 0);
> >>>>>>>>+
> >>>>>>>>+ bytes_hashed = opt->SizeOfHeaders;
> >>>>>>>>+ align = opt->FileAlignment;
> >>>>>>>>+ authoff = opt->DataDirectory[ctidx].VirtualAddress;
> >>>>>>>>+ authsz = opt->DataDirectory[ctidx].Size;
> >>>>>>>>+ } else {
> >>>>>>>>+ debug("%s: Invalid optional header magic %x\n",
> >>>>>>>>__func__,
> >>>>>>>>+ nt->OptionalHeader.Magic);
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* 2. Sections */
> >>>>>>>>+ num_sections = nt->FileHeader.NumberOfSections;
> >>>>>>>>+ sections = (void *)((uint8_t *)&nt->OptionalHeader +
> >>>>>>>>+ nt->FileHeader.SizeOfOptionalHeader);
> >>>>>>>>+ sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
> >>>>>>>>+ if (!sorted) {
> >>>>>>>>+ debug("%s: Out of memory\n", __func__);
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /*
> >>>>>>>>+ * Make sure the section list is in ascending order.
> >>>>>>>>+ * As we can assume that it is already ordered in most cases,
> >>>>>>>>+ * the following code is optimized for this.
> >>>>>>>>+ */
> >>>>>>>>+ for (sorted[0] = §ions[0], i = 1; i < num_sections; i++) {
> >>>>>>>
> >>>>>>>If sections[0] is not the lowest entry this function fails.
> >>>>>>>
> >>>>>>>Please, use qsort() supplied in lib/qsort.c.
> >>>>>>
> >>>>>>I'd rather fix my code as it is much simpler than qsort.
> >>>>>
> >>>>>Using qsort will result in a smaller code size. With qsort your code
> >>>>>will also be much easier to read.
> >>>>>
> >>>>>>
> >>>>>>>>+ if (sorted[i - 1]->VirtualAddress
> >>>>>>>>+ <= sections[i].VirtualAddress) {
> >>>>>>>>+ sorted[i] = §ions[i];
> >>>>>>>>+ } else {
> >>>>>>>>+ if (i == 1) {
> >>>>>>>>+ sorted[1] = sorted[0];
> >>>>>>>>+ sorted[0] = §ions[1];
> >>>>>>>>+ continue;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ sorted[i] = sorted[i - 1];
> >>>>>>>>+ for (j = i - 2; j >= 0; j--) {
> >>>>>>>>+ if (!j || sorted[j]->VirtualAddress
> >>>>>>>>+ <=
> >>>>>>>>sections[i].VirtualAddress) {
> >>>>>>>>+ sorted[j + 1] = §ions[i];
> >>>>>>>>+ continue;
> >>>>>>>>+ } else {
> >>>>>>>>+ sorted[j + 1] = sorted[j];
> >>>>>>>>+ }
> >>>>>>>>+ }
> >>>>>>>>+ }
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ for (i = 0; i < num_sections; i++) {
> >>>>>>>>+ if (!sorted[i]->SizeOfRawData)
> >>>>>>>>+ continue;
> >>>>>>>>+
> >>>>>>>>+ size = (sorted[i]->SizeOfRawData + align - 1) & ~(align
> >>>>>>>>- 1);
> >>>>>>>>+ efi_image_region_add(regs, efi +
> >>>>>>>>sorted[i]->PointerToRawData,
> >>>>>>>>+ efi + sorted[i]->PointerToRawData
> >>>>>>>>+ size,
> >>>>>>>>+ 0);
> >>>>>>>>+ debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
> >>>>>>>>+ i, sorted[i]->Name,
> >>>>>>>>+ sorted[i]->PointerToRawData,
> >>>>>>>>+ sorted[i]->PointerToRawData + size,
> >>>>>>>>+ sorted[i]->VirtualAddress,
> >>>>>>>>+ sorted[i]->VirtualAddress
> >>>>>>>>+ + sorted[i]->Misc.VirtualSize);
> >>>>>>>>+
> >>>>>>>>+ bytes_hashed += size;
> >>>>>>>>+ }
> >>>>>>>>+ free(sorted);
> >>>>>>>>+
> >>>>>>>>+ /* 3. Extra data excluding Certificates Table */
> >>>>>>>>+ if (bytes_hashed + authsz < len) {
> >>>>>>>>+ debug("extra data for hash: %lu\n",
> >>>>>>>>+ len - (bytes_hashed + authsz));
> >>>>>>>>+ efi_image_region_add(regs, efi + bytes_hashed,
> >>>>>>>>+ efi + len - authsz, 0);
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* Return Certificates Table */
> >>>>>>>>+ if (authsz) {
> >>>>>>>>+ if (len < authoff + authsz) {
> >>>>>>>>+ debug("%s: Size for auth too large: %u >=
> >>>>>>>>%zu\n",
> >>>>>>>>+ __func__, authsz, len - authoff);
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+ if (authsz < sizeof(*auth)) {
> >>>>>>>>+ debug("%s: Size for auth too small: %u < %zu\n",
> >>>>>>>>+ __func__, authsz, sizeof(*auth));
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+ *auth = efi + authoff;
> >>>>>>>>+ *auth_len = authsz;
> >>>>>>>>+ debug("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
> >>>>>>>>authsz);
> >>>>>>>>+ } else {
> >>>>>>>>+ *auth = NULL;
> >>>>>>>>+ *auth_len = 0;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ *regp = regs;
> >>>>>>>>+
> >>>>>>>>+ return true;
> >>>>>>>>+
> >>>>>>>>+err:
> >>>>>>>>+ free(regs);
> >>>>>>>>+
> >>>>>>>>+ return false;
> >>>>>>>>+}
> >>>>>>>>+
> >>>>>>>>+/**
> >>>>>>>>+ * efi_image_unsigned_authenticate - authenticate unsigned image with
> >>>>>>>>+ * SHA256 hash
> >>>>>>>>+ * @regs: List of regions to be verified
> >>>>>>>>+ *
> >>>>>>>>+ * If an image is not signed, it doesn't have a signature. In this
> >>>>>>>>case,
> >>>>>>>>+ * its message digest is calculated and it will be compared with one
> >>>>>>>>of
> >>>>>>>>+ * hash values stored in signature databases.
> >>>>>>>>+ *
> >>>>>>>>+ * Return: true if authenticated, false if not
> >>>>>>>>+ */
> >>>>>>>>+static bool efi_image_unsigned_authenticate(struct efi_image_regions
> >>>>>>>>*regs)
> >>>>>>>>+{
> >>>>>>>>+ struct efi_signature_store *db = NULL, *dbx = NULL;
> >>>>>>>>+ bool ret = false;
> >>>>>>>>+
> >>>>>>>>+ dbx = efi_sigstore_parse_sigdb(L"dbx");
> >>>>>>>>+ if (!dbx) {
> >>>>>>>>+ debug("Getting signature database(dbx) failed\n");
> >>>>>>>>+ goto out;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ db = efi_sigstore_parse_sigdb(L"db");
> >>>>>>>>+ if (!db) {
> >>>>>>>>+ debug("Getting signature database(db) failed\n");
> >>>>>>>>+ goto out;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* try black-list first */
> >>>>>>>>+ if (efi_signature_verify_with_sigdb(regs, NULL, dbx, NULL)) {
> >>>>>>>>+ debug("Image is not signed and rejected by \"dbx\"\n");
> >>>>>>>>+ goto out;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* try white-list */
> >>>>>>>>+ if (efi_signature_verify_with_sigdb(regs, NULL, db, NULL))
> >>>>>>>>+ ret = true;
> >>>>>>>>+ else
> >>>>>>>>+ debug("Image is not signed and not found in \"db\" or
> >>>>>>>>\"dbx\"\n");
> >>>>>>>>+
> >>>>>>>>+out:
> >>>>>>>>+ efi_sigstore_free(db);
> >>>>>>>>+ efi_sigstore_free(dbx);
> >>>>>>>>+
> >>>>>>>>+ return ret;
> >>>>>>>>+}
> >>>>>>>>+
> >>>>>>>>+/**
> >>>>>>>>+ * efi_image_authenticate - verify a signature of signed image
> >>>>>>>>+ * @efi: Pointer to image
> >>>>>>>>+ * @len: Size of @efi
> >>>>>>>>+ *
> >>>>>>>>+ * A signed image should have its signature stored in a table of its
> >>>>>>>>PE header.
> >>>>>>>>+ * So if an image is signed and only if if its signature is verified
> >>>>>>>>using
> >>>>>>>>+ * signature databases, an image is authenticated.
> >>>>>>>>+ * If an image is not signed, its validity is checked by using
> >>>>>>>>+ * efi_image_unsigned_authenticated().
> >>>>>>>>+ * TODO:
> >>>>>>>>+ * When AuditMode==0, if the image's signature is not found in
> >>>>>>>>+ * the authorized database, or is found in the forbidden database,
> >>>>>>>>+ * the image will not be started and instead, information about it
> >>>>>>>>+ * will be placed in this table.
> >>>>>>>>+ * When AuditMode==1, an EFI_IMAGE_EXECUTION_INFO element is created
> >>>>>>>>+ * in the EFI_IMAGE_EXECUTION_INFO_TABLE for every certificate found
> >>>>>>>>+ * in the certificate table of every image that is validated.
> >>>>>>>>+ *
> >>>>>>>>+ * Return: true if authenticated, false if not
> >>>>>>>>+ */
> >>>>>>>>+static bool efi_image_authenticate(void *efi, size_t len)
> >>>>>>>>+{
> >>>>>>>>+ struct efi_image_regions *regs = NULL;
> >>>>>>>>+ WIN_CERTIFICATE *wincerts = NULL, *wincert;
> >>>>>>>>+ size_t wincerts_len;
> >>>>>>>>+ struct pkcs7_message *msg = NULL;
> >>>>>>>>+ struct efi_signature_store *db = NULL, *dbx = NULL;
> >>>>>>>>+ struct x509_certificate *cert = NULL;
> >>>>>>>>+ bool ret = false;
> >>>>>>>>+
> >>>>>>>>+ if (!efi_secure_boot_enabled())
> >>>>>>>>+ return true;
> >>>>>>>>+
> >>>>>>>>+ if (!efi_image_parse(efi, len, ®s, &wincerts,
> >>>>>>>>+ &wincerts_len)) {
> >>>>>>>>+ debug("Parsing PE executable image failed\n");
> >>>>>>>>+ return false;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ if (!wincerts) {
> >>>>>>>>+ /* The image is not signed */
> >>>>>>>>+ ret = efi_image_unsigned_authenticate(regs);
> >>>>>>>>+ free(regs);
> >>>>>>>>+
> >>>>>>>>+ return ret;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /*
> >>>>>>>>+ * verify signature using db and dbx
> >>>>>>>>+ */
> >>>>>>>>+ db = efi_sigstore_parse_sigdb(L"db");
> >>>>>>>>+ if (!db) {
> >>>>>>>>+ debug("Getting signature database(db) failed\n");
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ dbx = efi_sigstore_parse_sigdb(L"dbx");
> >>>>>>>>+ if (!dbx) {
> >>>>>>>>+ debug("Getting signature database(dbx) failed\n");
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* go through WIN_CERTIFICATE list */
> >>>>>>>>+ for (wincert = wincerts;
> >>>>>>>>+ (void *)wincert < (void *)wincerts + wincerts_len;
> >>>>>>>>+ wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> >>>>>>>>+ if (wincert->dwLength < sizeof(*wincert)) {
> >>>>>>>>+ debug("%s: dwLength too small: %u < %zu\n",
> >>>>>>>>+ __func__, wincert->dwLength,
> >>>>>>>>sizeof(*wincert));
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+ msg = pkcs7_parse_message((void *)wincert +
> >>>>>>>>sizeof(*wincert),
> >>>>>>>>+ wincert->dwLength -
> >>>>>>>>sizeof(*wincert));
> >>>>>>>>+ if (!msg) {
> >>>>>>>>+ debug("Parsing image's signature failed\n");
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* try black-list first */
> >>>>>>>>+ if (efi_signature_verify_with_sigdb(regs, msg, dbx,
> >>>>>>>>NULL)) {
> >>>>>>>>+ debug("Signature was rejected by \"dbx\"\n");
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ if (!efi_signature_verify_signers(msg, dbx)) {
> >>>>>>>>+ debug("Signer was rejected by \"dbx\"\n");
> >>>>>>>>+ goto err;
> >>>>>>>>+ } else {
> >>>>>>>>+ ret = true;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* try white-list */
> >>>>>>>>+ if (!efi_signature_verify_with_sigdb(regs, msg, db,
> >>>>>>>>&cert)) {
> >>>>>>>>+ debug("Verifying signature with \"db\"
> >>>>>>>>failed\n");
> >>>>>>>>+ goto err;
> >>>>>>>>+ } else {
> >>>>>>>>+ ret = true;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ if (!efi_signature_verify_cert(cert, dbx)) {
> >>>>>>>>+ debug("Certificate was rejected by \"dbx\"\n");
> >>>>>>>>+ goto err;
> >>>>>>>>+ } else {
> >>>>>>>>+ ret = true;
> >>>>>>>>+ }
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+err:
> >>>>>>>>+ x509_free_certificate(cert);
> >>>>>>>>+ efi_sigstore_free(db);
> >>>>>>>>+ efi_sigstore_free(dbx);
> >>>>>>>>+ pkcs7_free_message(msg);
> >>>>>>>>+ free(regs);
> >>>>>>>>+
> >>>>>>>>+ return ret;
> >>>>>>>>+}
> >>>>>>>>+#else
> >>>>>>>>+static bool efi_image_authenticate(void *efi, size_t len)
> >>>>>>>>+{
> >>>>>>>>+ return true;
> >>>>>>>>+}
> >>>>>>>>+#endif /* CONFIG_EFI_SECURE_BOOT */
> >>>>>>>>+
> >>>>>>>> /**
> >>>>>>>> * efi_load_pe() - relocate EFI binary
> >>>>>>>> *
> >>>>>>>>@@ -216,7 +579,8 @@ static void efi_set_code_and_data_type(
> >>>>>>>> * @loaded_image_info: loaded image protocol
> >>>>>>>> * Return: status code
> >>>>>>>> */
> >>>>>>>>-efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void
> >>>>>>>>*efi,
> >>>>>>>>+efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
> >>>>>>>>+ void *efi, size_t efi_size,
> >>>>>>>> struct efi_loaded_image *loaded_image_info)
> >>>>>>>> {
> >>>>>>>> IMAGE_NT_HEADERS32 *nt;
> >>>>>>>>@@ -231,17 +595,57 @@ efi_status_t efi_load_pe(struct
> >>>>>>>>efi_loaded_image_obj *handle, void *efi,
> >>>>>>>> uint64_t image_base;
> >>>>>>>> unsigned long virt_size = 0;
> >>>>>>>> int supported = 0;
> >>>>>>>>+ void *new_efi = NULL;
> >>>>>>>>+ size_t new_efi_size;
> >>>>>>>>+ efi_status_t ret;
> >>>>>>>>+
> >>>>>>>>+ /*
> >>>>>>>>+ * Size must be 8-byte aligned and the trailing bytes must be
> >>>>>>>>+ * zero'ed. Otherwise hash value may be incorrect.
> >>>>>>>>+ */
> >>>>>>>>+ if (efi_size & 0x7) {
> >>>>>>>>+ new_efi_size = (efi_size + 0x7) & ~0x7ULL;
> >>>>>>>>+ 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;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* Sanity check for a file header */
> >>>>>>>>+ if (efi_size < sizeof(*dos)) {
> >>>>>>>>+ printf("%s: Truncated DOS Header\n", __func__);
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>
> >>>>>>>> dos = efi;
> >>>>>>>> if (dos->e_magic != IMAGE_DOS_SIGNATURE) {
> >>>>>>>> printf("%s: Invalid DOS Signature\n", __func__);
> >>>>>>>>- return EFI_LOAD_ERROR;
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* assume sizeof(IMAGE_NT_HEADERS32) <=
> >>>>>>>>sizeof(IMAGE_NT_HEADERS64) */
> >>>>>>>>+ if (efi_size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS32)) {
> >>>>>>>>+ printf("%s: Invalid offset for Extended Header\n",
> >>>>>>>>__func__);
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> nt = (void *) ((char *)efi + dos->e_lfanew);
> >>>>>>>>+ if ((nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC)
> >>>>>>>>&&
> >>>>>>>>+ (efi_size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS64))) {
> >>>>>>>>+ printf("%s: Invalid offset for Extended Header\n",
> >>>>>>>>__func__);
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>> if (nt->Signature != IMAGE_NT_SIGNATURE) {
> >>>>>>>> printf("%s: Invalid NT Signature\n", __func__);
> >>>>>>>>- return EFI_LOAD_ERROR;
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> for (i = 0; machines[i]; i++)
> >>>>>>>>@@ -253,14 +657,29 @@ efi_status_t efi_load_pe(struct
> >>>>>>>>efi_loaded_image_obj *handle, void *efi,
> >>>>>>>> if (!supported) {
> >>>>>>>> printf("%s: Machine type 0x%04x is not supported\n",
> >>>>>>>> __func__, nt->FileHeader.Machine);
> >>>>>>>>- return EFI_LOAD_ERROR;
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>>- /* Calculate upper virtual address boundary */
> >>>>>>>> num_sections = nt->FileHeader.NumberOfSections;
> >>>>>>>> sections = (void *)&nt->OptionalHeader +
> >>>>>>>> nt->FileHeader.SizeOfOptionalHeader;
> >>>>>>>>
> >>>>>>>>+ if (efi_size < ((void *)sections + sizeof(sections[0]) *
> >>>>>>>>num_sections
> >>>>>>>>+ - efi)) {
> >>>>>>>>+ printf("%s: Invalid number of sections: %d\n",
> >>>>>>>>+ __func__, num_sections);
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* Authenticate an image */
> >>>>>>>>+ if (!efi_image_authenticate(efi, efi_size)) {
> >>>>>>>>+ ret = EFI_ACCESS_DENIED;
> >>>>>>>
> >>>>>>>According to the UEFI specification LoadImage() should return
> >>>>>>>EFI_SECURITY_VIOLATION in this case.
> >>>>>>
> >>>>>>Will check.
> >>>>>>
> >>>>>>>Further behaviour should depend on variables AuditMode and
> >>>>>>>DeployedMode.
> >>>>>>>
> >>>>>>>If authentication fails, you must update the configuration table
> >>>>>>>identified by EFI_IMAGE_SECURITY_DATABASE_GUID containing the Image
> >>>>>>>Execution Information. If the authentication succeeds you may enter a
> >>>>>>>record.
> >>>>>>>
> >>>>>>>It seems to me that in your patch series you are not creating the
> >>>>>>>configuration table at all.
> >>>>>>
> >>>>>>>From the very beginning of my submissions, I have clearly said
> >>>>>>that this feature was *beyond the scope* in my current series.
> >>>>>>===8<===
> >>>>>>Unsupported features: (marked as TODO in most cases in the source code,
> >>>>>> and won't be included in this series)
> >>>>>>(snip)
> >>>>>>* recording rejected images in EFI_IMAGE_EXECUTION_INFO_TABLE
> >>>>>>===>8===
> >>>>>>
> >>>>>>>The content of the Image Execution Information Table should be used to
> >>>>>>>decide if StartImage() may start an image. This is still missing in the
> >>>>>>>patch series.
> >>>>>>
> >>>>>>No.
> >>>>>>Whether such information be in configuration table or not,
> >>>>>>non-authenticated image won't be started if secure boot is in force.
> >>>>>
> >>>>>I cannot find any such check in efi_start_image().
> >>>>
> >>>>Because my current code *rejects* any unauthenticated images being loaded
> >>>>and return no valid pointer to image handle.
> >>>>If an error code in this case is changed and efi_load_image() still
> >>>>returns
> >>>>a valid handle, we will also have to modify efi_start_image().
> >>>>
> >>>>>Please, provide an implementation that complies with the UEFI
> >>>>>specification:
> >>>>>
> >>>>>"The information is used to create the Image Execution Information
> >>>>>Table, which is added to the EFI System Configuration Table and assigned
> >>>>>the GUID EFI_IMAGE_SECURITY_DATABASE_GUID."
> >>>>
> >>>>Regarding "Image Execution Information Table", Heavily disagree.
> >>>>The current UEFI code, not only mine but yours, has bunch of unimplemented
> >>>>features.
> >>>
> >>>What is the point of this patch if efi_start_image() does not provide
> >>>any check?
> >>
> >>As I said in the previous reply,
> >>efi_load_image() doesn't return any valid handle for unauthenticated
> >>binaries
> >>in my *current* implementation, efi_start_image() has no chance to execute
> >>them.
> >>No check is necessary. That's it.
> >
> >I double-checked edk2 code as well as UEFI specification, and found
> >a couple of insights:
> >
> >a. EDK2 code has several internal help functions for verifications.
> > If they fail to find any valid signature in db's, the status (or
> > internal error code) is set to ACCESS_DENIED.
> > Then, at the end of verification, if the status is not EFI_SUCCESS,
> > the return code is anyhow rewritten to SECURITY_VIOLATION.
> > (That is why my code returns ACCESS_DENIED right now.)
> >b. While UEFI specification requires that efiLoadImage() return
> > SECURITY_VIOLATION if "the image signature is not valid,"
> > it doesn't mention if a handle to the image be returned or not.
>
> Please, have look at the return values for LoadImage() in the UEFI 2.8 spec.
>
> If you return ACCESS_DENIED, <cite>Image was not loaded because the
> platform policy prohibits the image from being loaded. NULL is returned
> in *ImageHandle</cite>
>
> If you return SECURITY_VIOLATION, <cite>Image was loaded and an
> ImageHandle was created with a valid EFI_LOADED_IMAGE_PROTOCOL.</cite>
>
> In the case of SECURITY_VIOLATION we have to ensure that
> efi_start_image() does not start the image.
Okay.
> This is why we need the
> EFI_IMAGE_SECURITY_DATABASE.
No. Disagree.
Whether we support Image Execution Information Table or not,
we should and can prevent the image from being executed.
It's just a matter of implementation of efi_start_image().
The information is nothing but for UEFI applications/drivers
like other tables in configuration table.
-Takahiro Akashi
> >c. "Status Codes Returned" can also read that it depends on
> > "platform policy" if we return ACCESS_DENIED or SECURITY_VIOLATION.
> > But the policy may be vendor/platform, or even U-Boot specific as
> > UEFI specification doesn't mention anything about that.
> >
> >Thinking of the fact that we don't have any consensus nor implementation
> >of "policy" yet, I believe that the best solution for now is:
> > efi_load_image() return EFI_SECURITY_VIOLATION if the image signature
> > is not verified and does *not* return a handle to image.
>
> This would violate the UEFI spec.
>
> Best regards
>
> Heinrich
>
> >
> >This behavior is safe and yet won't prevent us from additionally implementing
> >"policy" framework as well as image information table when adding
> >Audit/DeployedMode support in the future.
> >Can you agree?
> >
> >-Takahiro Akashi
> >
> >
> >>-Takahiro Akashi
> >>
> >>>Best regards
> >>>
> >>>Heinrich
> >>>
> >>>>
> >>>>-Takahiro Akashi
> >>>>
> >>>>
> >>>>>Best regards
> >>>>>
> >>>>>Heinrich
> >>>>>
> >>>>>>
> >>>>>>Thanks,
> >>>>>>-Takahiro Akashi
> >>>>>>
> >>>>>>>
> >>>>>>>Best regards
> >>>>>>>
> >>>>>>>Heinrich
> >>>>>>>
> >>>>>>>>+ goto err;
> >>>>>>>>+ }
> >>>>>>>>+
> >>>>>>>>+ /* Calculate upper virtual address boundary */
> >>>>>>>> for (i = num_sections - 1; i >= 0; i--) {
> >>>>>>>> IMAGE_SECTION_HEADER *sec = §ions[i];
> >>>>>>>> virt_size = max_t(unsigned long, virt_size,
> >>>>>>>>@@ -279,7 +698,8 @@ efi_status_t efi_load_pe(struct
> >>>>>>>>efi_loaded_image_obj *handle, void *efi,
> >>>>>>>> if (!efi_reloc) {
> >>>>>>>> printf("%s: Could not allocate %lu bytes\n",
> >>>>>>>> __func__, virt_size);
> >>>>>>>>- return EFI_OUT_OF_RESOURCES;
> >>>>>>>>+ ret = EFI_OUT_OF_RESOURCES;
> >>>>>>>>+ goto err;
> >>>>>>>> }
> >>>>>>>> handle->entry = efi_reloc + opt->AddressOfEntryPoint;
> >>>>>>>> rel_size = opt->DataDirectory[rel_idx].Size;
> >>>>>>>>@@ -295,7 +715,8 @@ efi_status_t efi_load_pe(struct
> >>>>>>>>efi_loaded_image_obj *handle, void *efi,
> >>>>>>>> if (!efi_reloc) {
> >>>>>>>> printf("%s: Could not allocate %lu bytes\n",
> >>>>>>>> __func__, virt_size);
> >>>>>>>>- return EFI_OUT_OF_RESOURCES;
> >>>>>>>>+ ret = EFI_OUT_OF_RESOURCES;
> >>>>>>>>+ goto err;
> >>>>>>>> }
> >>>>>>>> handle->entry = efi_reloc + opt->AddressOfEntryPoint;
> >>>>>>>> rel_size = opt->DataDirectory[rel_idx].Size;
> >>>>>>>>@@ -304,13 +725,16 @@ efi_status_t efi_load_pe(struct
> >>>>>>>>efi_loaded_image_obj *handle, void *efi,
> >>>>>>>> } else {
> >>>>>>>> printf("%s: Invalid optional header magic %x\n",
> >>>>>>>> __func__,
> >>>>>>>> nt->OptionalHeader.Magic);
> >>>>>>>>- return EFI_LOAD_ERROR;
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> /* Copy PE headers */
> >>>>>>>>- memcpy(efi_reloc, efi, sizeof(*dos) + sizeof(*nt)
> >>>>>>>>- + nt->FileHeader.SizeOfOptionalHeader
> >>>>>>>>- + num_sections * sizeof(IMAGE_SECTION_HEADER));
> >>>>>>>>+ memcpy(efi_reloc, efi,
> >>>>>>>>+ sizeof(*dos)
> >>>>>>>>+ + sizeof(*nt)
> >>>>>>>>+ + nt->FileHeader.SizeOfOptionalHeader
> >>>>>>>>+ + num_sections * sizeof(IMAGE_SECTION_HEADER));
> >>>>>>>>
> >>>>>>>> /* Load sections into RAM */
> >>>>>>>> for (i = num_sections - 1; i >= 0; i--) {
> >>>>>>>>@@ -327,7 +751,8 @@ efi_status_t efi_load_pe(struct
> >>>>>>>>efi_loaded_image_obj *handle, void *efi,
> >>>>>>>> (unsigned long)image_base) !=
> >>>>>>>> EFI_SUCCESS) {
> >>>>>>>> efi_free_pages((uintptr_t) efi_reloc,
> >>>>>>>> (virt_size + EFI_PAGE_MASK) >>
> >>>>>>>> EFI_PAGE_SHIFT);
> >>>>>>>>- return EFI_LOAD_ERROR;
> >>>>>>>>+ ret = EFI_LOAD_ERROR;
> >>>>>>>>+ goto err;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> /* Flush cache */
> >>>>>>>>@@ -340,4 +765,9 @@ efi_status_t efi_load_pe(struct
> >>>>>>>>efi_loaded_image_obj *handle, void *efi,
> >>>>>>>> loaded_image_info->image_size = virt_size;
> >>>>>>>>
> >>>>>>>> return EFI_SUCCESS;
> >>>>>>>>+
> >>>>>>>>+err:
> >>>>>>>>+ free(new_efi);
> >>>>>>>>+
> >>>>>>>>+ return ret;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >
>
