Add ECDSA signature support to PSA Crypto backend #1734

Roolli · 2023-06-29T12:21:34Z

Following up on the recent addition of the PSA Crypto API in MCUBoot, this PR adds the ability to verify ECDSA P256 and P384 signatures. If P384 is being used the hashing algorithm switches to SHA384.

boot/bootutil/include/bootutil/caps.h

sim/Cargo.toml

sim/mcuboot-sys/Cargo.toml

sim/mcuboot-sys/build.rs

sim/mcuboot-sys/csupport/config-ecdsa.h

boot/bootutil/include/bootutil/crypto/sha.h

boot/bootutil/src/image_validate.c

davidvincze · 2023-07-14T12:36:41Z

@nordicjm could someone from Nordic have a look at the SHA-related modifications? Thanks for the help!

de-nordic · 2023-07-14T15:10:55Z

@nordicjm could someone from Nordic have a look at the SHA-related modifications? Thanks for the help!

I will take a look.

de-nordic

I have enough #1754

davidvincze · 2023-07-21T13:33:40Z

I have enough #1754

Could you please elaborate on this in the focus of this change? You mention encryption in the issue, and I know there is at least one PR that I know of introducing some modifications to the encryption code, but in this case it's about the signature.
If this PR is breaking something I would like to know about it and I would like to understand how.

de-nordic · 2023-07-21T13:47:33Z

I have enough #1754

Could you please elaborate on this in the focus of this change? You mention encryption in the issue, and I know there is at least one PR that I know of introducing some modifications to the encryption code, but in this case it's about the signature. If this PR is breaking something I would like to know about it and I would like to understand how.

Build me MCUboot for Zephyr with BOOT_ENCRYPT_EC256 enabled; you can use latest Zephyr with current mcutools/main, preferred platform is nrf52840dk.

This commit enables ECDSA signature verification using PSA Crypto API. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I51c7aadba03348f335e89d9252e70c09f8787f30

davidvincze · 2023-08-03T08:11:43Z

@de-nordic may I ask you to review the changes around the hash functions, just to make sure these modifications won't cause you any problems with the integration work. Please request changes or approve because currently this PR is blocked for unrelated reasons thus delaying our work as well.

Thank you!

davidvincze · 2023-08-03T08:12:59Z

@Roolli please check the ext/nrf/cc310_glue.h file and update it as required.

de-nordic · 2023-08-03T08:27:22Z

@Roolli please check the ext/nrf/cc310_glue.h file and update it as required.

Here is commit, on sdk-nrf, that fixes th cc310 for us: nrfconnect/sdk-mcuboot@ae4344b

davidvincze · 2023-08-03T15:55:45Z

@Roolli please check the ext/nrf/cc310_glue.h file and update it as required.

Here is commit, on sdk-nrf, that fixes th cc310 for us: nrfconnect/sdk-mcuboot@ae4344b

Thank you for the patch. Is this broken? Has the CC-310 API been changed in the meantime?

Could you recommend a preferred Zephyr platform with CC-310 to test these modifications with?

oberon-sk · 2023-08-07T17:43:11Z

.github/workflows/sim.yaml

@@ -44,6 +44,8 @@ jobs:
        - "sig-rsa validate-primary-slot ram-load multiimage"
        - "sig-rsa validate-primary-slot direct-xip multiimage"
        - "sig-ecdsa hw-rollback-protection multiimage"
+        - "sig-ecdsa-psa psa-crypto-api"


towards a consistent naming, psa should not be specified twice
how about:

sig-ecdsa psa-crypto-api sig-p256 sig-ecdsa psa-crypto-api sig-p384

or

sig-ecdsa-psa sig-p256 sig-ecdsa-psa sig-p384

the latter fits better with the previous scheme and the psa-crypto-api feature should then be removed

(both proposals require changes in built.rs)

Hi, thanks for the suggestions. I've implemented the second approach as it fits the current scheme more, but I opted not to add an algorithm specifier for p256 as that's the default.

oberon-sk · 2023-08-15T13:03:42Z

sim/mcuboot-sys/build.rs

