[PATCH v4 08/16] efi_loader: image_loader: support image authentication
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu Jan 23 18:41:53 CET 2020
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.akashi at 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. This is why we need the
EFI_IMAGE_SECURITY_DATABASE.
> 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;
>>>>>>>> }
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
More information about the U-Boot
mailing list