[PATCH RFC v2 3/5] lib: Add support for ECDSA image signing

[Simon Glass]

Hi Alex,

On Thu, 7 Jan 2021 at 09:27, Alex G. <mr.nuke.me at gmail.com> wrote:
>
> On 1/7/21 6:35 AM, Simon Glass wrote:
> > Hi Alexandru,
>
> Hi Simon,
>
> (pun alert!) A lot of your comments have to do with comments. I use
> comments as a tool to add something of value to code. When the code is
> self-documenting, comments don't help much.
> See kernel coding style chapter 8.
>
> What comments can do, is increase code size and break code flow -- there
> is a cost to having them. I'm certain you've seen functions that need to
> be scrolled up and down because of a huge comment smack in the middle.
> I'll address individual comment comments below.
>

Don't get me started on comments in the kernel...there seems to almost
be a ban on them :-)

We used to follow the same approach but now we have comments for
non-trivial code. Comments and tests are closely related.

- if there is no comment, we don't know what the function is supposed
to do so we can't change it (there is no contract), we are not sure if
line 5 is a bug or a real quirk, casual readers can't understand what
is going on, the automated docs don't produce anything, no one wants
to refactor it, etc.
- if there is no test, presumably the code doesn't work now, if it ever did

> >
> > On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me at gmail.com> wrote:
> >>
> >> mkimage supports rsa2048, and rsa4096 signatures. With newer silicon
> >> now supporting hardware-accelerated ECDSA, it makes sense to expand
> >> signing support to elliptic curves.
> >>
> >> Implement host-side ECDSA signing and verification with libcrypto.
> >> Device-side implementation of signature verification is beyond the
> >> scope of this patch.
> >>
> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me at gmail.com>
> >> ---
> >>   common/image-sig.c          |  14 +-
> >>   include/u-boot/ecdsa.h      |  27 ++++
> >>   lib/ecdsa/ecdsa-libcrypto.c | 300 ++++++++++++++++++++++++++++++++++++
> >>   tools/Makefile              |   3 +
> >>   4 files changed, 342 insertions(+), 2 deletions(-)
> >>   create mode 100644 include/u-boot/ecdsa.h
> >>   create mode 100644 lib/ecdsa/ecdsa-libcrypto.c
> >>
> >> diff --git a/common/image-sig.c b/common/image-sig.c
> >> index 21dafe6b91..2548b3eb0f 100644
> >> --- a/common/image-sig.c
> >> +++ b/common/image-sig.c
> >> @@ -15,6 +15,7 @@
> >>   DECLARE_GLOBAL_DATA_PTR;
> >>   #endif /* !USE_HOSTCC*/
> >>   #include <image.h>
> >> +#include <u-boot/ecdsa.h>
> >>   #include <u-boot/rsa.h>
> >>   #include <u-boot/hash-checksum.h>
> >>
> >> @@ -82,8 +83,17 @@ struct crypto_algo crypto_algos[] = {
> >>                  .sign = rsa_sign,
> >>                  .add_verify_data = rsa_add_verify_data,
> >>                  .verify = rsa_verify,
> >> -       }
> >> -
> >> +       },
> >> +#if defined(USE_HOSTCC)
> >> +       /* Currently, only host support exists for ECDSA */
> >> +       {
> >> +               .name = "ecdsa256",
> >> +               .key_len = ECDSA256_BYTES,
> >> +               .sign = ecdsa_sign,
> >> +               .add_verify_data = ecdsa_add_verify_data,
> >> +               .verify = ecdsa_verify,
> >> +       },
> >> +#endif
> >>   };
> >>
> >>   struct padding_algo padding_algos[] = {
> >> diff --git a/include/u-boot/ecdsa.h b/include/u-boot/ecdsa.h
> >> new file mode 100644
> >> index 0000000000..a13a7267e1
> >> --- /dev/null
> >> +++ b/include/u-boot/ecdsa.h
> >> @@ -0,0 +1,27 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ */
> >> +/*
> >> + * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me at gmail.com>.
> >> + */
> >> +
> >> +#ifndef _ECDSA_H
> >> +#define _ECDSA_H
> >> +
> >> +#include <errno.h>
> >> +#include <image.h>
> >> +
> >> +/**
> >> + * crypto_algo API impementation for ECDSA;
> >> + * @see "struct crypt_algo"
> >> + * @{
> >> + */
> >> +int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
> >> +              int region_count, uint8_t **sigp, uint *sig_len);
> >> +int ecdsa_verify(struct image_sign_info *info,
> >> +                const struct image_region region[], int region_count,
> >> +                uint8_t *sig, uint sig_len);
> >> +int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest);
> >
> > Please always add full comments when you export functions, or have a
> > non-trivial static function.
>
> I disagree. These functions implement and are designed to be used via
> the crypt_algo API. One should understand the crypt_algo API, and any
> deviations in behavior would be a bug. So there is no scenario in which
> comments here would be useful.

Please add full comments on exported function, no exceptions. Again,
this is not Linux and people don't have as much time to cogitate on
code. They are just trying to get their device working.