@@ -15,6 +15,8 @@ fn main() {
    let sig_rsa3072 = env::var("CARGO_FEATURE_SIG_RSA3072").is_ok();
    let sig_ecdsa = env::var("CARGO_FEATURE_SIG_ECDSA").is_ok();
    let sig_ecdsa_mbedtls = env::var("CARGO_FEATURE_SIG_ECDSA_MBEDTLS").is_ok();
+    let sig_ecdsa_psa = env::var("CARGO_FEATURE_SIG_ECDSA_PSA").is_ok();
+    let sig_use_p384 = env::var("CARGO_FEATURE_SIG_USE_P384").is_ok();


remove "use" for consistency

nvlsianpu

I reviewed the code roughly,
looks sane to me. Don't see any regression.

nvlsianpu

@de-nordic Can you run your smoke tests suite on this PR?

This commit enables ECDSA signature verification using PSA Crypto API. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I33f559ecdd59b1ce41c6a2d5f315212300d585e3

Currently all the hashing functionality is done with SHA256 but if we would like to use ECDSA-P384 that requires SHA384 as the hashing algorithm, but MCUboot is using SHA256 for image hashing and public key hashing. This commit modifies the hashing operations to use SHA384 thus SHA256 can be omitted which is beneficial from a code size standpoint. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I59230f76f88e0b42ad6383b2c9b71b73f33d7dd7

Currently all the hashing functionality is done with SHA256 but if we would like to use ECDSA-P384 that requires SHA384 as the hashing algorithm. However, MCUboot is using SHA256 for image hashing and public key hashing. This commit modifies the hashing operations to use SHA384 thus SHA256 can be omitted which is beneficial from a code size standpoint. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I364eefe334e4fe6668b8a3b97991b5dbb0c80104

Add ECDSA verification tests to the CI using the PSA Crypto API Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I904c8929f355ec791ff28ac7c3e0ca3832b2403d

d3zd3z · 2023-09-05T16:53:21Z

So, one question that isn't clear to me. How are we distinguishing 256 and 384 bit ECDSA signatures in the TLV? Do we just rely on the size of the signature? What happens if there is a 256 bit signature in the TLV, but the build is configured for 384 bit signatures? Does it fail properly, or does something confusing happen.

I thought we were going to add an extra TLV to specify the signature size that should be given before the ECDSA signature, and would default to 256, but in the 384 case would be present to suggest a new signature length?

adeaarm · 2023-09-05T20:07:45Z

So, one question that isn't clear to me. How are we distinguishing 256 and 384 bit ECDSA signatures in the TLV? Do we just rely on the size of the signature? What happens if there is a 256 bit signature in the TLV, but the build is configured for 384 bit signatures? Does it fail properly, or does something confusing happen.

I thought we were going to add an extra TLV to specify the signature size that should be given before the ECDSA signature, and would default to 256, but in the 384 case would be present to suggest a new signature length?

Signatures encoding in ASN-1 is

 SEQUENCE  {
     INTEGER r
     INTEGER s
 }

This means that the length information is part of the signature, in the length field of the ASN-1 TLV. When the signature is verified this structure is parsed and if the length does not match the length of (r,s) the verification fails. This is similar to how the curve information is encoded as part of the key encoding.

d3zd3z · 2023-09-06T14:11:03Z

This does work to distinguish a given 256 bit key and a given 384 bit key. However, there are multiple curves possible at each of these sizes. Perhaps we are just saying in MCUboot that we will only ever support one key of a given size, but it seems like it might be a good idea to at least think of encoding the actual key type used, as the size isn't sufficient to determine which key is being used.

adeaarm · 2023-09-06T14:54:43Z

This does work to distinguish a given 256 bit key and a given 384 bit key. However, there are multiple curves possible at each of these sizes. Perhaps we are just saying in MCUboot that we will only ever support one key of a given size, but it seems like it might be a good idea to at least think of encoding the actual key type used, as the size isn't sufficient to determine which key is being used.

From image.h

/*
 * Image trailer TLV types.
 *
 * Signature is generated by computing signature over the image hash.
 * Currently the only image hash type is SHA256.
 *
 * Signature comes in the form of 2 TLVs.
 *   1st on identifies the public key which should be used to verify it.
 *   2nd one is the actual signature.
 */

the public key for ECDSA encodes the OID of the curve in ASN-1:

 SEQUENCE {
    SEQUENCE {
        OBJECT idEcPublicKey
        OBJECT namedCurve
    }
    BIT STRING publicKey
 }

i.e. from namedCurve you have the precise information about the curve used and this TLV is always present with the actual signature TLV (encoded as shown in the other comment). Please correct me if I am making wrong assumptions here, but I believe you have all the information you need already in the ASN-1 encodings to precisely identify the curve used and the associated signature pair.

d3zd3z · 2023-09-07T14:36:12Z

The public key has the OID of the algorithm used, but the signature itself is just r and s encoded in asn.1. We do reference a hash of the public key, so perhaps that is sufficient. I'm just going off other signature formats, such as COSE where the algorithm is always encoded as part of the signature.

Roolli force-pushed the psa-ecdsa-enablement branch from 2fbdb4f to 458b69b Compare June 29, 2023 12:45

thedjnK suggested changes Jun 29, 2023

View reviewed changes

boot/bootutil/include/bootutil/caps.h Outdated Show resolved Hide resolved

Roolli force-pushed the psa-ecdsa-enablement branch 2 times, most recently from 79b113e to d698133 Compare June 30, 2023 16:28

adeaarm approved these changes Jul 14, 2023

View reviewed changes

davidvincze reviewed Jul 14, 2023

View reviewed changes

Roolli force-pushed the psa-ecdsa-enablement branch 2 times, most recently from 7b91822 to 39a49cc Compare July 20, 2023 14:53

de-nordic requested changes Jul 21, 2023

View reviewed changes

davidvincze mentioned this pull request Jul 26, 2023

EC256 encryption is broken when building MCUboot for Zephyr #1758

Closed

bootutil: PSA Crypto ECDSA enablement

6dd067c

This commit enables ECDSA signature verification using PSA Crypto API. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I51c7aadba03348f335e89d9252e70c09f8787f30

Roolli force-pushed the psa-ecdsa-enablement branch from 39a49cc to d28eeea Compare July 28, 2023 13:03

oberon-sk reviewed Aug 7, 2023

View reviewed changes

Roolli force-pushed the psa-ecdsa-enablement branch from d28eeea to 383338e Compare August 11, 2023 11:41

oberon-sk reviewed Aug 15, 2023

View reviewed changes

davidvincze requested a review from de-nordic August 21, 2023 06:39

nvlsianpu reviewed Aug 21, 2023

View reviewed changes

Roland Mikhel added 4 commits August 22, 2023 09:39

sim: PSA Crypto ECDSA enablement

9cb0fdd

This commit enables ECDSA signature verification using PSA Crypto API. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I33f559ecdd59b1ce41c6a2d5f315212300d585e3

ci: Add test cases for ECDSA using PSA Crypto

b885890

Add ECDSA verification tests to the CI using the PSA Crypto API Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I904c8929f355ec791ff28ac7c3e0ca3832b2403d

Roolli force-pushed the psa-ecdsa-enablement branch from 383338e to b885890 Compare August 22, 2023 08:10

de-nordic approved these changes Sep 7, 2023

View reviewed changes

de-nordic self-requested a review September 7, 2023 16:09

d3zd3z approved these changes Sep 8, 2023

View reviewed changes

davidvincze merged commit 5c00da4 into mcu-tools:main Sep 12, 2023
55 checks passed

Roolli deleted the psa-ecdsa-enablement branch September 13, 2023 06:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ECDSA signature support to PSA Crypto backend #1734

Add ECDSA signature support to PSA Crypto backend #1734

Roolli commented Jun 29, 2023

davidvincze commented Jul 14, 2023

de-nordic commented Jul 14, 2023

de-nordic left a comment

davidvincze commented Jul 21, 2023

de-nordic commented Jul 21, 2023 •

edited

Loading

davidvincze commented Aug 3, 2023 •

edited

Loading

davidvincze commented Aug 3, 2023

de-nordic commented Aug 3, 2023

davidvincze commented Aug 3, 2023 •

edited

Loading

oberon-sk Aug 7, 2023 •

edited

Loading

Roolli Aug 14, 2023

oberon-sk Aug 15, 2023

nvlsianpu left a comment •

edited

Loading

nvlsianpu left a comment

d3zd3z commented Sep 5, 2023

adeaarm commented Sep 5, 2023

d3zd3z commented Sep 6, 2023

adeaarm commented Sep 6, 2023

d3zd3z commented Sep 7, 2023

Add ECDSA signature support to PSA Crypto backend #1734

Add ECDSA signature support to PSA Crypto backend #1734

Conversation

Roolli commented Jun 29, 2023

davidvincze commented Jul 14, 2023

de-nordic commented Jul 14, 2023

de-nordic left a comment

Choose a reason for hiding this comment

davidvincze commented Jul 21, 2023

de-nordic commented Jul 21, 2023 • edited Loading

davidvincze commented Aug 3, 2023 • edited Loading

davidvincze commented Aug 3, 2023

de-nordic commented Aug 3, 2023

davidvincze commented Aug 3, 2023 • edited Loading

oberon-sk Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Roolli Aug 14, 2023

Choose a reason for hiding this comment

oberon-sk Aug 15, 2023

Choose a reason for hiding this comment

nvlsianpu left a comment • edited Loading

Choose a reason for hiding this comment

nvlsianpu left a comment

Choose a reason for hiding this comment

d3zd3z commented Sep 5, 2023

adeaarm commented Sep 5, 2023

d3zd3z commented Sep 6, 2023

adeaarm commented Sep 6, 2023

d3zd3z commented Sep 7, 2023

de-nordic commented Jul 21, 2023 •

edited

Loading

davidvincze commented Aug 3, 2023 •

edited

Loading

davidvincze commented Aug 3, 2023 •

edited

Loading

oberon-sk Aug 7, 2023 •

edited

Loading

nvlsianpu left a comment •

edited

Loading