Heinrich, I will address other comments in a separate e-mail, but it is worth mentioning that Patrick deserves credit for initial implementation of efi secure boot, which the main logic of my current patch set heavily relies on;
On Sat, Nov 16, 2019 at 09:00:12PM +0100, Heinrich Schuchardt wrote: > On 11/13/19 1:52 AM, AKASHI Takahiro wrote: > > >diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > >new file mode 100644 > >index 000000000000..04f43e47d1a3 > >--- /dev/null > >+++ b/lib/efi_loader/efi_signature.c > >@@ -0,0 +1,600 @@ > >+// SPDX-License-Identifier: GPL-2.0+ > >+/* > >+ * Copyright (c) 2018 Patrick Wildt <patr...@blueri.se> > > From which upstream did you take the code? The original code has never been upstreamed, and I took over his patch directly from him. Thanks, -Takahiro Akashi > >+ * Copyright (c) 2019 Linaro Limited, Author: AKASHI Takahiro > >+ */ > >+ > > debug() checks the value of _DEBUG. > Please, include common.h as first include. > > Use the sequence of includes as defined in > https://www.denx.de/wiki/U-Boot/CodingStyle > > >+#include <charset.h> > >+#include <efi_loader.h> > >+#include <image.h> > >+#include <hexdump.h> > >+#include <malloc.h> > >+#include <pe.h> > >+#include <linux/compat.h> > >+#include <linux/oid_registry.h> > >+#include <u-boot/rsa.h> > > Shouldn't we move all cryptography stuff into a directory crypto/? > > >+#include <u-boot/sha256.h> > >+/* > >+ * avoid duplicated inclusion: > >+ * #include "../lib/crypto/x509_parser.h" > >+ */ > >+#include "../lib/crypto/pkcs7_parser.h" > >+ > >+const efi_guid_t efi_guid_image_security_database = > >+ EFI_IMAGE_SECURITY_DATABASE_GUID; > >+const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID; > >+const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID; > >+const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID; > >+const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID; > >+ > >+#ifdef CONFIG_EFI_SECURE_BOOT > >+static const unsigned char WinIndirectSha256[] = { > > We don't use camel case. > > >+ 0x30, 0x33, 0x06, 0x0a, 0x2b, 0x06, 0x01, 0x04, 0x01, 0x82, 0x37, 0x02, > >+ 0x01, 0x0f, 0x30, 0x25, 0x03, 0x01, 0x00, 0xa0, 0x20, 0xa2, 0x1e, 0x80, > >+ 0x1c, 0x00, 0x3c, 0x00, 0x3c, 0x00, 0x3c, 0x00, 0x4f, 0x00, 0x62, 0x00, > >+ 0x73, 0x00, 0x6f, 0x00, 0x6c, 0x00, 0x65, 0x00, 0x74, 0x00, 0x65, 0x00, > >+ 0x3e, 0x00, 0x3e, 0x00, 0x3e, 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, > >+ 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05, 0x00, 0x04, 0x20 > >+}; > > What secret sauce is this? - Please, add comments where applicable. Add > references where needed for verification of correctness. > > Below you describe it as DER wrapper. So why call the variable Sha256? > > >+ > >+/** > >+ * efi_hash_regions - calculate a hash value > >+ * @regs: List of regions > >+ * @hash: Pointer to a pointer to buffer holding a hash value > >+ * @size: Size of buffer to be returned > >+ * > >+ * Calculate a sha256 value of @regs and return a value in @hash. > > Is this a hash of the concatenation of all regions - or is this a list > of hashes one for each regions? > > >+ * > >+ * Return: true on success, false on error > >+ */ > >+static bool efi_hash_regions(struct efi_image_regions *regs, void **hash, > >+ size_t *size) > > Why use void** for hash? There should be a structure for SHA256 hashes. > > >+{ > >+ *size = 0; > >+ *hash = calloc(1, SHA256_SUM_LEN); > >+ if (!*hash) { > >+ debug("Out of memory\n"); > >+ return false; > >+ } > >+ *size = SHA256_SUM_LEN; > >+ > >+ hash_calculate("sha256", regs->reg, regs->num, *hash); > >+#ifdef DEBUG > >+ debug("hash calculated:\n"); > > debug checks the value of _DEBUG not of DEBUG. So wouldn't you use > #ifdef _DEBUG to be consistent? > > >+ print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > >+ *hash, SHA256_SUM_LEN, false); > > This will fail to compile if CONFIG_HEXDUMP is not defined? > > >+#endif > >+ > >+ return true; > >+} > >+ > >+/** > >+ * efi_hash_regions_in_der - calculate a hash value > >+ * @regs: List of regions > >+ * @hash: Pointer to a pointer to buffer holding a hash value > >+ * @size: Size of buffer to be returned > >+ * > >+ * Calculate a sha256 value of @regs and return a value in @hash. > >+ * This function is a variant of efi_hash_regions(), and the data will > >+ * be wrapped with some header in DER format before hash calculation. > >+ * Message digest in signature of PE format will be calculated this way. > >+ * > >+ * Return: true on success, false on error > >+ */ > >+static bool efi_hash_regions_in_der(struct efi_image_regions *regs, void > >**hash, > >+ size_t *size) > >+{ > >+ void *msg; > >+ size_t msg_size; > >+ struct image_region regtmp[2]; > >+ > >+ if (!efi_hash_regions(regs, &msg, &msg_size)) { > >+ debug("Hash calculation failed\n"); > >+ return false; > >+ ; > >+ } > >+ > >+ *size = 0; > >+ *hash = calloc(1, SHA256_SUM_LEN); > >+ if (!*hash) { > >+ debug("Out of memory\n"); > >+ free(msg); > >+ return false; > >+ } > >+ *size = SHA256_SUM_LEN; > >+ > >+ /* File image hash is digested with some DER wrapper. */ > >+ regtmp[0].data = WinIndirectSha256; > >+ regtmp[0].size = sizeof(WinIndirectSha256); > >+ regtmp[1].data = msg; > >+ regtmp[1].size = msg_size; > >+ > >+ hash_calculate("sha256", regtmp, 2, *hash); > >+#ifdef DEBUG > >+ debug("hash calculated with extra header:\n"); > >+ print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > >+ *hash, SHA256_SUM_LEN, false); > >+#endif > >+ > >+ free(msg); > >+ > >+ return true; > >+} > >+ > >+/** > >+ * efi_signature_verify - verify a signature with a certificate > >+ * @regs: List of regions to be authenticated > >+ * @signed_info: Pointer to PKCS7's signed_info > >+ * @cert: x509 certificate > >+ * > >+ * Signature pointed to by @signed_info against image pointed to by @regs > >+ * is verified by a certificate pointed to by @cert. > >+ * @signed_info holds a signature, including a message digest which is to be > >+ * compared with a hash value calculated from @regs. > >+ * > >+ * Return: true if signature is verified, false if not > >+ */ > >+static bool efi_signature_verify(struct efi_image_regions *regs, > >+ struct pkcs7_signed_info *ps_info, > >+ struct x509_certificate *cert) > >+{ > >+ struct image_sign_info info; > >+ struct image_region regtmp[2]; > >+ void *hash; > >+ size_t size; > >+ char c; > >+ bool verified; > >+ > >+ debug("%s: Enter, %p, %p, %p(issuer: %s, subject: %s)\n", __func__, > >+ regs, ps_info, cert, cert->issuer, cert->subject); > >+ > >+ verified = false; > >+ > >+ memset(&info, '\0', sizeof(info)); > >+ info.padding = image_get_padding_algo("pkcs-1.5"); > >+ /* > >+ * Note: image_get_[checksum|crypto]_algo takes an string > >+ * argument like "<checksum>,<crypto>" > >+ * TODO: support other hash algorithms > >+ */ > >+ if (!strcmp(ps_info->sig->hash_algo, "sha1")) { > >+ info.checksum = image_get_checksum_algo("sha1,rsa2048"); > >+ info.name = "sha1,rsa2048"; > >+ } else if (!strcmp(ps_info->sig->hash_algo, "sha256")) { > >+ info.checksum = image_get_checksum_algo("sha256,rsa2048"); > >+ info.name = "sha256,rsa2048"; > >+ } else { > >+ debug("unknown msg digest algo: %s\n", ps_info->sig->hash_algo); > >+ goto out; > >+ } > >+ info.crypto = image_get_crypto_algo(info.name); > >+ > >+ info.key = cert->pub->key; > >+ info.keylen = cert->pub->keylen; > >+ > >+ /* verify signature */ > >+ debug("%s: crypto: %s, signature len:%x\n", __func__, > >+ info.name, ps_info->sig->s_size); > >+ if (ps_info->aa_set & (1UL << sinfo_has_message_digest)) { > >+ debug("%s: RSA verify authentication attribute\n", __func__); > >+ /* > >+ * NOTE: This path will be executed only for > >+ * PE image authentication > >+ */ > >+ > >+ /* check if hash matches digest first */ > >+ debug("checking msg digest first, len:0x%x\n", > >+ ps_info->msgdigest_len); > >+ > >+#ifdef DEBUG > >+ debug("hash in database:\n"); > >+ print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > >+ ps_info->msgdigest, ps_info->msgdigest_len, > >+ false); > >+#endif > >+ if (efi_hash_regions_in_der(regs, &hash, &size)) { > >+ if (ps_info->msgdigest_len != size || > >+ memcmp(hash, ps_info->msgdigest, size)) { > >+ debug("Digest doesn't match\n"); > >+ free(hash); > >+ goto out; > >+ } > >+ > >+ free(hash); > >+ } else { > >+ debug("Digesting image failed\n"); > >+ goto out; > >+ } > >+ > >+ /* against digest */ > >+ c = 0x31; > >+ regtmp[0].data = &c; > >+ regtmp[0].size = 1; > >+ regtmp[1].data = ps_info->authattrs; > >+ regtmp[1].size = ps_info->authattrs_len; > >+ > >+ if (!rsa_verify(&info, regtmp, 2, > >+ ps_info->sig->s, ps_info->sig->s_size)) > >+ verified = true; > >+ } else { > >+ debug("%s: RSA verify content data\n", __func__); > >+ /* against all data */ > >+ if (!rsa_verify(&info, regs->reg, regs->num, > >+ ps_info->sig->s, ps_info->sig->s_size)) > >+ verified = true; > >+ } > >+ > >+out: > >+ debug("%s: Exit, verified: %d\n", __func__, verified); > >+ return verified; > >+} > >+ > >+/** > >+ * efi_signature_verify_with_list - verify a signature with signature list > >+ * @regs: List of regions to be authenticated > >+ * @signed_info: Pointer to PKCS7's signed_info > >+ * @siglist: Signature list for certificates > >+ * @valid_cert: x509 certificate that verifies this signature > >+ * > >+ * Signature pointed to by @signed_info against image pointed to by @regs > >+ * is verified by signature list pointed to by @siglist. > >+ * Signature database is a simple concatenation of one or more > >+ * signature list(s). > >+ * > >+ * Return: true if signature is verified, false if not > >+ */ > >+static > >+bool efi_signature_verify_with_list(struct efi_image_regions *regs, > >+ struct pkcs7_signed_info *signed_info, > >+ struct efi_signature_store *siglist, > >+ struct x509_certificate **valid_cert) > >+{ > >+ struct x509_certificate *cert; > >+ struct efi_sig_data *sig_data; > >+ bool verified = false; > >+ > >+ debug("%s: Enter, %p, %p, %p, %p\n", __func__, > >+ regs, signed_info, siglist, valid_cert); > > Shouldn't we use EFI_PRINT() to get proper indentation? > > >+ > >+ if (!signed_info) { > >+ void *hash; > >+ size_t size; > >+ > >+ debug("%s: unsigned image\n", __func__); > > We already have indicated the function above. > > >+ /* > >+ * verify based on calculated hash value > >+ * TODO: support other hash algorithms > >+ */ > >+ if (guidcmp(&siglist->sig_type, &efi_guid_sha256)) { > >+ debug("Digest algorithm is not supported: %pUl\n", > >+ &siglist->sig_type); > >+ goto out; > >+ } > >+ > >+ if (!efi_hash_regions(regs, &hash, &size)) { > >+ debug("Digesting unsigned image failed\n"); > >+ goto out; > >+ } > >+ > >+ /* go through the list */ > >+ for (sig_data = siglist->sig_data_list; sig_data; > >+ sig_data = sig_data->next) { > >+#ifdef DEBUG > >+ debug("Msg digest in database:\n"); > >+ print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > >+ sig_data->data, sig_data->size, false); > > Does this compile if CONFIG_HEXDUMP is not defined? > > >+#endif > >+ if ((sig_data->size == size) && > >+ !memcmp(sig_data->data, hash, size)) { > >+ verified = true; > >+ free(hash); > >+ goto out; > >+ } > >+ } > >+ free(hash); > >+ goto out; > >+ } > >+ > >+ debug("%s: signed image\n", __func__); > >+ if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509)) { > >+ debug("Signature type is not supported: %pUl\n", > >+ &siglist->sig_type); > >+ goto out; > >+ } > >+ > >+ /* go through the list */ > >+ for (sig_data = siglist->sig_data_list; sig_data; > >+ sig_data = sig_data->next) { > >+ /* TODO: support owner check based on policy */ > >+ > >+ cert = x509_cert_parse(sig_data->data, sig_data->size); > >+ if (IS_ERR(cert)) { > >+ debug("Parsing x509 certificate failed\n"); > >+ goto out; > >+ } > >+ > >+ verified = efi_signature_verify(regs, signed_info, cert); > >+ > >+ if (verified) { > >+ if (valid_cert) > >+ *valid_cert = cert; > >+ else > >+ x509_free_certificate(cert); > >+ break; > >+ } else { > >+ x509_free_certificate(cert); > >+ } > >+ } > >+ > >+out: > >+ debug("%s: Exit, verified: %d\n", __func__, verified); > >+ return verified; > >+} > >+ > >+/** > >+ * efi_signature_verify_with_sigdb - verify a signature with db > >+ * @regs: List of regions to be authenticated > >+ * @msg: Signature > >+ * @db: Signature database for trusted certificates > >+ * @cert: x509 certificate that verifies this signature > >+ * > >+ * Signature pointed to by @msg against image pointed to by @regs > >+ * is verified by signature database pointed to by @db. > >+ * > >+ * Return: true if signature is verified, false if not > >+ */ > >+bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs, > >+ struct pkcs7_message *msg, > >+ struct efi_signature_store *db, > >+ struct x509_certificate **cert) > >+{ > >+ struct pkcs7_signed_info *info; > >+ struct efi_signature_store *siglist; > >+ bool verified = false; > >+ > >+ debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, cert); > >+ > >+ if (!db) > >+ goto out; > >+ > >+ if (!db->sig_data_list) > >+ goto out; > >+ > >+ /* for unsigned image */ > >+ if (!msg) { > >+ debug("%s: Verify unsigned image with db\n", __func__); > >+ for (siglist = db; siglist; siglist = siglist->next) > >+ if (efi_signature_verify_with_list(regs, NULL, > >+ siglist, cert)) { > >+ verified = true; > >+ goto out; > >+ } > >+ > >+ goto out; > >+ } > >+ > >+ /* for signed image or variable */ > >+ debug("%s: Verify signed image with db\n", __func__); > >+ for (info = msg->signed_infos; info; info = info->next) { > >+ debug("Signed Info: digest algo: %s, pkey algo: %s\n", > >+ info->sig->hash_algo, info->sig->pkey_algo); > >+ > >+ for (siglist = db; siglist; siglist = siglist->next) { > >+ if (efi_signature_verify_with_list(regs, info, > >+ siglist, cert)) { > >+ verified = true; > >+ goto out; > >+ } > >+ } > >+ } > >+ > >+out: > >+ debug("%s: Exit, verified: %d\n", __func__, verified); > >+ return verified; > >+} > >+ > >+/** > >+ * efi_search_siglist - search signature list for a certificate > >+ * @cert: x509 certificate > >+ * @siglist: Signature list > >+ * @revoc_time: Pointer to buffer for revocation time > >+ * > >+ * Search signature list pointed to by @siglist and find a certificate > >+ * pointed to by @cert. > >+ * If found, revocation time that is specified in signature database is > >+ * returned in @revoc_time. > >+ * > >+ * Return: true if certificate is found, false if not > >+ */ > >+static bool efi_search_siglist(struct x509_certificate *cert, > >+ struct efi_signature_store *siglist, > >+ time64_t *revoc_time) > >+{ > >+ struct image_region reg[1]; > >+ void *hash = NULL, *msg = NULL; > >+ struct efi_sig_data *sig_data; > >+ bool found = false; > >+ > >+ /* can be null */ > >+ if (!siglist->sig_data_list) > >+ return false; > >+ > >+ if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256)) { > >+ /* TODO: other hash algos */ > >+ debug("Certificate's digest type is not supported: %pUl\n", > >+ &siglist->sig_type); > >+ goto out; > >+ } > >+ > >+ /* calculate hash of TBSCertificate */ > >+ msg = calloc(1, SHA256_SUM_LEN); > >+ if (!msg) { > >+ debug("Out of memory\n"); > >+ goto out; > >+ } > >+ > >+ hash = calloc(1, SHA256_SUM_LEN); > >+ if (!hash) { > >+ debug("Out of memory\n"); > >+ goto out; > >+ } > >+ > >+ reg[0].data = cert->tbs; > >+ reg[0].size = cert->tbs_size; > >+ hash_calculate("sha256", reg, 1, msg); > >+ > >+ /* go through signature list */ > >+ for (sig_data = siglist->sig_data_list; sig_data; > >+ sig_data = sig_data->next) { > >+ /* > >+ * struct efi_cert_x509_sha256 { > >+ * u8 tbs_hash[256/8]; > >+ * time64_t revocation_time; > >+ * }; > >+ */ > >+ if ((sig_data->size == SHA256_SUM_LEN) && > >+ !memcmp(sig_data->data, hash, SHA256_SUM_LEN)) { > >+ memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN, > >+ sizeof(*revoc_time)); > >+ found = true; > >+ goto out; > >+ } > >+ } > >+ > >+out: > >+ free(hash); > >+ free(msg); > >+ > >+ return found; > >+} > >+ > >+/** > >+ * efi_signature_verify_cert - verify a certificate with dbx > >+ * @cert: x509 certificate > >+ * @dbx: Signature database > >+ * > >+ * Search signature database pointed to by @dbx and find a certificate > >+ * pointed to by @cert. > >+ * This function is expected to be used against "dbx". > >+ * > >+ * Return: true if a certificate is not rejected, false otherwise. > >+ */ > >+bool efi_signature_verify_cert(struct x509_certificate *cert, > >+ struct efi_signature_store *dbx) > >+{ > >+ struct efi_signature_store *siglist; > >+ time64_t revoc_time; > >+ bool found = false; > >+ > >+ debug("%s: Enter, %p, %p\n", __func__, dbx, cert); > > EFI_PRINT() ? > > >+ > >+ if (!cert) > >+ return false; > >+ > >+ for (siglist = dbx; siglist; siglist = siglist->next) { > >+ if (efi_search_siglist(cert, siglist, &revoc_time)) { > >+ /* TODO */ > >+ /* compare signing time with revocation time */ > > Is this a difficult thing to do? If not, please, fill the gap. > > Best regards > > Heinrich > > >+ > >+ found = true; > >+ break; > >+ } > >+ } > >+ > >+ debug("%s: Exit, verified: %d\n", __func__, !found); > >+ return !found; > >+} > >+ > >+/** > >+ * efi_signature_verify_signers - verify signers' certificates with dbx > >+ * @msg: Signature > >+ * @dbx: Signature database > >+ * > >+ * Determine if any of signers' certificates in @msg may be verified > >+ * by any of certificates in signature database pointed to by @dbx. > >+ * This function is expected to be used against "dbx". > >+ * > >+ * Return: true if none of certificates is rejected, false otherwise. > >+ */ > >+bool efi_signature_verify_signers(struct pkcs7_message *msg, > >+ struct efi_signature_store *dbx) > >+{ > >+ struct pkcs7_signed_info *info; > >+ bool found = false; > >+ > >+ debug("%s: Enter, %p, %p\n", __func__, msg, dbx); > >+ > >+ if (!msg) > >+ goto out; > >+ > >+ for (info = msg->signed_infos; info; info = info->next) { > >+ if (info->signer && > >+ !efi_signature_verify_cert(info->signer, dbx)) { > >+ found = true; > >+ goto out; > >+ } > >+ } > >+out: > >+ debug("%s: Exit, verified: %d\n", __func__, !found); > >+ return !found; > >+} > >+ > >+/** > >+ * efi_image_region_add - add an entry of region > >+ * @regs: Pointer to array of regions > >+ * @start: Start address of region > >+ * @end: End address of region > >+ * @nocheck: flag against overlapped regions > >+ * > >+ * Take one entry of region [@start, @end] and append it to the list > >+ * pointed to by @regs. If @nocheck is false, overlapping among entries > >+ * will be checked first. > >+ * > >+ * Return: 0 on success, status code (negative) on error > >+ */ > >+efi_status_t efi_image_region_add(struct efi_image_regions *regs, > >+ const void *start, const void *end, > >+ int nocheck) > >+{ > >+ struct image_region *reg; > >+ int i, j; > >+ > >+ if (regs->num >= EFI_REGS_MAX) { > >+ debug("%s: no more room for regions\n", __func__); > >+ return EFI_OUT_OF_RESOURCES; > >+ } > >+ > >+ if (end < start) > >+ return EFI_INVALID_PARAMETER; > >+ > >+ for (i = 0; i < regs->num; i++) { > >+ reg = ®s->reg[i]; > >+ if (nocheck) > >+ continue; > >+ > >+ if (start > reg->data + reg->size) > >+ continue; > >+ > >+ if ((start >= reg->data && start < reg->data + reg->size) || > >+ (end > reg->data && end < reg->data + reg->size)) { > >+ debug("%s: new region already part of another\n", > >+ __func__); > >+ return EFI_INVALID_PARAMETER; > >+ } > >+ > >+ if (start < reg->data && end < reg->data + reg->size) { > >+ for (j = regs->num - 1; j >= i; j--) > >+ memcpy(®s->reg[j], ®s->reg[j + 1], > >+ sizeof(*reg)); > >+ break; > >+ } > >+ } > >+ > >+ reg = ®s->reg[i]; > >+ reg->data = start; > >+ reg->size = end - start; > >+ regs->num++; > >+ > >+ return EFI_SUCCESS; > >+} > >+#endif /* CONFIG_EFI_SECURE_BOOT */ > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
