[PATCH v4 0/6] Add support for ECDSA image signing (with test)

[PATCH v4 0/6] Add support for ECDSA image signing (with test)

[Alexandru Gagniuc]

## Purpose and intent

The purpose of this series is to enable ECDSA as an alternative to RSA
for FIT signing. As new chips have built-in support for ECDSA verified
boot, it makes sense to stick to one signing algorithm, instead of
resorting to RSA for u-boot images.

The focus of this series is signing an existing FIT image:

	mkimage -F some-existing.fit --signing-key some/key.pem

Signing while assembling a FIT is not a tested use case.
	
# Implementation

## Code organization

Unlike the RSA path, which mixes host and firmware code in the same,
source files, this series keeps a very clear distinction. 
ecdsa-libcrypto.c is intended to be used for host code and only for
host code. There is more opportunity for code reuse this way.

## Signing

There is one major difference from the RSA path. The 'key-name-hint'
property is ignored in the ECDSA path. There are two reasons:
  (1) The intent of 'key-name-hint' is not clear
  (2) Initial implementation is much easier to review
  
There is an intentional side-effect. The RSA path takes 'key-name-hint'
to decide which key file to read from disk. In the context of "which
fdt node describes my signing key", this makes sense. On the other
hand, 'key-name-hint' is also used as the basename of where the key is
on the filesystem. This leads to some funny search paths, such as

	"some/dir/(null).key"
	
So I am using the -K option to mkimage as the _full_ path to the key
file. It doesn't have to be named .key, it doesn't have to be named
.crt, and it doesn't have to exist in a particular directory (as is
the case for the RSA path). I understand and recognize that this
discrepancy must be resolved, but resolving it right now would make
the initial implementation much harder and longer.


# Testing

test/py/tests/test_fit_ecdsa.py is implemented withe the goal to check
that the signing is done correctly, and that the signature is encoded
in the proper raw format. Verification is done with pyCryptodomex, so
this test will catch both coding errors and openssl bugs. This is the
only scope of testing proposed here.


# Things not yet resolved:
 - is mkimage '-k' supposed to be a directory or file path
I'm hoping I can postpone answering this question pending further discussion.

Changes since v3:
 - Don't use 'log_msg_ret()', as it's not available host-side

Changes since v1 and v2:
 - Added lots of function comments
 - Replaced hardcoded error numbers with more meaningful errno numbers
 - Changed some error paths to use 'return log_msg_ret'

Alexandru Gagniuc (6):
  lib: Rename rsa-checksum.c to hash-checksum.c
  lib/rsa: Make fdt_add_bignum() available outside of RSA code
  lib: Add support for ECDSA image signing
  doc: signature.txt: Document devicetree format for ECDSA keys
  test/py: Add pycryptodomex to list of required pakages
  test/py: ecdsa: Add test for mkimage ECDSA signing

 common/image-fit-sig.c                        |   2 +-
 common/image-sig.c                            |  13 +-
 doc/uImage.FIT/signature.txt                  |   7 +-
 include/image.h                               |   5 +-
 include/u-boot/ecdsa.h                        |  94 ++++++
 include/u-boot/fdt-libcrypto.h                |  27 ++
 .../{rsa-checksum.h => hash-checksum.h}       |   0
 lib/Makefile                                  |   1 +
 lib/crypto/pkcs7_verify.c                     |   2 +-
 lib/crypto/x509_public_key.c                  |   2 +-
 lib/ecdsa/ecdsa-libcrypto.c                   | 306 ++++++++++++++++++
 lib/fdt-libcrypto.c                           |  72 +++++
 lib/{rsa/rsa-checksum.c => hash-checksum.c}   |   3 +-
 lib/rsa/Makefile                              |   2 +-
 lib/rsa/rsa-sign.c                            |  65 +---
 test/py/requirements.txt                      |   1 +
 test/py/tests/test_fit_ecdsa.py               | 111 +++++++
 tools/Makefile                                |   7 +-
 18 files changed, 645 insertions(+), 75 deletions(-)
 create mode 100644 include/u-boot/ecdsa.h
 create mode 100644 include/u-boot/fdt-libcrypto.h
 rename include/u-boot/{rsa-checksum.h => hash-checksum.h} (100%)
 create mode 100644 lib/ecdsa/ecdsa-libcrypto.c
 create mode 100644 lib/fdt-libcrypto.c
 rename lib/{rsa/rsa-checksum.c => hash-checksum.c} (96%)
 create mode 100644 test/py/tests/test_fit_ecdsa.py

-- 
2.26.2

[PATCH v4 1/6] lib: Rename rsa-checksum.c to hash-checksum.c

[Alexandru Gagniuc]

rsa-checksum.c sontains the hash_calculate() implementations. Despite
the "rsa-" file prefix, this function is useful for other algorithms.

To prevent confusion, move this file to lib/crypto, and rename it to
hash-checksum.c, to give it a more "generic" feel.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/image-fit-sig.c                             | 2 +-
 common/image-sig.c                                 | 2 +-
 include/image.h                                    | 2 +-
 include/u-boot/{rsa-checksum.h => hash-checksum.h} | 0
 lib/Makefile                                       | 1 +
 lib/crypto/pkcs7_verify.c                          | 2 +-
 lib/crypto/x509_public_key.c                       | 2 +-
 lib/{rsa/rsa-checksum.c => hash-checksum.c}        | 3 ++-
 lib/rsa/Makefile                                   | 2 +-
 tools/Makefile                                     | 3 ++-
 10 files changed, 11 insertions(+), 8 deletions(-)
 rename include/u-boot/{rsa-checksum.h => hash-checksum.h} (100%)
 rename lib/{rsa/rsa-checksum.c => hash-checksum.c} (96%)

diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index 5401d9411b..7fcbb47235 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -15,7 +15,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #include <fdt_region.h>
 #include <image.h>
 #include <u-boot/rsa.h>
-#include <u-boot/rsa-checksum.h>
+#include <u-boot/hash-checksum.h>
 
 #define IMAGE_MAX_HASHED_NODES		100
 
diff --git a/common/image-sig.c b/common/image-sig.c
index f3c209ae8b..21dafe6b91 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -16,7 +16,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #endif /* !USE_HOSTCC*/
 #include <image.h>
 #include <u-boot/rsa.h>
-#include <u-boot/rsa-checksum.h>
+#include <u-boot/hash-checksum.h>
 
 #define IMAGE_MAX_HASHED_NODES		100
 
diff --git a/include/image.h b/include/image.h
index 41473dbb9c..a55b11b3ae 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1258,7 +1258,7 @@ struct image_region {
 };
 
 #if IMAGE_ENABLE_VERIFY
