mbox series

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

Message ID 20210108191737.615022-1-mr.nuke.me@gmail.com
Headers show
Series Add support for ECDSA image signing (with test) | expand

Message

Alex G. Jan. 8, 2021, 7:17 p.m. UTC
## 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

Comments

Patrick DELAUNAY Jan. 28, 2021, 4:40 p.m. UTC | #1
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@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@1 {
....
         signature@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
Alex G. Jan. 28, 2021, 5:54 p.m. UTC | #2
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@1 {
> ....
>          signature@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
Simon Glass Feb. 1, 2021, 8:44 p.m. UTC | #3
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@1 {
> > ....
> >          signature@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