[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