From mboxrd@z Thu Jan  1 00:00:00 1970
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Thu, 13 May 2021 16:38:05 +0900
Subject: [PATCH 1/4] tools: mkeficapsule: add firmwware image signing
In-Reply-To: <9E142C4F-7B38-4DCE-B557-4847AB70C638@gmx.de>
References: <20210512045753.62288-1-takahiro.akashi@linaro.org>
 <20210512045753.62288-2-takahiro.akashi@linaro.org>
 <c6b21901-15e0-2b24-22a2-5e4b4e293311@gmx.de>
 <20210513030839.GC16848@laputa>
 <6876a081-8f16-e747-6036-471b48f60318@gmx.de>
 <CAA93ih3rhuWwEfn1GbqPnXc0zZcSe1pw_zc2mX5dP6Hstng4Dw@mail.gmail.com>
 <b25e1fbf-655e-6ed0-5bf5-77714cf48e41@gmx.de>
 <CAA93ih1rJ-4NacYjdoGLO_x7M1DD=qQAGBTdo-hEMJtYmWQrxw@mail.gmail.com>
 <9E142C4F-7B38-4DCE-B557-4847AB70C638@gmx.de>
Message-ID: <20210513073805.GJ16848@laputa>
List-Id: <u-boot.lists.denx.de>
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:52:18AM +0200, Heinrich Schuchardt wrote:
> Am 13. Mai 2021 08:44:24 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>:
> >Hi Heinrich,
> >
> >2021?5?13?(?) 14:50 Heinrich Schuchardt <xypron.glpk@gmx.de>:
> >>
> >> On 5/13/21 7:12 AM, Masami Hiramatsu wrote:
> >> > Hi Heinrich,
> >> >
> >> > 2021?5?13?(?) 13:22 Heinrich Schuchardt <xypron.glpk@gmx.de>:
> >> >>
> >> >> 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 <count> : monotonic count
> >> >>>>>     -private-key <private key file> : private key file
> >> >>>>>     -certificate <certificate file> : 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 <takahiro.akashi@linaro.org>
> >> >>>>> ---
> >> >>>>>    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?
> >
> >Yeah, at this point I agreed. I think there should be a separated
> >CONFIG
> >for tools

I prefer this.

>  or forcibly link those libraries. (I think most people don't
> >mind if it requires new libraries to be built, that usually happens.)
> >
> >> Who would want capsule updates without authentication?
> >
> >Hm, so you think even CONFIG_EFI_CAPSULE_AUTHENTICATE is
> >only for development. Capsule must be signed, right?
> >Then, all distro should build u-boot with
> >CONFIG_EFI_CAPSULE_AUTHENTICATE=y, isn't it?
> 
> There will still be U-Boot images without capsule updates.

Users are not always distro.
There may be users who want to use capsule updates for their
own systems that they manage by themselves.

> >
> >> > 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).
> >
> >I meant U-Boot has NO key to authenticate the capsule. I think in that
> >case U-Boot build process must require the key (ESL) and if user
> >doesn't
> >provide it, the build should fail (if it doesn't skip capsule
> >authentication.)
> >Or, we have no way to update U-Boot anymore.
> >
> >> > Then, user can choose whether enabling capsule authentication or
> >not
> >> > by embedding ESL into their devicetree.
> >>
> >> The user shall not be able to decide anything that might hamper
> >> security. The U-Boot binary must dictate if a capsule is safe.
> >
> >Hmm, I think the root issue is that the ESL embedding process is not
> >integrated into the build process yet.

Sughosh has already submitted the patch:
https://lists.denx.de/pipermail/u-boot/2021-April/447183.html

My patch is based on top of that.

> For the safe capsule update,
> >we must enable capsule authentication with keys. (unsafe one is only
> >for testing/development)
> >Moreover, the key is stored in the U-Boot itself OR, in the secure
> >storage
> >outside of U-Boot (Hardware OTP or TPM/HSM are preferable.)
> >
> >Thus,
> >CONFIG_EFI_CAPSULE_AUTHENTICATE must depend on (or select)
> >a new config which points the path for the ESL file (this is embedded
> >while
> >the build process), OR, the platform driver provides key from its
> >hardware
> >secure storage.
> >
> >What would you think about this idea?
> 
> That is the direction I would like to go.

Which part of Masami's comment do you intend to agree to?
Sugosh's approach is the one that you and Sughosh has agreed
in the past discussion. Do you want to revert it?

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> >Thank you,
> >
> >>
> >> 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 <sys/stat.h>
> >> >>>>>    #include <sys/types.h>
> >> >>>>>
> >> >>>>> -#include "fdt_host.h"
> >> >>>>> +#include <linux/kconfig.h>
> >> >>>>> +#if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> >> >>>>
> >> >>>> see above
> >> >>>>
> >> >>>>> +#include <openssl/asn1.h>
> >> >>>>> +#include <openssl/bio.h>
> >> >>>>> +#include <openssl/evp.h>
> >> >>>>> +#include <openssl/err.h>
> >> >>>>> +#include <openssl/pem.h>
> >> >>>>> +#include <openssl/pkcs7.h>
> >> >>>>> +#endif
> >> >>>>> +
> >> >>>>> +#include <linux/libfdt.h>
> >> >>>>>
> >> >>>>>    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 <instance>   update hardware
> >instance\n"
> >> >>>>>             "\t-K, --public-key <key file> public key esl
> >file\n"
> >> >>>>>             "\t-D, --dtb <dtb file>        dtb file\n"
> >> >>>>> +#if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> >> >>>>
> >> >>>> see above
> >> >>>>
> >> >>>>> +          "\t-P, --private-key <privkey file>  private key
> >file\n"
> >> >>>>> +          "\t-C, --certificate <cert file>     signer's
> >certificate file\n"
> >> >>>>> +          "\t-m, --monotonic-count <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);
>