-# include <u-boot/rsa-checksum.h>
+# include <u-boot/hash-checksum.h>
 #endif
 struct checksum_algo {
 	const char *name;
diff --git a/include/u-boot/rsa-checksum.h b/include/u-boot/hash-checksum.h
similarity index 100%
rename from include/u-boot/rsa-checksum.h
rename to include/u-boot/hash-checksum.h
diff --git a/lib/Makefile b/lib/Makefile
index 851a80ef3b..cf64188ba5 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -60,6 +60,7 @@ endif
 obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
 obj-$(CONFIG_$(SPL_)MD5) += md5.o
 obj-$(CONFIG_$(SPL_)RSA) += rsa/
+obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o
 obj-$(CONFIG_SHA1) += sha1.o
 obj-$(CONFIG_SHA256) += sha256.o
 obj-$(CONFIG_SHA512_ALGO) += sha512.o
diff --git a/lib/crypto/pkcs7_verify.c b/lib/crypto/pkcs7_verify.c
index 320ba49f79..3c411f651f 100644
--- a/lib/crypto/pkcs7_verify.c
+++ b/lib/crypto/pkcs7_verify.c
@@ -15,7 +15,7 @@
 #include <linux/bitops.h>
 #include <linux/compat.h>
 #include <linux/asn1.h>
-#include <u-boot/rsa-checksum.h>
+#include <u-boot/hash-checksum.h>
 #include <crypto/public_key.h>
 #include <crypto/pkcs7_parser.h>
 #else
diff --git a/lib/crypto/x509_public_key.c b/lib/crypto/x509_public_key.c
index 91810a8640..d557ab27ae 100644
--- a/lib/crypto/x509_public_key.c
+++ b/lib/crypto/x509_public_key.c
@@ -19,7 +19,7 @@
 #include <linux/kernel.h>
 #ifdef __UBOOT__
 #include <crypto/x509_parser.h>
-#include <u-boot/rsa-checksum.h>
+#include <u-boot/hash-checksum.h>
 #else
 #include <linux/slab.h>
 #include <keys/asymmetric-subtype.h>
diff --git a/lib/rsa/rsa-checksum.c b/lib/hash-checksum.c
similarity index 96%
rename from lib/rsa/rsa-checksum.c
rename to lib/hash-checksum.c
index e60debb7df..d732ecc38f 100644
--- a/lib/rsa/rsa-checksum.c
+++ b/lib/hash-checksum.c
@@ -13,7 +13,8 @@
 #else
 #include "fdt_host.h"
 #endif
-#include <u-boot/rsa.h>
+#include <hash.h>
+#include <image.h>
 
 int hash_calculate(const char *name,
 		    const struct image_region region[],
diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
index 8b75d41f04..c9ac72c1e2 100644
--- a/lib/rsa/Makefile
+++ b/lib/rsa/Makefile
@@ -5,6 +5,6 @@
 # (C) Copyright 2000-2007
 # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
 
-obj-$(CONFIG_$(SPL_TPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
+obj-$(CONFIG_$(SPL_TPL_)RSA_VERIFY) += rsa-verify.o
 obj-$(CONFIG_$(SPL_TPL_)RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
 obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
diff --git a/tools/Makefile b/tools/Makefile
index 253a6b9706..b1595ad814 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -67,7 +67,7 @@ LIBFDT_OBJS := $(addprefix libfdt/, fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o \
 		fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o)
 
 RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/rsa/, \
-					rsa-sign.o rsa-verify.o rsa-checksum.o \
+					rsa-sign.o rsa-verify.o \
 					rsa-mod-exp.o)
 
 AES_OBJS-$(CONFIG_FIT_CIPHER) := $(addprefix lib/aes/, \
@@ -105,6 +105,7 @@ dumpimage-mkimage-objs := aisimage.o \
 			$(ROCKCHIP_OBS) \
 			socfpgaimage.o \
 			lib/crc16.o \
+			lib/hash-checksum.o \
 			lib/sha1.o \
 			lib/sha256.o \
 			lib/sha512.o \
-- 
2.26.2

[PATCH v4 2/6] lib/rsa: Make fdt_add_bignum() available outside of RSA code

[Alexandru Gagniuc]

fdt_add_bignum() is useful for algorithms other than just RSA. To
allow its use for ECDSA, move it to a common file under lib/.

The new file is suffixed with '-libcrypto' because it has a direct
dependency on openssl. This is due to the use of the "BIGNUM *" type.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 include/u-boot/fdt-libcrypto.h | 27 +++++++++++++
 lib/fdt-libcrypto.c            | 72 ++++++++++++++++++++++++++++++++++
 lib/rsa/rsa-sign.c             | 65 +-----------------------------
 tools/Makefile                 |  1 +
 4 files changed, 101 insertions(+), 64 deletions(-)
 create mode 100644 include/u-boot/fdt-libcrypto.h
 create mode 100644 lib/fdt-libcrypto.c

diff --git a/include/u-boot/fdt-libcrypto.h b/include/u-boot/fdt-libcrypto.h
new file mode 100644
index 0000000000..5142f37039
--- /dev/null
+++ b/include/u-boot/fdt-libcrypto.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me@gmail.com>
+ * Copyright (c) 2013, Google Inc.
+ */
+
+#ifndef _FDT_LIBCRYPTO_H
+#define _FDT_LIBCRYPTO_H
+
+#include <openssl/bn.h>
+
+/**
+ * fdt_add_bignum() - Write a libcrypto BIGNUM as an FDT property
+ *
+ * Convert a libcrypto BIGNUM * into a big endian array of integers.
+ *
+ * @blob:	FDT blob to modify
+ * @noffset:	Offset of the FDT node
+ * @prop_name:	What to call the property in the FDT
+ * @num:	pointer to a libcrypto big number
+ * @num_bits:	How big is 'num' in bits?
+ * @return 0 if all good all working, -ve on horror
+ */
+int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
+		   BIGNUM *num, int num_bits);
+
+#endif /* _FDT_LIBCRYPTO_H */
diff --git a/lib/fdt-libcrypto.c b/lib/fdt-libcrypto.c
new file mode 100644
index 0000000000..ecb0344c8f
--- /dev/null
+++ b/lib/fdt-libcrypto.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me@gmail.com>
+ * Copyright (c) 2013, Google Inc.
+ */
+
+#include <libfdt.h>
+#include <u-boot/fdt-libcrypto.h>
+
+int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
+		   BIGNUM *num, int num_bits)
+{
+	int nwords = num_bits / 32;
+	int size;
+	uint32_t *buf, *ptr;
+	BIGNUM *tmp, *big2, *big32, *big2_32;
+	BN_CTX *ctx;
+	int ret;
+
+	tmp = BN_new();
+	big2 = BN_new();
+	big32 = BN_new();
+	big2_32 = BN_new();
+
+	/*
+	 * Note: This code assumes that all of the above succeed, or all fail.
+	 * In practice memory allocations generally do not fail (unless the
+	 * process is killed), so it does not seem worth handling each of these
+	 * as a separate case. Technicaly this could leak memory on failure,
+	 * but a) it won't happen in practice, and b) it doesn't matter as we
+	 * will immediately exit with a failure code.
+	 */
+	if (!tmp || !big2 || !big32 || !big2_32) {
+		fprintf(stderr, "Out of memory (bignum)\n");
+		return -ENOMEM;
+	}
+	ctx = BN_CTX_new();
+	if (!ctx) {
+		fprintf(stderr, "Out of memory (bignum context)\n");
+		return -ENOMEM;
+	}
+	BN_set_word(big2, 2L);
+	BN_set_word(big32, 32L);
+	BN_exp(big2_32, big2, big32, ctx); /* B = 2^32 */
+
+	size = nwords * sizeof(uint32_t);
+	buf = malloc(size);
+	if (!buf) {
+		fprintf(stderr, "Out of memory (%d bytes)\n", size);
+		return -ENOMEM;
+	}
+
+	/* Write out modulus as big endian array of integers */
+	for (ptr = buf + nwords - 1; ptr >= buf; ptr--) {
+		BN_mod(tmp, num, big2_32, ctx); /* n = N mod B */
+		*ptr = cpu_to_fdt32(BN_get_word(tmp));
+		BN_rshift(num, num, 32); /*  N = N/B */
+	}
+
+	/*
+	 * We try signing with successively increasing size values, so this
+	 * might fail several times
+	 */
+	ret = fdt_setprop(blob, noffset, prop_name, buf, size);
+	free(buf);
+	BN_free(tmp);
+	BN_free(big2);
+	BN_free(big32);
+	BN_free(big2_32);
+
+	return ret ? -FDT_ERR_NOSPACE : 0;
+}
diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
index 1f0d81bd7a..557c690a6d 100644
--- a/lib/rsa/rsa-sign.c
+++ b/lib/rsa/rsa-sign.c
@@ -9,6 +9,7 @@
 #include <string.h>
 #include <image.h>
 #include <time.h>
+#include <u-boot/fdt-libcrypto.h>
 #include <openssl/bn.h>
 #include <openssl/rsa.h>
 #include <openssl/pem.h>
@@ -680,70 +681,6 @@ int rsa_get_params(RSA *key, uint64_t *exponent, uint32_t *n0_invp,
 	return ret;
 }
 
-static int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
-			  BIGNUM *num, int num_bits)
-{
-	int nwords = num_bits / 32;
-	int size;
-	uint32_t *buf, *ptr;
-	BIGNUM *tmp, *big2, *big32, *big2_32;
-	BN_CTX *ctx;
-	int ret;
-
-	tmp = BN_new();
-	big2 = BN_new();
-	big32 = BN_new();
-	big2_32 = BN_new();
-
-	/*
-	 * Note: This code assumes that all of the above succeed, or all fail.
-	 * In practice memory allocations generally do not fail (unless the
-	 * process is killed), so it does not seem worth handling each of these
-	 * as a separate case. Technicaly this could leak memory on failure,
-	 * but a) it won't happen in practice, and b) it doesn't matter as we
-	 * will immediately exit with a failure code.
-	 */
-	if (!tmp || !big2 || !big32 || !big2_32) {
-		fprintf(stderr, "Out of memory (bignum)\n");
-		return -ENOMEM;
-	}
-	ctx = BN_CTX_new();
-	if (!ctx) {
-		fprintf(stderr, "Out of memory (bignum context)\n");
-		return -ENOMEM;
-	}
-	BN_set_word(big2, 2L);
-	BN_set_word(big32, 32L);
-	BN_exp(big2_32, big2, big32, ctx); /* B = 2^32 */
-
-	size = nwords * sizeof(uint32_t);
-	buf = malloc(size);
-	if (!buf) {
-		fprintf(stderr, "Out of memory (%d bytes)\n", size);
-		return -ENOMEM;
-	}
-
-	/* Write out modulus as big endian array of integers */
-	for (ptr = buf + nwords - 1; ptr >= buf; ptr--) {
-		BN_mod(tmp, num, big2_32, ctx); /* n = N mod B */
-		*ptr = cpu_to_fdt32(BN_get_word(tmp));
-		BN_rshift(num, num, 32); /*  N = N/B */
-	}
-
-	/*
-	 * We try signing with successively increasing size values, so this
-	 * might fail several times
-	 */
-	ret = fdt_setprop(blob, noffset, prop_name, buf, size);
-	free(buf);
-	BN_free(tmp);
-	BN_free(big2);
-	BN_free(big32);
-	BN_free(big2_32);
-
-	return ret ? -FDT_ERR_NOSPACE : 0;
-}
-
 int rsa_add_verify_data(struct image_sign_info *info, void *keydest)
 {
 	BIGNUM *modulus, *r_squared;
diff --git a/tools/Makefile b/tools/Makefile
index b1595ad814..af7698fd01 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -106,6 +106,7 @@ dumpimage-mkimage-objs := aisimage.o \
 			socfpgaimage.o \
 			lib/crc16.o \
 			lib/hash-checksum.o \
+			lib/fdt-libcrypto.o \
 			lib/sha1.o \
 			lib/sha256.o \
 			lib/sha512.o \
-- 
2.26.2

[PATCH v4 3/6] lib: Add support for ECDSA image signing

[Alexandru Gagniuc]

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          |  11 +-
 include/image.h             |   3 +
 include/u-boot/ecdsa.h      |  94 +++++++++++
 lib/ecdsa/ecdsa-libcrypto.c | 306 ++++++++++++++++++++++++++++++++++++
 tools/Makefile              |   3 +
 5 files changed, 415 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..4b7d01f6c2 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,14 @@ struct crypto_algo crypto_algos[] = {
 		.sign = rsa_sign,
 		.add_verify_data = rsa_add_verify_data,
 		.verify = rsa_verify,
-	}
-
+	},
+	{
+		.name = "ecdsa256",
+		.key_len = ECDSA256_BYTES,
+		.sign = ecdsa_sign,
+		.add_verify_data = ecdsa_add_verify_data,
+		.verify = ecdsa_verify,
+	},
 };
 
 struct padding_algo padding_algos[] = {
diff --git a/include/image.h b/include/image.h
index a55b11b3ae..6628173dca 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1199,16 +1199,19 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 # if defined(CONFIG_FIT_SIGNATURE)
 #  define IMAGE_ENABLE_SIGN	1
 #  define IMAGE_ENABLE_VERIFY	1
+#  define IMAGE_ENABLE_VERIFY_ECDSA	1
 #  define FIT_IMAGE_ENABLE_VERIFY	1
 #  include <openssl/evp.h>
 # else
 #  define IMAGE_ENABLE_SIGN	0
 #  define IMAGE_ENABLE_VERIFY	0
+# define IMAGE_ENABLE_VERIFY_ECDSA	0
 #  define FIT_IMAGE_ENABLE_VERIFY	0
 # endif
 #else
 # define IMAGE_ENABLE_SIGN	0
 # define IMAGE_ENABLE_VERIFY		CONFIG_IS_ENABLED(RSA_VERIFY)
+# define IMAGE_ENABLE_VERIFY_ECDSA	0
 # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
 #endif
 
diff --git a/include/u-boot/ecdsa.h b/include/u-boot/ecdsa.h
new file mode 100644
index 0000000000..979690d966
--- /dev/null
+++ b/include/u-boot/ecdsa.h
@@ -0,0 +1,94 @@
+/* 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>
+#include <linux/kconfig.h>
+
+/**
+ * crypto_algo API impementation for ECDSA;
+ * @see "struct crypto_algo"
+ * @{
+ */
+#if IMAGE_ENABLE_SIGN
+/**
+ * sign() - calculate and return signature for given input data
+ *
+ * @info:	Specifies key and FIT information
+ * @data:	Pointer to the input data
+ * @data_len:	Data length
+ * @sigp:	Set to an allocated buffer holding the signature
+ * @sig_len:	Set to length of the calculated hash
+ *
+ * This computes input data signature according to selected algorithm.
+ * Resulting signature value is placed in an allocated buffer, the
+ * pointer is returned as *sigp. The length of the calculated
+ * signature is returned via the sig_len pointer argument. The caller
+ * should free *sigp.
+ *
+ * @return: 0, on success, -ve on error
+ */
+int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
+	       int region_count, uint8_t **sigp, uint *sig_len);
+
+/**
+ * add_verify_data() - Add verification information to FDT
+ *
+ * Add public key information to the FDT node, suitable for
+ * verification at run-time. The information added depends on the
+ * algorithm being used. I just copypasted this from rsa.h.
+ *
+ * @info:	Specifies key and FIT information
+ * @keydest:	Destination FDT blob for public key data
+ * @return: 0, on success, -ENOSPC if the keydest FDT blob ran out of space,
+ * other -ve value on error
+ */
+int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest);
+#else
+static inline
+int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
+	       int region_count, uint8_t **sigp, uint *sig_len)
+{
+	return -ENXIO;
+}
+
+static inline
+int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest)
+{
+	return -ENXIO;
+}
+#endif
+
+#if IMAGE_ENABLE_VERIFY_ECDSA
+/**
+ * verify() - Verify a signature against some data
+ *
+ * @info:	Specifies key and FIT information
+ * @data:	Pointer to the input data
+ * @data_len:	Data length
+ * @sig:	Signature
+ * @sig_len:	Number of bytes in signature
+ * @return 0 if verified, -ve on error
+ */
+int ecdsa_verify(struct image_sign_info *info,
+		 const struct image_region region[], int region_count,
+		 uint8_t *sig, uint sig_len);
+#else
+static inline
+int ecdsa_verify(struct image_sign_info *info,
+		 const struct image_region region[], int region_count,
+		 uint8_t *sig, uint sig_len)
+{
+	return -ENXIO;
+}
+#endif
+/** @} */
+
+#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..322880963f
--- /dev/null
+++ b/lib/ecdsa/ecdsa-libcrypto.c
@@ -0,0 +1,306 @@
+// 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.
+ *  - Handling of keys and key paths:
+ *    - The '-K' key directory option must contain path to the key file,
+ *      instead of the key directory.
+ *    - 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,2021, 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>
+
+/* Image signing context for openssl-libcrypto */
+struct signer {
+	EVP_PKEY *evp_key;	/* Pointer to EVP_PKEY object */
+	EC_KEY *ecdsa_key;	/* Pointer to EC_KEY object */
+	void *hash;		/* Pointer to hash used for verification */
+	void *signature;	/* Pointer to output signature. Do not free()!*/
+};
+
+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 -ENOMEM;
+
+	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);
+}
+
+/* Get a signature from a raw encoding */
+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;
+}
+
+/* ECDSA key size in bytes */
+static size_t ecdsa_key_size_bytes(const EC_KEY *key)
+{
+	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 -ENOENT;
+	}
+
+	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 -EIO;
+	}
+
+	if (EVP_PKEY_id(ctx->evp_key) != EVP_PKEY_EC) {
+		fprintf(stderr, "'%s' is not an ECDSA key\n", key_name);
+		return -EINVAL;
+	}
+
+	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 : -EINVAL;
+}
+
+/* Prepare a 'signer' context that's ready to sign and verify. */
+static int prepare_ctx(struct signer *ctx, const struct image_sign_info *info)
+{
+	const char *kname = info->keydir;
+	int key_len_bytes, ret;
+
+	ret = alloc_ctx(ctx, info);
+	if (ret)
+		return ret;
+
+	ret = read_key(ctx, kname);
+	if (ret)
+		return ret;
+
+	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 -EINVAL;
+	}
+
+	return 0;
+}
+
+static int do_sign(struct signer *ctx, struct image_sign_info *info,
+		   const struct image_region region[], int region_count)
+{
+	const struct checksum_algo *algo = info->checksum;
+	ECDSA_SIG *sig;
+
+	algo->calculate(algo->name, region, region_count, ctx->hash);
+	sig = ECDSA_do_sign(ctx->hash, algo->checksum_len, ctx->ecdsa_key);
+
+	ecdsa_sig_encode_raw(ctx->signature, sig, info->crypto->key_len);
+
+	return 0;
+}
+
+static int ecdsa_check_signature(struct signer *ctx, struct image_sign_info *info)
+{
+	ECDSA_SIG *sig;
+	int okay;
+
+	sig = ecdsa_sig_from_raw(ctx->signature, info->crypto->key_len);
+	if (!sig)
+		return -ENOMEM;
+
+	okay = ECDSA_do_verify(ctx->hash, info->checksum->checksum_len,
+			       sig, ctx->ecdsa_key);
+	if (!okay)
+		fprintf(stderr, "WARNING: Signature is fake news!\n");
+
+	ECDSA_SIG_free(sig);
+	return !okay;
+}
+
+static int do_verify(struct signer *ctx, struct image_sign_info *info,
+		     const struct image_region region[], int region_count,
+		     uint8_t *raw_sig, uint sig_len)
+{
+	const struct checksum_algo *algo = info->checksum;
+
+	if (sig_len != info->crypto->key_len * 2) {
+		fprintf(stderr, "Signature has wrong length\n");
+		return -EINVAL;
+	}
+
+	memcpy(ctx->signature, raw_sig, sig_len);
+	algo->calculate(algo->name, region, region_count, ctx->hash);
+
+	return ecdsa_check_signature(ctx, info);
+}
+
+int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
+	       int region_count, uint8_t **sigp, uint *sig_len)
+{
+	struct signer ctx;
+	int ret;
+
+	ret = prepare_ctx(&ctx, info);
+	if (ret >= 0) {
+		do_sign(&ctx, info, region, region_count);
+		*sigp = ctx.signature;
+		*sig_len = info->crypto->key_len * 2;
+
+		ret = ecdsa_check_signature(&ctx, info);
+	}
+
+	free_ctx(&ctx);
+	return ret;
+}
+
+int ecdsa_verify(struct image_sign_info *info,
+		 const struct image_region region[], int region_count,
+		 uint8_t *sig, uint sig_len)
+{
+	struct signer ctx;
+	int ret;
+
+	ret = prepare_ctx(&ctx, info);
+	if (ret >= 0)
+		ret = do_verify(&ctx, info, region, region_count, sig, sig_len);
+
+	free_ctx(&ctx);
+	return ret;
+}
+
+static int do_add(struct signer *ctx, void *fdt, const char *key_node_name)
+{
+	int signature_node, key_node, ret, key_bits;
+	const char *curve_name;
+	const EC_GROUP *group;
+	const EC_POINT *point;
+	BIGNUM *x, *y;
+
+	signature_node = fdt_subnode_offset(fdt, 0, FIT_SIG_NODENAME);
+	if (signature_node < 0) {
+		fprintf(stderr, "Could not find 'signature node: %s\n",
+			fdt_strerror(signature_node));
+		return signature_node;
+	}
+
+	key_node = fdt_add_subnode(fdt, signature_node, key_node_name);
+	if (key_node < 0) {
+		fprintf(stderr, "Could not create '%s' node: %s\n",
+			key_node_name, fdt_strerror(key_node));
+		return key_node;
+	}
+
+	group = EC_KEY_get0_group(ctx->ecdsa_key);
+	key_bits = EC_GROUP_order_bits(group);
+	curve_name = OBJ_nid2sn(EC_GROUP_get_curve_name(group));
+	/* Let 'x' and 'y' memory leak by not BN_free()'ing them. */
+	x = BN_new();
+	y = BN_new();
+	point = EC_KEY_get0_public_key(ctx->ecdsa_key);
+	EC_POINT_get_affine_coordinates(group, point, x, y, NULL);
+
+	ret = fdt_setprop_string(fdt, key_node, "ecdsa,curve", curve_name);
+	if (ret < 0)
+		return ret;
+
+	ret = fdt_add_bignum(fdt, key_node, "ecdsa,x-point", x, key_bits);
+	if (ret < 0)
+		return ret;
+
+	ret = fdt_add_bignum(fdt, key_node, "ecdsa,y-point", y, key_bits);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+int ecdsa_add_verify_data(struct image_sign_info *info, void *fdt)
+{
+	const char *fdt_key_name;
+	struct signer ctx;
+	int ret;
+
+	fdt_key_name = info->keyname ? info->keyname : "default-key";
+	ret = prepare_ctx(&ctx, info);
+	if (ret >= 0)
+		do_add(&ctx, fdt, fdt_key_name);
+
+	free_ctx(&ctx);
+	return ret;
+}
diff --git a/tools/Makefile b/tools/Makefile
index af7698fd01..a4f7b7e80c 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -70,6 +70,8 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/rsa/, \
 					rsa-sign.o rsa-verify.o \
 					rsa-mod-exp.o)
 
+ECDSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.o)
+
 AES_OBJS-$(CONFIG_FIT_CIPHER) := $(addprefix lib/aes/, \
 					aes-encrypt.o aes-decrypt.o)
 
@@ -119,6 +121,7 @@ dumpimage-mkimage-objs := aisimage.o \
 			gpimage.o \
 			gpimage-common.o \
 			mtk_image.o \
+			$(ECDSA_OBJS-y) \
 			$(RSA_OBJS-y) \
 			$(AES_OBJS-y)
 
-- 
2.26.2

[PATCH v4 4/6] doc: signature.txt: Document devicetree format for ECDSA keys

[Alexandru Gagniuc]

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 doc/uImage.FIT/signature.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/doc/uImage.FIT/signature.txt b/doc/uImage.FIT/signature.txt
index a3455889ed..0139295d33 100644
--- a/doc/uImage.FIT/signature.txt
+++ b/doc/uImage.FIT/signature.txt
@@ -142,7 +142,7 @@ public key in U-Boot's control FDT (using CONFIG_OF_CONTROL).
 Public keys should be stored as sub-nodes in a /signature node. Required
 properties are:
 
-- algo: Algorithm name (e.g. "sha1,rsa2048")
+- algo: Algorithm name (e.g. "sha1,rsa2048" or "sha256,ecdsa256")
 
 Optional properties are:
 
@@ -167,6 +167,11 @@ For RSA the following are mandatory:
 - rsa,r-squared: (2^num-bits)^2 as a big-endian multi-word integer
 - rsa,n0-inverse: -1 / modulus[0] mod 2^32
 
+For ECDSA the following are mandatory:
+- ecdsa,curve: Name of ECDSA curve (e.g. "prime256v1")
+- ecdsa,x-point: Public key X coordinate as a big-endian multi-word integer
+- ecdsa,y-point: Public key Y coordinate as a big-endian multi-word integer
+
 These parameters can be added to a binary device tree using parameter -K of the
 mkimage command::
 
-- 
2.26.2

[PATCH v4 5/6] test/py: Add pycryptodomex to list of required pakages

[Alexandru Gagniuc]

We wish to use pycryptodomex to verify code paths involving ECDSA
signatures. Add it to requirements.txt so that they get picked up
automatically .gitlab and .azure tasks

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 test/py/requirements.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/py/requirements.txt b/test/py/requirements.txt
index cf251186f4..5a25a9d5d5 100644
--- a/test/py/requirements.txt
+++ b/test/py/requirements.txt
@@ -10,6 +10,7 @@ packaging==19.2
 pbr==5.4.3
 pluggy==0.13.0
 py==1.8.0
+pycryptodomex==3.9.8
 pyparsing==2.4.2
 pytest==5.2.1
 python-mimeparse==1.6.0
