From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Mon, 27 Jan 2020 15:52:44 +0900 Subject: [PATCH v4 08/16] efi_loader: image_loader: support image authentication In-Reply-To: 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: <20200127065242.GD10165@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > >>>>>>>>--- > >>>>>>>> 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. 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; > >>>>>>>> } > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > > >