[U-Boot] [PATCH v3 4/6] lib: rsa: generate additional parameters for public key

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Simon,

On Tue, Nov 19, 2019 at 06:59:59PM -0800, Simon Glass wrote:
> Hi Takahiro,
> 
> On Tue, 12 Nov 2019 at 16:47, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > In the current implementation of FIT_SIGNATURE, five parameters for
> > a RSA public key are required while only two of them are essential.
> > (See rsa-mod-exp.h and uImage.FIT/signature.txt)
> > This is a result of considering relatively limited computer power
> > and resources on embedded systems, while such a assumption may not
> > be quite practical for other use cases.
> >
> > In this patch, added is a function, rsa_gen_key_prop(), which will
> > generate additional parameters for other uses, in particular
> > UEFI secure boot, on the fly.
> >
> > Note: the current code uses some "big number" routines from BearSSL
> > for the calculation.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/u-boot/rsa-mod-exp.h |  21 +
> >  lib/rsa/Kconfig              |   1 +
> >  lib/rsa/Makefile             |   1 +
> >  lib/rsa/rsa-keyprop.c        | 717 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 740 insertions(+)
> >  create mode 100644 lib/rsa/rsa-keyprop.c
> >
> > diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
> > index 8a428c4b6a1a..374169b8304e 100644
> > --- a/include/u-boot/rsa-mod-exp.h
> > +++ b/include/u-boot/rsa-mod-exp.h
> > @@ -26,6 +26,27 @@ struct key_prop {
> >         uint32_t exp_len;       /* Exponent length in number of uint8_t */
> >  };
> >
> > +/**
> > + * rsa_gen_key_prop() - Generate key properties of RSA public key
> > + * @key:       Specifies key data in DER format
> > + * @keylen:    Length of @key
> > + *
> > + * This function takes a blob of encoded RSA public key data in DER
> > + * format, parse it and generate all the relevant properties
> > + * in key_prop structure.
> > + *
> > + * Return:     Pointer to struct key_prop on success, NULL on error
> > + */
> > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> > +
> > +/**
> > + * rsa_free_key_prop() - Free key properties
> > + * @prop:      Pointer to struct key_prop
> > + *
> > + * This function frees all the memories allocated by rsa_gen_key_prop().
> > + */
> > +void rsa_free_key_prop(struct key_prop *prop);
> > +
> >  /**
> >   * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
> >   *
> > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > index 71e4c06bf883..d1d6f6cf64a3 100644
> > --- a/lib/rsa/Kconfig
> > +++ b/lib/rsa/Kconfig
> > @@ -33,6 +33,7 @@ config RSA_VERIFY
> >  config RSA_VERIFY_WITH_PKEY
> >         bool "Execute RSA verification without key parameters from FDT"
> >         depends on RSA
> > +       imply RSA_PUBLIC_KEY_PARSER
> >         help
> >           The standard RSA-signature verification code (FIT_SIGNATURE) uses
> >           pre-calculated key properties, that are stored in fdt blob, in
> > diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> > index c07305188e0c..14ed3cb4012b 100644
> > --- a/lib/rsa/Makefile
> > +++ b/lib/rsa/Makefile
> > @@ -6,4 +6,5 @@
> >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> >
> >  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> > +obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> >  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> > diff --git a/lib/rsa/rsa-keyprop.c b/lib/rsa/rsa-keyprop.c
> > new file mode 100644
> > index 000000000000..9458337cc608
> > --- /dev/null
> > +++ b/lib/rsa/rsa-keyprop.c
> > @@ -0,0 +1,717 @@
> > +// SPDX-License-Identifier: GPL-2.0+ and MIT
> > +/*
> > + * RSA library - generate parameters for a public key
> > + *
> > + * Copyright (c) 2019 Linaro Limited
> > + * Author: AKASHI Takahiro
> > + *
> > + * Big number routines in this file come from BearSSL:
> 
> Can we put these in their own file if we expect to update them? Really
> should be called by the original filename and go in lib/bearsll.

Honestly, I'd rather disagree with you because
* in the original code, each file has only a single function, then
  we will have to introduce bunch of small source files.
* it is quite unlikely for other components to re-use some of
  those functions in the future.
So splitting the file would practically make little use.

> [..]
> 
> > +/**
> > + * rsa_gen_key_prop() - Generate key properties of RSA public key
> > + * @key:       Specifies key data in DER format
> > + * @keylen:    Length of @key
> > + *
> > + * This function takes a blob of encoded RSA public key data in DER
> > + * format, parse it and generate all the relevant properties
> > + * in key_prop structure.
> > + *
> > + * Return:     Pointer to struct key_prop on success, NULL on error
> > + */
> > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen)
> > +{
> > +       struct key_prop *prop;
> > +       struct rsa_key rsa_key;
> > +#define BR_MAX_RSA_SIZE 4096
> 
> const int?

Will change it to
        const int max_rsa_size = 4096;

> > +       uint32_t *n, *rr, *rrtmp;
> > +       int rlen, i, ret;
> > +
> > +       prop = calloc(sizeof(*prop), 1);
> > +       if (!prop)
> > +               return NULL;
> > +       n = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
> > +       rr = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
> > +       rrtmp = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
> > +       if (!n || !rr || !rrtmp)
> > +               return NULL;
> 
> This can cause memory leak and will likely generate a coverity warning.

Okay, will fix it.

> > +
> > +       ret = rsa_parse_pub_key(&rsa_key, key, keylen);
> > +       if (ret)
> > +               goto err;
> > +
> > +       /* modulus */
> > +       /* removing leading 0's */
> > +       for (i = 0; i < rsa_key.n_sz && !rsa_key.n[i]; i++)
> > +               ;
> > +       prop->num_bits = (rsa_key.n_sz - i) * 8;
> > +       prop->modulus = malloc(rsa_key.n_sz - i);
> > +       if (!prop->modulus)
> > +               goto err;
> > +       memcpy((void *)prop->modulus, &rsa_key.n[i], rsa_key.n_sz - i);
> > +
> > +       /* exponent */
> > +       prop->public_exponent = calloc(1, sizeof(uint64_t));
> > +       if (!prop->public_exponent)
> > +               goto err;
> > +       memcpy((void *)prop->public_exponent + sizeof(uint64_t) - rsa_key.e_sz,
> > +              rsa_key.e, rsa_key.e_sz);
> > +       prop->exp_len = rsa_key.e_sz;
> > +
> > +       /* n0 inverse */
> > +       br_i32_decode(n, &rsa_key.n[i], rsa_key.n_sz - i);
> > +       prop->n0inv = br_i32_ninv32(n[1]);
> > +
> > +       /* R^2 mod n; R = 2^(num_bits) */
> > +       rlen = prop->num_bits * 2; /* #bits of R^2 = (2^num_bits)^2 */
> > +       rr[0] = 0;
> > +       *(uint8_t *)&rr[0] = (1 << (rlen % 8));
> > +       for (i = 1; i < (((rlen + 31) >> 5) + 1); i++)
> > +               rr[i] = 0;
> > +       br_i32_decode(rrtmp, rr, ((rlen + 7) >> 3) + 1);
> > +       br_i32_reduce(rr, rrtmp, n);
> > +
> > +       rlen = (prop->num_bits + 7) >> 3; /* #bytes of R^2 mod n */
> > +       prop->rr = malloc(rlen);
> > +       if (!prop->rr)
> > +               goto err;
> > +       br_i32_encode((void *)prop->rr, rlen, rr);
> > +
> > +       return prop;
> > +
> > +err:
> > +       free(n);
> > +       free(rr);
> > +       free(rrtmp);
> > +       rsa_free_key_prop(prop);
> > +       return NULL;
> 
> This should return -ENOMEM if we are out of memory. The caller cannot
> divine what went wrong.

Agree.

Thank you for your review,
-Takahiro Akashi


> > +}
> > --
> > 2.21.0
> >
> 
> Regards,
> Simon