From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Thu, 13 May 2021 16:23:59 +0900 Subject: [PATCH 1/4] tools: mkeficapsule: add firmwware image signing In-Reply-To: <0686AB79-8431-43A2-8EF6-7853DD29524B@gmx.de> References: <20210512045753.62288-1-takahiro.akashi@linaro.org> <20210512045753.62288-2-takahiro.akashi@linaro.org> <20210513030839.GC16848@laputa> <6876a081-8f16-e747-6036-471b48f60318@gmx.de> <20210513065054.GF16848@laputa> <0686AB79-8431-43A2-8EF6-7853DD29524B@gmx.de> Message-ID: <20210513072359.GI16848@laputa> 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, May 13, 2021 at 08:55:17AM +0200, Heinrich Schuchardt wrote: > Am 13. Mai 2021 08:50:54 MESZ schrieb AKASHI Takahiro : > >On Thu, May 13, 2021 at 07:50:52AM +0200, Heinrich Schuchardt wrote: > >> On 5/13/21 7:12 AM, Masami Hiramatsu wrote: > >> > Hi Heinrich, > >> > > >> > 2021?5?13?(?) 13:22 Heinrich Schuchardt : > >> > > > >> > > On 5/13/21 5:08 AM, AKASHI Takahiro wrote: > >> > > > On Wed, May 12, 2021 at 10:56:41AM +0200, Heinrich Schuchardt > >wrote: > >> > > > > On 12.05.21 06:57, AKASHI Takahiro wrote: > >> > > > > > With this enhancement, mkeficapsule will be able to create > >a capsule > >> > > > > > file with a signature which will be verified later by FMP's > >SetImage(). > >> > > > > > > >> > > > > > We will have to specify addtional command parameters: > >> > > > > > -monotonic-cout : monotonic count > >> > > > > > -private-key : private key file > >> > > > > > -certificate : certificate file > >> > > > > > Only when those parameters are given, a signature will be > >added > >> > > > > > to a capsule file. > >> > > > > > > >> > > > > > Users are expected to maintain the monotonic count for each > >firmware > >> > > > > > image. > >> > > > > > > >> > > > > > Signed-off-by: AKASHI Takahiro > >> > > > > > --- > >> > > > > > tools/Makefile | 4 + > >> > > > > > tools/mkeficapsule.c | 324 > >+++++++++++++++++++++++++++++++++++++++---- > >> > > > > > 2 files changed, 303 insertions(+), 25 deletions(-) > >> > > > > > > >> > > > > > diff --git a/tools/Makefile b/tools/Makefile > >> > > > > > index d020c55d6644..02eae0286e20 100644 > >> > > > > > --- a/tools/Makefile > >> > > > > > +++ b/tools/Makefile > >> > > > > > @@ -231,6 +231,10 @@ hostprogs-$(CONFIG_MIPS) += > >mips-relocs > >> > > > > > hostprogs-$(CONFIG_ASN1_COMPILER) += asn1_compiler > >> > > > > > HOSTCFLAGS_asn1_compiler.o = -idirafter > >$(srctree)/include > >> > > > > > > >> > > > > > +ifneq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),) > >> > > > > > +HOSTLDLIBS_mkeficapsule += \ > >> > > > > > + $(shell pkg-config --libs libssl libcrypto 2> /dev/null > >|| echo "-lssl -lcrypto") > >> > > > > > >> > > > > I don't expect any user wants to install two tool versions in > >parallel. > >> > > > > > >> > > > > The tool should always be able to add a signature. > >> > > > > Adding a signature must be optional. > >> > > > > >> > > > It seems to me that those two statements mutually contradict. > >> > > > Or do you intend to say that we should have a separate kconfig > >> > > > option to enable/disable signing feature in mkeficapsule? > >> > > > > >> > > > If so, I can agree. > >> > > > > >> > > > In either way, we should have an option to turn on/off this > >functionality > >> > > > as not all users use signed capsules. > >> > > > >> > > I want to have a single binary to distribute with Linux distros > >(e.g. > >> > > Debian/Ubuntu package u-boot-tools). > >> > > >> > I couldn't catch your point. If so, the distros can build u-boot > >with > >> > CONFIG_EFI_CAPSULE_AUTHENTICATE=y... > >> > >> Why should the tool depend on board configuration? > >> Who would want capsule updates without authentication? > > > >I believe that there are bunch of users who don't need authentication > >on their own systems. > > > > They should think again. Why? > >> > > >> > BTW, IMHO, if u-boot.bin can not find the ESL in the device tree, > >> > it should skip authentication too. > >> > >> In this case the capsule should be rejected (if > >> CONFIG_EFI_CAPSULE_AUTHENTICATE=y). > > > >That's basically right. > >But as I mentioned in my comment against Sughosh's patch, > >the authentication process will be enforced only if the capsule has > >an attribute, IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED. > > > > That would be a security desaster. The requirement that I mentioned above is clearly described in UEFI specification. If you think that it is a disaster, please discuss the topic in UEFI Forum first. -Takahiro Akashi > Best regards > > Heinrich > > >I believe that this flag should be able to be specified and managed > >outside U-Boot configuration. > >So there can be a case where FW update is performed even if > >the key/certificate is not found in the device tree. > > > >> > > >> > Then, user can choose whether enabling capsule authentication or > >not > >> > by embedding ESL into their devicetree. > > > >Same comment above. > > > >-Takahiro Akashi > > > >> The user shall not be able to decide anything that might hamper > >> security. The U-Boot binary must dictate if a capsule is safe. > >> > >> Best regards > >> > >> Heinrich > >> > >> > > >> > Thank you > >> > > >> > > > >> > > This should allow both > >> > > > >> > > - create signed capsules > >> > > - create unsigned capsules > >> > > > >> > > The user shall select signing via command line parameters. > >> > > > >> > > Support for signing via the tool shall not depend on board > >Kconfig > >> > > parameters. > >> > > > >> > > Best regards > >> > > > >> > > Heinrich > >> > > > >> > > > > >> > > > > > +endif > >> > > > > > mkeficapsule-objs := mkeficapsule.o $(LIBFDT_OBJS) > >> > > > > > hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += > >mkeficapsule > >> > > > > > > >> > > > > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > >> > > > > > index de0a62898886..34ff1bdd82eb 100644 > >> > > > > > --- a/tools/mkeficapsule.c > >> > > > > > +++ b/tools/mkeficapsule.c > >> > > > > > @@ -18,7 +18,17 @@ > >> > > > > > #include > >> > > > > > #include > >> > > > > > > >> > > > > > -#include "fdt_host.h" > >> > > > > > +#include > >> > > > > > +#if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) > >> > > > > > >> > > > > see above > >> > > > > > >> > > > > > +#include > >> > > > > > +#include > >> > > > > > +#include > >> > > > > > +#include > >> > > > > > +#include > >> > > > > > +#include > >> > > > > > +#endif > >> > > > > > + > >> > > > > > +#include > >> > > > > > > >> > > > > > typedef __u8 u8; > >> > > > > > typedef __u16 u16; > >> > > > > > @@ -46,6 +56,13 @@ efi_guid_t efi_guid_image_type_uboot_fit > >= > >> > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID; > >> > > > > > efi_guid_t efi_guid_image_type_uboot_raw = > >> > > > > > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID; > >> > > > > > +efi_guid_t efi_guid_cert_type_pkcs7 = > >EFI_CERT_TYPE_PKCS7_GUID; > >> > > > > > + > >> > > > > > +#if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) > >> > > > > > >> > > > > see above > >> > > > > > >> > > > > > +static const char *opts_short = "f:r:i:I:v:D:K:P:C:m:dOh"; > >> > > > > > +#else > >> > > > > > +static const char *opts_short = "f:r:i:I:v:D:K:Oh"; > >> > > > > > +#endif > >> > > > > > > >> > > > > > static struct option options[] = { > >> > > > > > {"fit", required_argument, NULL, 'f'}, > >> > > > > > @@ -54,6 +71,12 @@ static struct option options[] = { > >> > > > > > {"instance", required_argument, NULL, 'I'}, > >> > > > > > {"dtb", required_argument, NULL, 'D'}, > >> > > > > > {"public key", required_argument, NULL, 'K'}, > >> > > > > > +#if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) > >> > > > > > + {"private-key", required_argument, NULL, 'P'}, > >> > > > > > + {"certificate", required_argument, NULL, 'C'}, > >> > > > > > + {"monotonic-count", required_argument, NULL, 'm'}, > >> > > > > > >> > > > > These options should not be required. > >> > > > > >> > > > I don't get you. What do you mean? > >> > > > > >> > > > > > + {"dump-sig", no_argument, NULL, 'd'}, > >> > > > > > +#endif > >> > > > > > {"overlay", no_argument, NULL, 'O'}, > >> > > > > > {"help", no_argument, NULL, 'h'}, > >> > > > > > {NULL, 0, NULL, 0}, > >> > > > > > @@ -70,6 +93,12 @@ static void print_usage(void) > >> > > > > > "\t-I, --instance update hardware > >instance\n" > >> > > > > > "\t-K, --public-key public key esl > >file\n" > >> > > > > > "\t-D, --dtb dtb file\n" > >> > > > > > +#if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) > >> > > > > > >> > > > > see above > >> > > > > > >> > > > > > + "\t-P, --private-key private key > >file\n" > >> > > > > > + "\t-C, --certificate signer's > >certificate file\n" > >> > > > > > + "\t-m, --monotonic-count monotonic > >count\n" > >> > > > > > + "\t-d, --dump_sig dump signature > >(*.p7)\n" > >> > > > > > +#endif > >> > > > > > "\t-O, --overlay the dtb file is > >an overlay\n" > >> > > > > > "\t-h, --help print a help > >message\n", > >> > > > > > tool_name); > >> > > > > > @@ -249,12 +278,167 @@ err: > >> > > > > > return ret; > >> > > > > > } > >> > > > > > > >> > > > > > +struct auth_context { > >> > > > > > + char *key_file; > >> > > > > > + char *cert_file; > >> > > > > > + u8 *image_data; > >> > > > > > + size_t image_size; > >> > > > > > + struct efi_firmware_image_authentication auth; > >> > > > > > + u8 *sig_data; > >> > > > > > + size_t sig_size; > >> > > > > > +}; > >> > > > > > + > >> > > > > > +static int dump_sig; > >> > > > > > + > >> > > > > > +#if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) > >> > > > > > >> > > > > see above > >> > > > > > >> > > > > > +static EVP_PKEY *fileio_read_pkey(const char *filename) > >> > > > > > +{ > >> > > > > > + EVP_PKEY *key = NULL; > >> > > > > > + BIO *bio; > >> > > > > > + > >> > > > > > + bio = BIO_new_file(filename, "r"); > >> > > > > > + if (!bio) > >> > > > > > + goto out; > >> > > > > > + > >> > > > > > + key = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL); > >> > > > > > + > >> > > > > > +out: > >> > > > > > + BIO_free_all(bio); > >> > > > > > + if (!key) { > >> > > > > > + printf("Can't load key from file '%s'\n", > >filename); > >> > > > > > >> > > > > Please, you use fprintf(stderr,) for error messages. > >> > > > > > >> > > > > > + ERR_print_errors_fp(stderr); > >> > > > > > + } > >> > > > > > + > >> > > > > > + return key; > >> > > > > > +} > >> > > > > > + > >> > > > > > +static X509 *fileio_read_cert(const char *filename) > >> > > > > > +{ > >> > > > > > + X509 *cert = NULL; > >> > > > > > + BIO *bio; > >> > > > > > + > >> > > > > > + bio = BIO_new_file(filename, "r"); > >> > > > > > + if (!bio) > >> > > > > > + goto out; > >> > > > > > + > >> > > > > > + cert = PEM_read_bio_X509(bio, NULL, NULL, NULL); > >> > > > > > + > >> > > > > > +out: > >> > > > > > + BIO_free_all(bio); > >> > > > > > + if (!cert) { > >> > > > > > + printf("Can't load certificate from file > >'%s'\n", filename); > >> > > > > > >> > > > > fprintf(stderr,) > >> > > > > > >> > > > > > + ERR_print_errors_fp(stderr); > >> > > > > > + } > >> > > > > > + > >> > > > > > + return cert; > >> > > > > > +} > >> > > > > > + > >> > > > > > +static int create_auth_data(struct auth_context *ctx) > >> > > > > > +{ > >> > > > > > + EVP_PKEY *key = NULL; > >> > > > > > + X509 *cert = NULL; > >> > > > > > + BIO *data_bio = NULL; > >> > > > > > + const EVP_MD *md; > >> > > > > > + PKCS7 *p7; > >> > > > > > + int flags, ret = -1; > >> > > > > > + > >> > > > > > + OpenSSL_add_all_digests(); > >> > > > > > + OpenSSL_add_all_ciphers(); > >> > > > > > + ERR_load_crypto_strings(); > >> > > > > > + > >> > > > > > + key = fileio_read_pkey(ctx->key_file); > >> > > > > > + if (!key) > >> > > > > > + goto err; > >> > > > > > + cert = fileio_read_cert(ctx->cert_file); > >> > > > > > + if (!cert) > >> > > > > > + goto err; > >> > > > > > + > >> > > > > > + /* > >> > > > > > + * create a BIO, containing: > >> > > > > > + * * firmware image > >> > > > > > + * * monotonic count > >> > > > > > + * in this order! > >> > > > > > + * See EDK2's FmpAuthenticatedHandlerRsa2048Sha256() > >> > > > > > + */ > >> > > > > > + data_bio = BIO_new(BIO_s_mem()); > >> > > > > > + BIO_write(data_bio, ctx->image_data, ctx->image_size); > >> > > > > > + BIO_write(data_bio, &ctx->auth.monotonic_count, > >> > > > > > + sizeof(ctx->auth.monotonic_count)); > >> > > > > > + > >> > > > > > + md = EVP_get_digestbyname("SHA256"); > >> > > > > > + if (!md) > >> > > > > > + goto err; > >> > > > > > + > >> > > > > > + /* create signature */ > >> > > > > > + /* TODO: maybe add PKCS7_NOATTR and PKCS7_NOSMIMECAP */ > >> > > > > > >> > > > > PKCS7_NOATTR is a value without any documentation in the > >code. > >> > > > > >> > > > Nak. > >> > > > Those macros are part of openssl library. See openssl/pkcs7.h. > >> > > > > >> > > > > Please, replace variable names by a long text describing what > >it missing. > >> > > > > > >> > > > > > + flags = PKCS7_BINARY | PKCS7_DETACHED; > >> > > > > > >> > > > > Those constants lack documentation in the code. > >> > > > > >> > > > Nak again. > >> > > > > >> > > > > > + p7 = PKCS7_sign(NULL, NULL, NULL, data_bio, flags | > >PKCS7_PARTIAL); > >> > > > > > + if (!p7) > >> > > > > > + goto err; > >> > > > > > + if (!PKCS7_sign_add_signer(p7, cert, key, md, flags)) > >> > > > > > + goto err; > >> > > > > > + if (!PKCS7_final(p7, data_bio, flags)) > >> > > > > > + goto err; > >> > > > > > + > >> > > > > > + /* convert pkcs7 into DER */ > >> > > > > > + ctx->sig_data = NULL; > >> > > > > > + ctx->sig_size = ASN1_item_i2d((ASN1_VALUE *)p7, > >&ctx->sig_data, > >> > > > > > + ASN1_ITEM_rptr(PKCS7)); > >> > > > > > + if (!ctx->sig_size) > >> > > > > > + goto err; > >> > > > > > + > >> > > > > > + /* fill auth_info */ > >> > > > > > + ctx->auth.auth_info.hdr.dwLength = > >sizeof(ctx->auth.auth_info) > >> > > > > > + + > >ctx->sig_size; > >> > > > > > + ctx->auth.auth_info.hdr.wRevision = > >WIN_CERT_REVISION_2_0; > >> > > > > > + ctx->auth.auth_info.hdr.wCertificateType = > >WIN_CERT_TYPE_EFI_GUID; > >> > > > > > + memcpy(&ctx->auth.auth_info.cert_type, > >&efi_guid_cert_type_pkcs7, > >> > > > > > + sizeof(efi_guid_cert_type_pkcs7)); > >> > > > > > + > >> > > > > > + ret = 0; > >> > > > > > +err: > >> > > > > > + BIO_free_all(data_bio); > >> > > > > > + EVP_PKEY_free(key); > >> > > > > > + X509_free(cert); > >> > > > > > + > >> > > > > > + return ret; > >> > > > > > +} > >> > > > > > + > >> > > > > > +static int dump_signature(const char *path, u8 *signature, > >size_t sig_size) > >> > > > > > +{ > >> > > > > > + char *sig_path; > >> > > > > > + FILE *f; > >> > > > > > + size_t size; > >> > > > > > + int ret = -1; > >> > > > > > + > >> > > > > > + sig_path = malloc(strlen(path) + 3 + 1); > >> > > > > > + if (!sig_path) > >> > > > > > + return ret; > >> > > > > > + > >> > > > > > + sprintf(sig_path, "%s.p7", path); > >> > > > > > + f = fopen(sig_path, "w"); > >> > > > > > + if (!f) > >> > > > > > + goto err; > >> > > > > > + > >> > > > > > + size = fwrite(signature, 1, sig_size, f); > >> > > > > > + if (size == sig_size) > >> > > > > > + ret = 0; > >> > > > > > + > >> > > > > > + fclose(f); > >> > > > > > +err: > >> > > > > > + free(sig_path); > >> > > > > > + return ret; > >> > > > > > +} > >> > > > > > +#endif > >> > > > > > + > >> > > > > > static int create_fwbin(char *path, char *bin, > >efi_guid_t *guid, > >> > > > > > - unsigned long index, unsigned long > >instance) > >> > > > > > + unsigned long index, unsigned long > >instance, > >> > > > > > + uint64_t mcount, char *privkey_file, > >char *cert_file) > >> > > > > > { > >> > > > > > struct efi_capsule_header header; > >> > > > > > struct efi_firmware_management_capsule_header capsule; > >> > > > > > struct efi_firmware_management_capsule_image_header > >image; > >> > > > > > + struct auth_context auth_context; > >> > > > > > FILE *f, *g; > >> > > > > > struct stat bin_stat; > >> > > > > > u8 *data; > >> > > > > > @@ -266,6 +450,7 @@ static int create_fwbin(char *path, > >char *bin, efi_guid_t *guid, > >> > > > > > printf("\tbin: %s\n\ttype: %pUl\n", bin, guid); > >> > > > > > printf("\tindex: %ld\n\tinstance: %ld\n", index, > >instance); > >> > > > > > #endif > >> > > > > > + auth_context.sig_size = 0; > >> > > > > > > >> > > > > > g = fopen(bin, "r"); > >> > > > > > if (!g) { > >> > > > > > @@ -281,11 +466,36 @@ static int create_fwbin(char *path, > >char *bin, efi_guid_t *guid, > >> > > > > > printf("cannot allocate memory: %zx\n", > >(size_t)bin_stat.st_size); > >> > > > > > goto err_1; > >> > > > > > } > >> > > > > > - f = fopen(path, "w"); > >> > > > > > - if (!f) { > >> > > > > > - printf("cannot open %s\n", path); > >> > > > > > + > >> > > > > > + size = fread(data, 1, bin_stat.st_size, g); > >> > > > > > + if (size < bin_stat.st_size) { > >> > > > > > + printf("read failed (%zx)\n", size); > >> > > > > > goto err_2; > >> > > > > > } > >> > > > > > + > >> > > > > > + /* first, calculate signature to determine its size */ > >> > > > > > +#if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) > >> > > > > > >> > > > > see above > >> > > > > > >> > > > > > + if (privkey_file && cert_file) { > >> > > > > > + auth_context.key_file = privkey_file; > >> > > > > > + auth_context.cert_file = cert_file; > >> > > > > > + auth_context.auth.monotonic_count = mcount; > >> > > > > > + auth_context.image_data = data; > >> > > > > > + auth_context.image_size = bin_stat.st_size; > >> > > > > > + > >> > > > > > + if (create_auth_data(&auth_context)) { > >> > > > > > + printf("Signing firmware image > >failed\n"); > >> > > > > > + goto err_3; > >> > > > > > + } > >> > > > > > + > >> > > > > > + if (dump_sig && > >> > > > > > + dump_signature(path, auth_context.sig_data, > >> > > > > > + auth_context.sig_size)) { >