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

[Alex G. <mr.nuke.me@gmail.com>]

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.

> 
> On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@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@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@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.

>> +/** @} */
>> +
>> +#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.

>> + *  - 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.

>> + *    - 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@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.

>> +};
>> +
>> +static int alloc_ctx(struct signer *ctx, const struct image_sign_info *info)
>> +{
>> +       memset(ctx, 0, sizeof(*ctx));
>> +
>> +       if (!OPENSSL_init_ssl(0, NULL)) {
>> +               fprintf(stderr, "Failure to init SSL library\n");
>> +               return -1;
>> +       }
>> +
>> +       ctx->hash = malloc(info->checksum->checksum_len);
>> +       ctx->signature = malloc(info->crypto->key_len * 2);
>> +
>> +       if (!ctx->hash || !ctx->signature)
>> +               return -1;
>> +
>> +       return 0;
>> +}
>> +
>> +static void free_ctx(struct signer *ctx)
>> +{
>> +       if (ctx->ecdsa_key)
>> +               EC_KEY_free(ctx->ecdsa_key);
>> +
>> +       if (ctx->evp_key)
>> +               EVP_PKEY_free(ctx->evp_key);
>> +
>> +       if (ctx->hash)
>> +               free(ctx->hash);
>> +}
>> +
>> +/*
>> + * Convert an ECDSA signature to raw format
>> + *
>> + * openssl DER-encodes 'binary' signatures. We want the signature in a raw
>> + * (R, S) point pair. So we have to dance a bit.
>> + */
>> +static void ecdsa_sig_encode_raw(void *buf, const ECDSA_SIG *sig, size_t order)
>> +{
>> +       int point_bytes = order;
>> +       const BIGNUM *r, *s;
>> +       uintptr_t s_buf;
>> +
>> +       ECDSA_SIG_get0(sig, &r, &s);
>> +       s_buf = (uintptr_t)buf + point_bytes;
>> +       BN_bn2binpad(r, buf, point_bytes);
>> +       BN_bn2binpad(s, (void *)s_buf, point_bytes);
>> +}
>> +
>> +static ECDSA_SIG *ecdsa_sig_from_raw(void *buf, size_t order)
>> +{
>> +       int point_bytes = order;
>> +       uintptr_t s_buf;
>> +       ECDSA_SIG *sig;
>> +       BIGNUM *r, *s;
>> +
>> +       sig = ECDSA_SIG_new();
>> +       if (!sig)
>> +               return NULL;
>> +
>> +       s_buf = (uintptr_t)buf + point_bytes;
>> +       r = BN_bin2bn(buf, point_bytes, NULL);
>> +       s = BN_bin2bn((void *)s_buf, point_bytes, NULL);
>> +       ECDSA_SIG_set0(sig, r, s);
>> +
>> +       return sig;
>> +}
>> +
>> +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.

> 
>> +{
>> +       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?

>> +       }
>> +
>> +       if (EVP_PKEY_id(ctx->evp_key) != EVP_PKEY_EC) {
>> +               fprintf(stderr, "'%s' is not an ECDSA key\n", key_name);
>> +               return -1;
>> +       }
>> +
>> +       ctx->ecdsa_key = EVP_PKEY_get1_EC_KEY(ctx->evp_key);
>> +       if (!ctx->ecdsa_key)
>> +               fprintf(stderr, "Can not extract ECDSA key\n");
>> +
>> +       return (ctx->ecdsa_key) ? 0 : -1;
>> +}
>> +
>> +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.

>> +               return -1;
>> +
>> +       if (read_key(ctx, kname) < 0)
>> +               return -1;
>> +
>> +       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
>