From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex G. Date: Thu, 7 Jan 2021 10:27:50 -0600 Subject: [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing In-Reply-To: References: <20201230210028.4065824-1-mr.nuke.me@gmail.com> <20201230210028.4065824-4-mr.nuke.me@gmail.com> Message-ID: <826b4d42-c9bb-70cf-9deb-c2b29145e10f@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 >> --- >> 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 >> +#include >> #include >> #include >> >> @@ -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 . >> + */ >> + >> +#ifndef _ECDSA_H >> +#define _ECDSA_H >> + >> +#include >> +#include >> + >> +/** >> + * 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 >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +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 >