| Message ID | 20210108191737.615022-1-mr.nuke.me@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for ECDSA image signing (with test) | expand |
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
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
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
## 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