From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Thu, 23 Jan 2020 18:41:53 +0100 Subject: [PATCH v4 08/16] efi_loader: image_loader: support image authentication In-Reply-To: <20200122074228.GB10165@linaro.org> References: <20191218004512.24939-1-takahiro.akashi@linaro.org> <20191218004512.24939-9-takahiro.akashi@linaro.org> <57d363f6-2294-c9dd-ea2c-745fe8c74865@gmx.de> <20200117051136.GL28530@linaro.org> <20200121061245.GE8146@linaro.org> <3992a41c-8829-4636-cdbf-80b35dd96d35@gmx.de> <20200122011347.GH8146@linaro.org> <20200122074228.GB10165@linaro.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>>> --- >>>>>>>> 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 >>>>>>>> #include >>>>>>>> #include >>>>>>>> +#include >>>>>>>> >>>>>>>> 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 >>>>>>>> #include >>>>>>>> +#include >>>>>>>> #include >>>>>>>> +#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, Image was not loaded because the platform policy prohibits the image from being loaded. NULL is returned in *ImageHandle If you return SECURITY_VIOLATION, Image was loaded and an ImageHandle was created with a valid EFI_LOADED_IMAGE_PROTOCOL. 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; >>>>>>>> } >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >