Remove pkwrite dependency in pk using PSA for ECDSA #6389

gilles-peskine-arm · 2022-10-04T21:08:17Z

Remove the dependency on MBEDTLS_PK_WRITE_C when MBEDTLS_PSA_CRYPTO_C and MBEDTLS_PK_C are enabled but not MBEDTLS_RSA_C, by no longer using pkwrite functions for ECDSA.

Status: ~~needs to be rebased now that #6408 has been fixed.~~ done
~~Depends on #6970~~

Gatekeeper checklist

changelog not required (optimization only)
backport not required (optimization only)
tests provided (covered by existing tests)

include/mbedtls/build_info.h

valeriosetti

In general it LGTM. I just have a couple of comments/suggestions for defines

valeriosetti · 2023-01-16T08:58:33Z

library/pk_wrap.c

+    unsigned char buf[PSA_KEY_EXPORT_ECC_PUBLIC_KEY_MAX_SIZE(
+            PSA_VENDOR_ECC_MAX_CURVE_BITS )];


Isn't it possible to use the MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH define here? It seems to me that it should be equivalent

valeriosetti · 2023-01-16T09:36:24Z

library/pk_wrap.c

+    unsigned char buf[PSA_KEY_EXPORT_ECC_KEY_PAIR_MAX_SIZE(
+            PSA_VENDOR_ECC_MAX_CURVE_BITS )];


In case the above comment makes sense to you, what about creating a define to MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH for this purpose?
Perhaps PSA_EXPORT_PUBLIC_KEY_MAX_SIZE is already OK for this?

gilles-peskine-arm · 2023-01-26T11:33:08Z

I held off on this while other high-priority work was touching pk. Now I think it would help with the ongoing work on driver-only ECC. But I don't have time for it right now — if someone has time now and wants to adopt it, please feel free. Otherwise I'll get around to it eventually.

valeriosetti · 2023-01-26T12:49:40Z

if someone has time now and wants to adopt it, please feel free

I'll be glad to complete this work ;)

valeriosetti · 2023-01-27T15:32:13Z

I just did a force push in order to align with the new code style

Under MBEDTLS_USE_PSA_CRYPTO, ecdsa_verify_wrap() was calling mbedtls_pk_write_pubkey() to write a public key in the form of a subjectPublicKey, only to then extract the part that represents the EC point which psa_import_key() actually wants. Instead, call an ecp function to directly get the public key in the desired format (just the point). This slightly reduces the code size and stack usage, and removes a dependency on pk_write. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Under MBEDTLS_USE_PSA_CRYPTO, ecdsa_sign_wrap() was calling mbedtls_pk_write_key_der() to write a private key in SEC1 format, only to then extract the part that represents the private value which is what psa_import_key() actually wants. Instead, call an mpi function to directly get the private key in the desired format. This slightly reduces the code size and stack usage, and removes a dependency on pk_write. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The dependency is still useful for RSA, for which PSA encodes keys with an ASN.1 structure. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

To better reflect what the code relies on, limit the headers that are included when MBEDTLS_USE_PSA_CRYPTO is disabled. Also stop including "pkwrite.h" when it is no longer needed. Include "mbedlts/platform_util.h" unconditionally. It was only included for RSA ALT but was also used for MBEDTLS_USE_PSA_CRYPTO (the code worked because other headers include "mbedtls/platform_util.h"). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti · 2023-02-08T12:51:26Z

Following the suggestion made previously and since #6970 was just merged into development, this branch was rebased

mpg

LGTM.

I've verified that after the last commit there is no occurrence of ECDSA_C left in pk.c and pk_wrap.c other than (1) those associated with restartable and (2) those that guard #include "mbedtls/ecdsa.h" .

mprse

LGTM, left one question.

mprse · 2023-02-24T09:37:42Z

library/pk_wrap.c

+     * is not currently supported in PSA, the public key is one byte longer
+     * (header byte + 2 numbers, while the signature is only 2 numbers),
+     * so use that as the buffer size. */
+    unsigned char buf[MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH];


This is buffer on stack and holds public key or signature, but just to confirm. Shouldn't this buffer be cleared in cleanup stage?

Ah, excellent question! As the name implies, public keys are not supposed to be sensitive data so they don't need to be wiped out. I'd say signatures are not sensitive either in general, but I may be missing some corner cases, so I'll ask on slack.

gilles-peskine-arm added enhancement component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests priority-medium Medium priority - this can be reviewed as time permits labels Oct 4, 2022

gilles-peskine-arm force-pushed the ecdsa-use-psa-without-pkwrite branch from a6fb419 to cf4e95e Compare October 5, 2022 17:49

gilles-peskine-arm added the size-s Estimated task size: small (~2d) label Oct 5, 2022

gilles-peskine-arm force-pushed the ecdsa-use-psa-without-pkwrite branch from cf4e95e to f9c1cec Compare October 5, 2022 20:19

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Oct 6, 2022

gilles-peskine-arm commented Oct 11, 2022

View reviewed changes