-- 
2.26.2

[PATCH v4 6/6] test/py: ecdsa: Add test for mkimage ECDSA signing

[Alexandru Gagniuc]

Add a test to make sure that the ECDSA signatures generated by
mkimage can be verified successfully. pyCryptodomex was chosen as the
crypto library because it integrates much better with python code.
Using openssl would have been unnecessarily painful.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 test/py/tests/test_fit_ecdsa.py | 111 ++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100644 test/py/tests/test_fit_ecdsa.py

diff --git a/test/py/tests/test_fit_ecdsa.py b/test/py/tests/test_fit_ecdsa.py
new file mode 100644
index 0000000000..6cadb0cbb5
--- /dev/null
+++ b/test/py/tests/test_fit_ecdsa.py
@@ -0,0 +1,111 @@
+# SPDX-License-Identifier:	GPL-2.0+
+#
+# Copyright (c) 2020,2021 Alexandru Gagniuc <mr.nuke.me@gmail.com>
+
+"""
+Test ECDSA signing of FIT images
+
+This test uses mkimage to sign an existing FIT image with an ECDSA key. The
+signature is then extracted, and verified against pyCryptodome.
+This test doesn't run the sandbox. It only checks the host tool 'mkimage'
+"""
+
+import pytest
+import u_boot_utils as util
+from Cryptodome.Hash import SHA256
+from Cryptodome.PublicKey import ECC
+from Cryptodome.Signature import DSS
+
+class SignableFitImage(object):
+    """ Helper to manipulate a FIT image on disk """
+    def __init__(self, cons, file_name):
+        self.fit = file_name
+        self.cons = cons
+        self.signable_nodes = set()
+
+    def __fdt_list(self, path):
+        return util.run_and_log(self.cons, f'fdtget -l {self.fit} {path}')
+
+    def __fdt_set(self, node, **prop_value):
+        for prop, value in prop_value.items():
+            util.run_and_log(self.cons, f'fdtput -ts {self.fit} {node} {prop} {value}')
+
+    def __fdt_get_binary(self, node, prop):
+        numbers = util.run_and_log(self.cons, f'fdtget -tbi {self.fit} {node} {prop}')
+
+        bignum = bytearray()
+        for little_num in numbers.split():
+            bignum.append(int(little_num))
+
+        return bignum
+
+    def find_signable_image_nodes(self):
+        for node in self.__fdt_list('/images').split():
+            image = f'/images/{node}'
+            if 'signature' in self.__fdt_list(image):
+                self.signable_nodes.add(image)
+
+        return self.signable_nodes
+
+    def change_signature_algo_to_ecdsa(self):
+        for image in self.signable_nodes:
+            self.__fdt_set(f'{image}/signature', algo='sha256,ecdsa256')
+
+    def sign(self, mkimage, key_file):
+        util.run_and_log(self.cons, [mkimage, '-F', self.fit, f'-k{key_file}'])
+
+    def check_signatures(self, key):
+        for image in self.signable_nodes:
+            raw_sig = self.__fdt_get_binary(f'{image}/signature', 'value')
+            raw_bin = self.__fdt_get_binary(image, 'data')
+
+            sha = SHA256.new(raw_bin)
+            verifier = DSS.new(key, 'fips-186-3')
+            verifier.verify(sha, bytes(raw_sig))
+
+
+ at pytest.mark.buildconfigspec('fit_signature')
+ at pytest.mark.requiredtool('dtc')
+ at pytest.mark.requiredtool('fdtget')
+ at pytest.mark.requiredtool('fdtput')
+def test_fit_ecdsa(u_boot_console):
+    """ Test that signatures generated by mkimage are legible. """
+    def generate_ecdsa_key():
+        return ECC.generate(curve='prime256v1')
+
+    def assemble_fit_image(dest_fit, its, destdir):
+        dtc_args = f'-I dts -O dtb -i {destdir}'
+        util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f', its, dest_fit])
+
+    def dtc(dts):
+        dtb = dts.replace('.dts', '.dtb')
+        util.run_and_log(cons, f'dtc {datadir}/{dts} -O dtb -o {tempdir}/{dtb}')
+
+    cons = u_boot_console
+    mkimage = cons.config.build_dir + '/tools/mkimage'
+    datadir = cons.config.source_dir + '/test/py/tests/vboot/'
+    tempdir = cons.config.result_dir
+    key_file = f'{tempdir}/ecdsa-test-key.pem'
+    fit_file = f'{tempdir}/test.fit'
+    dtc('sandbox-kernel.dts')
+
+    key = generate_ecdsa_key()
+
+    # Create a fake kernel image -- zeroes will do just fine
+    with open(f'{tempdir}/test-kernel.bin', 'w') as fd:
+        fd.write(500 * chr(0))
+
+    # invokations of mkimage expect to read the key from disk
+    with open(key_file, 'w') as f:
+        f.write(key.export_key(format='PEM'))
+
+    assemble_fit_image(fit_file, f'{datadir}/sign-images-sha256.its', tempdir)
+
+    fit = SignableFitImage(cons, fit_file)
+    nodes = fit.find_signable_image_nodes()
+    if len(nodes) == 0:
+        raise ValueError('FIT image has no "/image" nodes with "signature"')
+
+    fit.change_signature_algo_to_ecdsa()
+    fit.sign(mkimage, key_file)
+    fit.check_signatures(key)
-- 
2.26.2