>
> >> +/** @} */
> >> +
> >> +#define ECDSA256_BYTES (256 / 8)
> >> +
> >> +#endif
> >> diff --git a/lib/ecdsa/ecdsa-libcrypto.c b/lib/ecdsa/ecdsa-libcrypto.c
> >> new file mode 100644
> >> index 0000000000..ff491411d0
> >> --- /dev/null
> >> +++ b/lib/ecdsa/ecdsa-libcrypto.c
> >> @@ -0,0 +1,300 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/*
> >> + * ECDSA image signing implementation using libcrypto backend
> >> + *
> >> + * The signature is a binary representation of the (R, S) points, padded to the
> >> + * key size. The signature will be (2 * key_size_bits) / 8 bytes.
> >> + *
> >> + * Deviations from behavior of RSA equivalent:
> >> + *  - Verification uses private key. This is not technically required, but a
> >> + *    limitation on how clumsy the openssl API is to use.
> >
> > I'm not quite sure what the implications are on this. If this is
> > public key crypto, the private key is not available, so how can you
> > verify with it?
>
> I presume this is fixable, but only as an academic exercise. This file
> is for mkimage, which doesn't do standalone verification. The way you
> verify is in u-boot. That is the subject of another series.

OK I'm just a bit confused as to how this tests anything. But sure it
can come later.

>
> >> + *  - Handling of keys and key paths:
> >> + *    - The '-K' key directory option must contain path to the key file,
> >> + *      instead of the key directory.
> >
> > If we make this change we should update RSA to do the same.
>
> Of course, but is there agreement as to this direction? There seem to be
> some hidden subtleties to key-name-hint that I don't fully understand yet.

It's just that the filename is defined by the code not the parameter.
There is a public key file and a certificate file in the same dir, so
it is convenient to specify the dir rather than the filename. But I
don't see a big problem with changing it.  Everyone will have to
update their scripts I suppose, so there is that...

Another option would be to add a new arg which specifies the exact file.

>
> >> + *    - No assumptions are made about the file extension of the key
> >> + *    - The 'key-name-hint' property is only used for naming devicetree nodes,
> >> + *      but is not used for looking up keys on the filesystem.
> >> + *
> >> + * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me at gmail.com>
> >> + */
> >> +
> >> +#include <u-boot/ecdsa.h>
> >> +#include <u-boot/fdt-libcrypto.h>
> >> +#include <openssl/ssl.h>
> >> +#include <openssl/ec.h>
> >> +#include <openssl/bn.h>
> >> +
> >> +struct signer {
> >> +       EVP_PKEY *evp_key;
> >> +       EC_KEY *ecdsa_key;
> >> +       void *hash;
> >> +       void *signature;        /* Do not free() !*/
> >
> > need comments
>
> Is this for the sake of having comments, or is there something of value
> that I've missed? The only member that could be confusing is evp_key,
> but that becomes obvious in the context of libcrypto.

What does hash point to and what is it for? Same with signature. Why
are there two keys and what is the difference? Please, if you are
contributing to U-Boot, add comments.

>[..]

> >> +static size_t ecdsa_key_size_bytes(const EC_KEY *key)
> >
> > I think all of these functions need a comment.
>
> The function names are self-explanatory. One point of confusion would be
> the meaning of raw signature -- this is already explained at the top.

I would still like the comments please. If it's really trivial that's
fine, but this code is not obvious to me. We need to make it easy to
contribute to U-Boot.

>
> >
> >> +{
> >> +       const EC_GROUP *group;
> >> +
> >> +       group = EC_KEY_get0_group(key);
> >> +       return EC_GROUP_order_bits(group) / 8;
> >> +}
> >> +
> >> +static int read_key(struct signer *ctx, const char *key_name)
> >> +{
> >> +       FILE *f = fopen(key_name, "r");
> >> +
> >> +       if (!f) {
> >> +               fprintf(stderr, "Can not get key file '%s'\n", key_name);
> >> +               return -1;
> >
> > return -EIO perhaps?
>
> Good idea!
>
> >> +       }
> >> +
> >> +       ctx->evp_key = PEM_read_PrivateKey(f, NULL, NULL, NULL);
> >> +       fclose(f);
> >> +       if (!ctx->evp_key) {
> >> +               fprintf(stderr, "Can not read key from '%s'\n", key_name);
> >> +               return -1;
> >
> > These should return useful error number: -1 is -EPERM which doesn't
> > seem right here.
>
> -EIO again?

Sure, or perhaps a different error if this could mean that the file is corrupt.

[..]

> >> +static int prepare_ctx(struct signer *ctx, const struct image_sign_info *info)
> >> +{
> >> +       const char *kname = info->keydir;
> >> +       int key_len_bytes;
> >> +
> >> +       if (alloc_ctx(ctx, info) < 0)
> >
> > That function returns 0 on success, so you don't need the < 0. A
> > comment on the above function would make that clear.
>
> I think the following would be less clear:
>
>         if (alloc_ctx())
>                  error();
>
> Does alloc_ctx() return an int? Does it return memory? It has (m)alloc
> in the name. By having a '< 0' in the predicate, it's now clear that
> this can't return a pointer. So yes, you might scratch your head as to
> why there's a '< 0' that's not needed. Also, you know that this function
> returns an error code without needing to scroll up to its definition. A
> comment above the function wouldn't eliminate the need to scroll up.

U-Boot generally returns 0 for success and -ve for error. The +ve
numbers are used when the function needs to return a value, if valid.

>
> >> +               return -1;

-ENOMEM

> >> +
> >> +       if (read_key(ctx, kname) < 0)
> >> +               return -1;

I recommend handling errors like this:

int ret;

ret = read_key(ctx, kname);
if (ret)
   return log_msg_ret("read", ret);

so that people can quickly figure out exactly what is failing either
by looking at the error number (propagated to the top) or turning on
CONFIG_LOG_ERROR_RETURN

> >> +
> >> +       key_len_bytes = ecdsa_key_size_bytes(ctx->ecdsa_key);
> >> +       if (key_len_bytes != info->crypto->key_len) {
> >> +               fprintf(stderr, "Expected a %u-bit key, got %u-bit key\n",
> >> +                       info->crypto->key_len * 8, key_len_bytes * 8);
> >> +               return -1;
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >
> > [..]
> >
> > Regards,
> > Simon
> >

Regards,
Simon