[PATCH 1/4] tools: mkeficapsule: add firmwware image signing
AKASHI Takahiro
takahiro.akashi at linaro.org
Thu May 13 09:45:40 CEST 2021
On Thu, May 13, 2021 at 08:45:10AM +0200, Heinrich Schuchardt wrote:
> Am 13. Mai 2021 08:36:05 MESZ schrieb AKASHI Takahiro <takahiro.akashi at linaro.org>:
> >On Thu, May 13, 2021 at 07:35:36AM +0200, Heinrich Schuchardt wrote:
> >> On 5/13/21 7:00 AM, AKASHI Takahiro wrote:
> >> > On Thu, May 13, 2021 at 06:22:39AM +0200, Heinrich Schuchardt
> >wrote:
> >> > > 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 at 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).
> >> > >
> >> > > 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.
> >> >
> >> > That is why I proposed that we create a new kconfig option.
> >>
> >> What do you want to configure? Signing shall always be enabled in
> >> mkeficapsule.
> >
> >I don't think so.
>
> Capsule updates without authentication should never be rolled out in production for security reasons.
It's up to the providers.
(In this case, providers are not always distro.)
We should provide mechanism, but not enforce policy.
> >
> >> >
> >> > Please note that enabling signing feature in mkeficapsule
> >> > requires openssl library, and we should not enforce users who don't
> >> > need this feature to install an unnecessary package.
> >>
> >> Why? There are dozens of other packages depending on OpenSSL on a
> >> developer's machine.
> >
> >We don't expect all users have openssl-related packages on their
> >desktop.
>
> We are not talking about users but developers here.
There is not distinct difference between users and developpers.
> I haven't seen a Linux distro without an OpenSSL package. The package management system will pull it in when u-boot-tools is installed.
Distro may have its own policy, that's fine.
We will only want to provide the mechanism with which
they want to build their own distro.
-Takahiro Akashi
>
> Best regards
>
> Heinrich
>
> >
> >-Takahiro Akashi
> >
> >
> >> Best regards
> >>
> >> Heinrich
> >>
> >> >
> >> > -Takahiro Akashi
> >> >
> >> > > 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);
> >> > > > > > 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)) {
> >> > > > > > + printf("Creating signature file failed\n");
> >> > > > > > + goto err_3;
> >> > > > > > + }
> >> > > > > > + }
> >> > > > > > +#endif
> >> > > > > > +
> >> > > > > > header.capsule_guid = efi_guid_fm_capsule;
> >> > > > > > header.header_size = sizeof(header);
> >> > > > > > /* TODO: The current implementation ignores flags */
> >> > > > > > @@ -294,11 +504,20 @@ static int create_fwbin(char *path,
> >char *bin, efi_guid_t *guid,
> >> > > > > > + sizeof(capsule) + sizeof(u64)
> >> > > > > > + sizeof(image)
> >> > > > > > + bin_stat.st_size;
> >> > > > > > + if (auth_context.sig_size)
> >> > > > > > + header.capsule_image_size += sizeof(auth_context.auth)
> >> > > > > > + + auth_context.sig_size;
> >> > > > > > +
> >> > > > > > + f = fopen(path, "w");
> >> > > > > > + if (!f) {
> >> > > > > > + printf("cannot open %s\n", path);
> >> > > > > > + goto err_3;
>
More information about the U-Boot
mailing list