[PATCH v4 1/6] lib: Rename rsa-checksum.c to hash-checksum.c

[Patrick DELAUNAY]

Hi,

On 1/8/21 8:17 PM, Alexandru Gagniuc wrote:
> rsa-checksum.c sontains the hash_calculate() implementations. Despite
> the "rsa-" file prefix, this function is useful for other algorithms.
>
> To prevent confusion, move this file to lib/crypto, and rename it to

Today the file is moved in lib and not in lib/crypto...

change the comment or the file location.

> hash-checksum.c, to give it a more "generic" feel.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   common/image-fit-sig.c                             | 2 +-
>   common/image-sig.c                                 | 2 +-
>   include/image.h                                    | 2 +-
>   include/u-boot/{rsa-checksum.h => hash-checksum.h} | 0
>   lib/Makefile                                       | 1 +
>   lib/crypto/pkcs7_verify.c                          | 2 +-
>   lib/crypto/x509_public_key.c                       | 2 +-
>   lib/{rsa/rsa-checksum.c => hash-checksum.c}        | 3 ++-
>   lib/rsa/Makefile                                   | 2 +-
>   tools/Makefile                                     | 3 ++-
>   10 files changed, 11 insertions(+), 8 deletions(-)
>   rename include/u-boot/{rsa-checksum.h => hash-checksum.h} (100%)
>   rename lib/{rsa/rsa-checksum.c => hash-checksum.c} (96%)
>
> diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
> index 5401d9411b..7fcbb47235 100644
> --- a/common/image-fit-sig.c
> +++ b/common/image-fit-sig.c
> @@ -15,7 +15,7 @@ DECLARE_GLOBAL_DATA_PTR;
>   #include <fdt_region.h>
>   #include <image.h>
>   #include <u-boot/rsa.h>
> -#include <u-boot/rsa-checksum.h>
> +#include <u-boot/hash-checksum.h>
>   
>   #define IMAGE_MAX_HASHED_NODES		100
>   
> diff --git a/common/image-sig.c b/common/image-sig.c
> index f3c209ae8b..21dafe6b91 100644
> --- a/common/image-sig.c
> +++ b/common/image-sig.c
> @@ -16,7 +16,7 @@ DECLARE_GLOBAL_DATA_PTR;
>   #endif /* !USE_HOSTCC*/
>   #include <image.h>
>   #include <u-boot/rsa.h>
> -#include <u-boot/rsa-checksum.h>
> +#include <u-boot/hash-checksum.h>
>   
>   #define IMAGE_MAX_HASHED_NODES		100
>   
> diff --git a/include/image.h b/include/image.h
> index 41473dbb9c..a55b11b3ae 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1258,7 +1258,7 @@ struct image_region {
>   };
>   
>   #if IMAGE_ENABLE_VERIFY
> -# include <u-boot/rsa-checksum.h>
> +# include <u-boot/hash-checksum.h>
>   #endif
>   struct checksum_algo {
>   	const char *name;
> diff --git a/include/u-boot/rsa-checksum.h b/include/u-boot/hash-checksum.h
> similarity index 100%
> rename from include/u-boot/rsa-checksum.h
> rename to include/u-boot/hash-checksum.h
> diff --git a/lib/Makefile b/lib/Makefile
> index 851a80ef3b..cf64188ba5 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -60,6 +60,7 @@ endif
>   obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
>   obj-$(CONFIG_$(SPL_)MD5) += md5.o
>   obj-$(CONFIG_$(SPL_)RSA) += rsa/
> +obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o

compiled if CONFIG_FIT_SIGNATURE = y

but after used in mkimage even if CONFIG_FIT_SIGNATURE is not used...

(see after dumpimage-mkimage-objs)

it could be more simple to have :

obj-y += hash-checksum.o

or add a new config CONFIG_HASH (but we need to managed dependancy in Kconfig / Makefile)
  

(...)
> diff --git a/tools/Makefile b/tools/Makefile
> index 253a6b9706..b1595ad814 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -67,7 +67,7 @@ LIBFDT_OBJS := $(addprefix libfdt/, fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o \
>   		fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o)
>   
>   RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/rsa/, \
> -					rsa-sign.o rsa-verify.o rsa-checksum.o \
> +					rsa-sign.o rsa-verify.o \
>   					rsa-mod-exp.o)
>   
>   AES_OBJS-$(CONFIG_FIT_CIPHER) := $(addprefix lib/aes/, \
> @@ -105,6 +105,7 @@ dumpimage-mkimage-objs := aisimage.o \
>   			$(ROCKCHIP_OBS) \
>   			socfpgaimage.o \
>   			lib/crc16.o \
> +			lib/hash-checksum.o \
>   			lib/sha1.o \
>   			lib/sha256.o \
>   			lib/sha512.o \

lib/hash-checksum.o is required here...

Regards

Patrick

[PATCH v4 2/6] lib/rsa: Make fdt_add_bignum() available outside of RSA code

[Patrick DELAUNAY]

Hi,

On 1/8/21 8:17 PM, Alexandru Gagniuc wrote:
> fdt_add_bignum() is useful for algorithms other than just RSA. To
> allow its use for ECDSA, move it to a common file under lib/.
>
> The new file is suffixed with '-libcrypto' because it has a direct
> dependency on openssl. This is due to the use of the "BIGNUM *" type.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   include/u-boot/fdt-libcrypto.h | 27 +++++++++++++
>   lib/fdt-libcrypto.c            | 72 ++++++++++++++++++++++++++++++++++
>   lib/rsa/rsa-sign.c             | 65 +-----------------------------
>   tools/Makefile                 |  1 +
>   4 files changed, 101 insertions(+), 64 deletions(-)
>   create mode 100644 include/u-boot/fdt-libcrypto.h
>   create mode 100644 lib/fdt-libcrypto.c
(...)
> diff --git a/tools/Makefile b/tools/Makefile
> index b1595ad814..af7698fd01 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -106,6 +106,7 @@ dumpimage-mkimage-objs := aisimage.o \
>   			socfpgaimage.o \
>   			lib/crc16.o \
>   			lib/hash-checksum.o \
> +			lib/fdt-libcrypto.o \
>   			lib/sha1.o \
>   			lib/sha256.o \
>   			lib/sha512.o \


This part cause compilation issue when CONFIG_FIT_SIGNATURE is not 
defined / when openssl is not used

fdt-libcrypto.c:(.text+0x2f): undefined reference to `BN_new'
fdt-libcrypto.c:(.text+0x37): undefined reference to `BN_new'
fdt-libcrypto.c:(.text+0x44): undefined reference to `BN_new'
fdt-libcrypto.c:(.text+0x51): undefined reference to `BN_new'
fdt-libcrypto.c:(.text+0x7d): undefined reference to `BN_CTX_new'
fdt-libcrypto.c:(.text+0x9d): undefined reference to `BN_set_word'
fdt-libcrypto.c:(.text+0xaf): undefined reference to `BN_set_word'
fdt-libcrypto.c:(.text+0xc5): undefined reference to `BN_exp'
fdt-libcrypto.c:(.text+0x135): undefined reference to `BN_div'
fdt-libcrypto.c:(.text+0x13d): undefined reference to `BN_get_word'
fdt-libcrypto.c:(.text+0x172): undefined reference to `BN_rshift'
fdt-libcrypto.c:(.text+0x1a9): undefined reference to `BN_free'
fdt-libcrypto.c:(.text+0x1b3): undefined reference to `BN_free'
fdt-libcrypto.c:(.text+0x1bd): undefined reference to `BN_free'
fdt-libcrypto.c:(.text+0x1c5): undefined reference to `BN_free'

libssl not always include in mkimage, see the lines:

 ? # MXSImage needs LibSSL
 ? ifneq 
($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X)$(CONFIG_FIT_SIGNATURE)$(CONFIG_FIT_CIPHER),)
 ? HOSTCFLAGS_kwbimage.o += \
 ? ? ? $(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
 ? HOSTLDLIBS_mkimage += \
 ?? ?? $(shell pkg-config --libs libssl libcrypto 2> /dev/null || echo 
"-lssl -lcrypto")


=> I propose to add the added lib only when CONFIG_FIT_SIGNATURE is

 ????? activated by removing dumpimage-mkimage-objs udpate and with:


- FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o 
common/image-fit-sig.o

+ FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o 
common/image-fit-sig.o lib/fdt-libcrypto.o


Regards

Patrick

[PATCH v4 2/6] lib/rsa: Make fdt_add_bignum() available outside of RSA code

[Tom Rini]

On Fri, Jan 08, 2021 at 01:17:33PM -0600, Alexandru Gagniuc wrote:

> fdt_add_bignum() is useful for algorithms other than just RSA. To
> allow its use for ECDSA, move it to a common file under lib/.
> 
> The new file is suffixed with '-libcrypto' because it has a direct
> dependency on openssl. This is due to the use of the "BIGNUM *" type.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

This makes a large number of platform tools fail to build such as
j7200_evm_a72.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210127/8ea63b1f/attachment.sig>

[PATCH v4 0/6] Add support for ECDSA image signing (with test)

[Patrick DELAUNAY]

Hi Alexandru,

On 1/8/21 8:17 PM, Alexandru Gagniuc wrote:

> ## Purpose and intent  > > The purpose of this series is to enable ECDSA as an alternative to 
 > RSA for FIT signing. As new chips have built-in support for ECDSA > 
verified boot, it makes sense to stick to one signing algorithm, > 
instead of resorting to RSA for u-boot images. > > The focus of this 
series is signing an existing FIT image: > > mkimage -F 
some-existing.fit --signing-key some/key.pem > > Signing while 
assembling a FIT is not a tested use case. # > Implementation > > ## 
Code organization > > Unlike the RSA path, which mixes host and firmware 
code in the same, > source files, this series keeps a very clear 
distinction. > ecdsa-libcrypto.c is intended to be used for host code 
and only for > host code. There is more opportunity for code reuse this 
way. > > ## Signing > > There is one major difference from the RSA path. 
The 'key-name-hint' > property is ignored in the ECDSA path. There are 
two reasons: (1) The > intent of 'key-name-hint' is not clear (2) 
Initial implementation is > much easier to review >
I found in doc/uImage.FIT/signature.txt the description

 ??? - key-name-hint: Name of key to use for signing. The keys will 
normally be in
 ??? a single directory (parameter -k to mkimage). For a given key 
<name>, its
 ??? private key is stored in <name>.key and the certificate is stored in
 ??? <name>.crt.


And i check the code and the result is a little different:

include/image.h:1000:#define FIT_KEY_HINT??????? "key-name-hint"

common/image-fit-sig.c:89:??? info->keyname = fdt_getprop(fit, noffset, 
FIT_KEY_HINT, NULL);

lib/rsa/rsa-sign.c:742: ret = fdt_setprop_string(keydest, node, 
FIT_KEY_HINT, info->keyname);

tools/image-host.c: info->keyname = fdt_getprop(fit, noffset, 
FIT_KEY_HINT, NULL);


=> "keyname" provided the name oft he node in device tree for verify

lib/rsa/rsa-verify.c:518:

 ??????? snprintf(name, sizeof(name), "key-%s", info->keyname);
 ??????? node = fdt_subnode_offset(blob, sig_node, name);
 ??????? ret = rsa_verify_with_keynode(info, hash, sig, sig_len, node);


=> "keyname" provide the name of the key provider (pem file or key 
prefix for engine ) in keydir directory


lib/rsa/rsa-sign.c:537:??? ret = rsa_get_priv_key(info->keydir, 
info->keyname, e, &rsa);

lib/rsa/rsa-sign.c:702:??? ret = rsa_get_pub_key(info->keydir, 
info->keyname, e, &rsa);


dir and namle are used to get crt or key file.

=> rsa_pem_get_priv_key? = "%s/%s.key", keydir, name

=> rsa_pem_get_pub_key = "%s/%s.crt", keydir, name


so today the RSA features seens more compete based on openssl (with 
ENGINE and pkcs11 support for exmaple)

and keydir / keyname seens clear enought...


PS: I think the engine part could be shared between RSA and EDCSA part.


> There is an intentional side-effect. The RSA path takes  > 'key-name-hint' to decide which key file to read from disk. In the > 
context of "which fdt node describes my signing key", this makes > 
sense. On the other hand, 'key-name-hint' is also used as the > basename 
of where the key is on the filesystem. This leads to some > funny search 
paths, such as > > "some/dir/(null).key" So I am using the -K option to 
mkimage as the > _full_ path to the key file. It doesn't have to be 
named .key, it > doesn't have to be named .crt, and it doesn't have to 
exist in a > particular directory (as is the case for the RSA path). I 
understand > and recognize that this discrepancy must be resolved, but 
resolving > it right now would make the initial implementation much 
harder and > longer. marex at denx.de

This new option -K with full path will be permanent added to mkimage

or it is a temporarily solution (for initial ECDSA implementation).


If it is permanent it should be also supported in RSA mode ?

 ??? => for example: -K allow to override the "key-name-hint" value


Why you can't easily could use the existing -k option and 
"key-name-hint" to keep the current RSA behavior for EDCSA (even without 
ENGINE support)


For me, EDCSA part can rely on "key-name-hint" to chose the expected key 
file to read the pem from disk

 ??? info->keydir / params->keydir (from option -k)

 ??? info->keyname


perhaps : edcsa_get_priv_key()? => "%s/%s.pem", keydir, keyname


in prepare_ctx() function


and in the test:

 ??? keyname = "ecdsa-test-key"

 ??? keydir =? "${tempdir}"


or I miss something.....


> key_file  > > # Testing > > test/py/tests/test_fit_ecdsa.py is implemented withe 
the goal to > check that the signing is done correctly, and that the 
signature is > encoded in the proper raw format. Verification is done 
with > pyCryptodomex, so this test will catch both coding errors and 
openssl > bugs. This is the only scope of testing proposed here. > > > # 
Things not yet resolved: - is mkimage '-k' supposed to be a > directory 
or file path I'm hoping I can postpone answering this > question pending 
further discussion.

Sorry to open debate.


I propose to change the test with previous proposal.

mkimage -F /local/home/frq07632/views/u-boot/build-sandbox/test.fit -k 
/local/home/frq07632/views/u-boot/build-sandbox/ecdsa-test-key.pem

=> -k use directory as RSA

mkimage -F /local/home/frq07632/views/u-boot/build-sandbox/test.fit -k 
/local/home/frq07632/views/u-boot/build-sandbox/

with test/py/tests/vboot/sign-images-sha256.its
 ?? ?fdt at 1 {
....
 ?? ??? ?signature at 1 {
 ??????????????????????????????? algo = "sha1,rsa2048";
-??????????????????????????????? key-name-hint = "dev";
+?????????????????????????????? key-name-hint = "ecdsa-test-key.pem";


 ??????????????????????? };
 ??????????????? };

Or change? key_file = f'{tempdir}/ecdsa-test-key.pem'

to '{tempdir}/dev.pem' file (as:? key-name-hint = "dev";)


Regards.


Patrick

[PATCH v4 0/6] Add support for ECDSA image signing (with test)

[Alex G.]

On 1/28/21 10:40 AM, Patrick DELAUNAY wrote:
> Hi Alexandru,
Hi Patrick

> I found in doc/uImage.FIT/signature.txt the description
> 
>  ??? - key-name-hint: Name of key to use for signing. The keys will 
> normally be in a single directory (parameter -k to mkimage).
[snip]

You are correct that the ECDSA path does not use the "key-name-hint". 
And this deviates from what RSA does. This is intentional.


[snip]> so today the RSA features seens more compete based on openssl (with
> ENGINE and pkcs11 support for exmaple)
> 
> and keydir / keyname seens clear enought...

The the common case with image signing is that one wants to sign an 
image with a key. keyname comes from the console, and keydir comes from 
the FIT image, which contradicts this simplicity.

Another issue is incorporating this into a bigger build system like 
yocto. Now mkimage would impose a specific directory structure for 
signing keys. This would not be ideal.

> PS: I think the engine part could be shared between RSA and EDCSA part.

I don't see the benefit of using the engine, and it seems highly 
libcrypto specific. It would be a lot more code, with no useful 
functionality -- we can ecdsa sign with the simpler code.

[snip]
> This new option -K with full path will be permanent added to mkimage
> 
> or it is a temporarily solution (for initial ECDSA implementation).
> 
> 
> If it is permanent it should be also supported in RSA mode ?
> 
>  ??? => for example: -K allow to override the "key-name-hint" value

Yes and no. It is temporary in that we'd like to update the RSA path to 
be consistent with the ECDSA path. It's permanent in that we want to 
have the -'k' option to specify the key filename instead of the key dir. 
At least that's my understanding given the previous discussion with Simon.


[snip]
> Sorry to open debate.
> I propose to change the test with previous proposal.
> 
[snip]
> with test/py/tests/vboot/sign-images-sha256.its
>  ?? ?fdt at 1 {
> ....
>  ?? ??? ?signature at 1 {
>  ??????????????????????????????? algo = "sha1,rsa2048";
> -??????????????????????????????? key-name-hint = "dev";
> +?????????????????????????????? key-name-hint = "ecdsa-test-key.pem";

This would go against us wanting to decouple the key filename from the 
key name. Consider haing several keys:

    dev-key-frobnozzle.pem
    prov-key-frobnozzle-r1.pem
    prov-key-frobnozzle-r2.pem
    prov-key-frobnozzle-r3-after-hack-mitigation.pem

One might not want to expose those key names in the .its. The issue is, 
when the fit-image is created, the key filename must be known. But when 
the signing happens on a separate machine, the filename really isn't known.

So we should really use the "key-name-hint" as a hint rather than a 
filename or part of a filename.

Alex

[PATCH v4 0/6] Add support for ECDSA image signing (with test)

[Simon Glass]

Hi Alex,

On Thu, 28 Jan 2021 at 10:54, Alex G. <mr.nuke.me@gmail.com> wrote:
>
> On 1/28/21 10:40 AM, Patrick DELAUNAY wrote:
> > Hi Alexandru,
> Hi Patrick
>
> > I found in doc/uImage.FIT/signature.txt the description
> >
> >      - key-name-hint: Name of key to use for signing. The keys will
> > normally be in a single directory (parameter -k to mkimage).
> [snip]
>
> You are correct that the ECDSA path does not use the "key-name-hint".
> And this deviates from what RSA does. This is intentional.
>

We need to join these two up though. We can't add a new way of doing
things whenever we add a new algorithm.

>
> [snip]> so today the RSA features seens more compete based on openssl (with
> > ENGINE and pkcs11 support for exmaple)
> >
> > and keydir / keyname seens clear enought...
>
> The the common case with image signing is that one wants to sign an
> image with a key. keyname comes from the console, and keydir comes from
> the FIT image, which contradicts this simplicity.
>
> Another issue is incorporating this into a bigger build system like
> yocto. Now mkimage would impose a specific directory structure for
> signing keys. This would not be ideal.

It more likely requires a standard filename for keys...I wonder how
this works in yocto at present?

>
> > PS: I think the engine part could be shared between RSA and EDCSA part.
>
> I don't see the benefit of using the engine, and it seems highly
> libcrypto specific. It would be a lot more code, with no useful
> functionality -- we can ecdsa sign with the simpler code.

The hope was that the same API could be used...is that possible?

>
> [snip]
> > This new option -K with full path will be permanent added to mkimage
> >
> > or it is a temporarily solution (for initial ECDSA implementation).
> >
> >
> > If it is permanent it should be also supported in RSA mode ?
> >
> >      => for example: -K allow to override the "key-name-hint" value
>
> Yes and no. It is temporary in that we'd like to update the RSA path to
> be consistent with the ECDSA path. It's permanent in that we want to
> have the -'k' option to specify the key filename instead of the key dir.
> At least that's my understanding given the previous discussion with Simon.

But we do need to either move RSA over (in the same patch series ) or
use a different flag. I'm leaning towards a different flag, since if
we are not changing RSA, it is just going to get confusing.

>
>
> [snip]
> > Sorry to open debate.
> > I propose to change the test with previous proposal.
> >
> [snip]
> > with test/py/tests/vboot/sign-images-sha256.its
> >      fdt at 1 {
> > ....
> >          signature at 1 {
> >                                  algo = "sha1,rsa2048";
> > -                                key-name-hint = "dev";
> > +                               key-name-hint = "ecdsa-test-key.pem";
>
> This would go against us wanting to decouple the key filename from the
> key name. Consider haing several keys:
>
>     dev-key-frobnozzle.pem
>     prov-key-frobnozzle-r1.pem
>     prov-key-frobnozzle-r2.pem
>     prov-key-frobnozzle-r3-after-hack-mitigation.pem
>
> One might not want to expose those key names in the .its. The issue is,
> when the fit-image is created, the key filename must be known. But when
> the signing happens on a separate machine, the filename really isn't known.
>
> So we should really use the "key-name-hint" as a hint rather than a
> filename or part of a filename.

Yes it is just a hint. We must not rely on it. Its only purpose is to
hopefully speed up verification since fewer signatures might need to
be tried.

Regards,
Simon