include/mbedtls/build_info.h Show resolved Hide resolved

gilles-peskine-arm removed the needs-preceding-pr Requires another PR to be merged first label Dec 7, 2022

gilles-peskine-arm mentioned this pull request Jan 5, 2023

Study: PK including parse/write in ECP-free builds #6009

Closed

mpg added the priority-medium Medium priority - this can be reviewed as time permits label Jan 6, 2023

mpg self-requested a review January 6, 2023 09:27

valeriosetti self-requested a review January 16, 2023 08:57

valeriosetti reviewed Jan 16, 2023

View reviewed changes

valeriosetti self-assigned this Jan 26, 2023

valeriosetti force-pushed the ecdsa-use-psa-without-pkwrite branch from 2099adb to 368fed2 Compare January 27, 2023 15:31

valeriosetti added needs-ci Needs to pass CI tests and removed needs-work needs-ci Needs to pass CI tests labels Jan 30, 2023

valeriosetti removed the needs-work label Feb 8, 2023

mpg previously approved these changes Feb 8, 2023

View reviewed changes

gilles-peskine-arm and others added 13 commits February 8, 2023 13:39

pk no longer needs pk_write for ECDSA with MBEDTLS_USE_PSA_CRYPTO

bbccdd4

The dependency is still useful for RSA, for which PSA encodes keys with an ASN.1 structure. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The pk_psa_sign test function needs pk_write

be9e2a1

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

pk_wrap: fix for DETERMINISTIC_ECDSA case in ecdsa_sign_wrap()

5bc5224

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

pk_wrap: use specific lengths for EC's private key and key-pair

1337a4f

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

build_info: fix PK's requirements for RSA_C

d0b83e1

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

fix code style

b761b15

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

pk_wrap: fix comment in ecdsa_verify_wrap

5c032b5

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

test: pk: keep PK_WRITE_C only in RSA tests

a1e3e3a

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

fix code style

683a432

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

pk_wrap: use new macros for ECDSA capabilities

80d0798

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti dismissed mpg’s stale review via 80d0798 February 8, 2023 12:49

valeriosetti force-pushed the ecdsa-use-psa-without-pkwrite branch from f6a5b87 to 80d0798 Compare February 8, 2023 12:49

valeriosetti requested a review from mpg February 9, 2023 08:22

valeriosetti removed needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Feb 9, 2023

mpg approved these changes Feb 9, 2023

View reviewed changes

mpg mentioned this pull request Feb 9, 2023

Avoid parse/unparse public ECC keys in PK with USE_PSA when !ECP_C #7073

Closed

joerchan mentioned this pull request Feb 17, 2023

Wrong dependency on PK_WRITE for RSA_C when PSA_CRYPTO_C is enabled #7126

Closed

mprse self-requested a review February 24, 2023 08:47

mprse approved these changes Feb 24, 2023

View reviewed changes

mprse added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Feb 24, 2023

gilles-peskine-arm merged commit 7e677fa into Mbed-TLS:development Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pkwrite dependency in pk using PSA for ECDSA #6389

Remove pkwrite dependency in pk using PSA for ECDSA #6389

gilles-peskine-arm commented Oct 4, 2022 •

edited by valeriosetti

Loading

valeriosetti left a comment

valeriosetti Jan 16, 2023

valeriosetti Jan 16, 2023

gilles-peskine-arm commented Jan 26, 2023

valeriosetti commented Jan 26, 2023

valeriosetti commented Jan 27, 2023

valeriosetti commented Feb 8, 2023

mpg left a comment

mprse left a comment

mprse Feb 24, 2023

mpg Feb 24, 2023

		unsigned char buf[PSA_KEY_EXPORT_ECC_PUBLIC_KEY_MAX_SIZE(
		PSA_VENDOR_ECC_MAX_CURVE_BITS )];

		unsigned char buf[PSA_KEY_EXPORT_ECC_KEY_PAIR_MAX_SIZE(
		PSA_VENDOR_ECC_MAX_CURVE_BITS )];

Remove pkwrite dependency in pk using PSA for ECDSA #6389

Remove pkwrite dependency in pk using PSA for ECDSA #6389

Conversation

gilles-peskine-arm commented Oct 4, 2022 • edited by valeriosetti Loading

Gatekeeper checklist

valeriosetti left a comment

Choose a reason for hiding this comment

valeriosetti Jan 16, 2023

Choose a reason for hiding this comment

valeriosetti Jan 16, 2023

Choose a reason for hiding this comment

gilles-peskine-arm commented Jan 26, 2023

valeriosetti commented Jan 26, 2023

valeriosetti commented Jan 27, 2023

valeriosetti commented Feb 8, 2023

mpg left a comment

Choose a reason for hiding this comment

mprse left a comment

Choose a reason for hiding this comment

mprse Feb 24, 2023

Choose a reason for hiding this comment

mpg Feb 24, 2023

Choose a reason for hiding this comment

gilles-peskine-arm commented Oct 4, 2022 •

edited by valeriosetti

Loading