PK: don't use mbedtls_ecp_check_pub_priv() when USE_PSA is enabled #7391

valeriosetti · 2023-04-03T13:03:09Z

Instead of using the legacy mbedtls_ecp_check_pub_priv() function which was based on ECP math, we add a new option named eckey_check_pair_psa() which takes advantage of PSA.

Resolves #7387

Gatekeeper checklist

changelog not required because it's an internal improvement
backport not required because it's an improvement
tests not required: using already existing tests for the legacy function

Instead of using the legacy mbedtls_ecp_check_pub_priv() function which was based on ECP math, we add a new option named eckey_check_pair_psa() which takes advantage of PSA. Of course, this is available when MBEDTLS_USE_PSA_CRYPTO in enabled. Tests were also fixed accordingly. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

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

mpg

I think there was a misunderstanding about the scope of the task, and changes should be limited to PK. A few other points, and also the CI is unhappy.

include/mbedtls/ecp.h

library/pk_wrap.c

tests/suites/test_suite_ecp.function

library/pk_wrap.c

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

mpg

LGTM.

mprse

Left only few minor suggestions. Otherwise looks good.

mprse · 2023-04-06T11:46:38Z

library/pk_wrap.c

+    if (status != PSA_SUCCESS) {
+        ret = PSA_PK_TO_MBEDTLS_ERR(status);
+        status = psa_destroy_key(key_id);
+        return (status != PSA_SUCCESS) ? PSA_PK_TO_MBEDTLS_ERR(status) : ret;
+    }
+
+    status = psa_destroy_key(key_id);
+    if (status != PSA_SUCCESS) {
+        return PSA_PK_TO_MBEDTLS_ERR(status);
+    }


Call to psa_destroy_key(key_id) is duplicated. Key is destroyed regardless of status of psa_export_public_key. Seems that this can be optimized.

Key is destroyed regardless of status of psa_export_public_key

But if psa_export_public_key() fails then we need to destroy the key that was imported few lines above before returning from this function. Am I missing something?

As for the possible optimization I thought something like this:

ret = PSA_PK_TO_MBEDTLS_ERR(psa_export_public_key(key_id, prv_key_buf, sizeof(prv_key_buf), &prv_key_len)); status = psa_destroy_key(key_id); if (ret != 0 || status != PSA_SUCCESS) { return (ret != 0) ? ret : PSA_PK_TO_MBEDTLS_ERR(status); }

Is that what you were looking for?

Ok, done ;)

mprse · 2023-04-06T11:54:51Z

library/pk_wrap.c

+    size_t pub_key_len;
+    mbedtls_svc_key_id_t key_id = MBEDTLS_SVC_KEY_ID_INIT;
+    size_t curve_bits;
+    psa_ecc_family_t curve =


Suggested change

psa_ecc_family_t curve =

const psa_ecc_family_t curve =

mprse · 2023-04-06T11:55:02Z

library/pk_wrap.c

+    size_t curve_bits;
+    psa_ecc_family_t curve =
+        mbedtls_ecc_group_to_psa(prv_ctx->grp.id, &curve_bits);
+    size_t curve_bytes = PSA_BITS_TO_BYTES(curve_bits);


Suggested change

size_t curve_bytes = PSA_BITS_TO_BYTES(curve_bits);

const size_t curve_bytes = PSA_BITS_TO_BYTES(curve_bits);

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

mprse

Thanks for addressing my comments! LGTM

mpg

LGTM

valeriosetti added enhancement needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Apr 3, 2023

valeriosetti requested a review from mpg April 3, 2023 13:03

valeriosetti self-assigned this Apr 3, 2023

valeriosetti added the size-s Estimated task size: small (~2d) label Apr 3, 2023

fixed guard position for doxygen

f3dc4a1

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

valeriosetti mentioned this pull request Apr 3, 2023

PK: use PSA to complete public key when USE_PSA is enabled #7392

Merged

3 tasks

mpg requested changes Apr 4, 2023

View reviewed changes

include/mbedtls/ecp.h Outdated Show resolved Hide resolved

library/pk_wrap.c Outdated Show resolved Hide resolved

tests/suites/test_suite_ecp.function Outdated Show resolved Hide resolved

library/pk_wrap.c Show resolved Hide resolved

valeriosetti added 2 commits April 4, 2023 10:20

pk_wrap: fix sizing for private key buffer

8eb5526

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

ecp: revert changes to ECP module and test suite

98680fc

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

valeriosetti requested a review from mpg April 4, 2023 08:23

mpg previously approved these changes Apr 4, 2023

View reviewed changes

mprse self-requested a review April 5, 2023 07:23

mpg removed needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Apr 5, 2023

mprse reviewed Apr 6, 2023

View reviewed changes

valeriosetti dismissed mpg’s stale review via 9a6be87 April 6, 2023 14:50

valeriosetti requested review from mpg and mprse April 7, 2023 06:05

pk_wrap: minor code optimizations

f286664

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

valeriosetti force-pushed the issue7387 branch from 9a6be87 to f286664 Compare April 7, 2023 06:37

mprse approved these changes Apr 7, 2023

View reviewed changes

mpg approved these changes Apr 7, 2023

View reviewed changes

mprse mentioned this pull request Apr 7, 2023

Avoid parse/unparse public ECC keys in PK with USE_PSA #7202

Closed

3 tasks

mpg 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, labels Apr 7, 2023

mpg added needs-ci Needs to pass CI tests and removed needs-ci Needs to pass CI tests labels Apr 7, 2023

mpg merged commit f740767 into Mbed-TLS:development Apr 7, 2023

valeriosetti deleted the issue7387 branch December 6, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PK: don't use mbedtls_ecp_check_pub_priv() when USE_PSA is enabled #7391

PK: don't use mbedtls_ecp_check_pub_priv() when USE_PSA is enabled #7391

valeriosetti commented Apr 3, 2023 •

edited by mpg

Loading

mpg left a comment

mpg left a comment

mprse left a comment

mprse Apr 6, 2023

valeriosetti Apr 6, 2023

valeriosetti Apr 6, 2023 •

edited

Loading

valeriosetti Apr 7, 2023

mprse Apr 6, 2023

mprse Apr 6, 2023

mprse left a comment

mpg left a comment

	size_t curve_bytes = PSA_BITS_TO_BYTES(curve_bits);
	const size_t curve_bytes = PSA_BITS_TO_BYTES(curve_bits);

PK: don't use mbedtls_ecp_check_pub_priv() when USE_PSA is enabled #7391

PK: don't use mbedtls_ecp_check_pub_priv() when USE_PSA is enabled #7391

Conversation

valeriosetti commented Apr 3, 2023 • edited by mpg Loading

Gatekeeper checklist

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mprse left a comment

Choose a reason for hiding this comment

mprse Apr 6, 2023

Choose a reason for hiding this comment

valeriosetti Apr 6, 2023

Choose a reason for hiding this comment

valeriosetti Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

valeriosetti Apr 7, 2023

Choose a reason for hiding this comment

mprse Apr 6, 2023

Choose a reason for hiding this comment

mprse Apr 6, 2023

Choose a reason for hiding this comment

mprse left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Apr 3, 2023 •

edited by mpg

Loading

valeriosetti Apr 6, 2023 •

edited

Loading