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

valeriosetti · 2023-05-05T07:33:07Z

As suggested here, this PR is the reshaped version of #7202 which includes only the changes which are relevant to solve issue #7073.

Depends on:

Resolves #7073

PR checklist

changelog not required - internal refactoring
backport just the bug fix in test_suite_debug Backport: add missing USE_PSA_INIT/DONE in test_suite_debug #7642
tests covered by existing tests

valeriosetti · 2023-05-05T14:02:42Z

Ok, even though I didn't list #7514 in the dependency list, it seems that its merging caused some conflict. I'll do a rebase

mpg · 2023-05-09T07:45:59Z

backport in general I would say that it's not required because it's an improvement. However there is a fix in the test_suite_debug which I think we missed in Fixing USE_PSA_INIT/DONE in SSL/X509/PK test suites #7461 and which should be backported

Fully agree. Most of the PR shouldn't be backported as it's a refactoring aimed at supporting new features, but the bug fix should be. Did you go over each function in test_suite_debug to check if there are others in this case? (Looks like we completely forgot about this file previously.)

If there are more changes to be done to test_suite_debug, perhaps we should have a separate PR doing just that. (And this one doesn't even need to depend on it - whichever we merge first, the other will be rebased if there are conflicts.) Wdyt?

mpg

This is clearly going in the right direction. I left a number of comments, but the general structure is looking good.

include/mbedtls/pk.h

library/pkparse.c

library/pkwrite.c

library/pkparse.c

library/pk_wrap.c

library/pk.c

mpg · 2023-05-09T09:46:23Z

@valeriosetti Btw, can you also address the comments Jerry left on 6838 as this is the first PR that builds on it?

valeriosetti · 2023-05-09T14:59:07Z

@valeriosetti Btw, can you also address the comments Jerry left on 6838 as this is the first PR that builds on it?

Done! I just skipped this comment because it was related to a line being too long, but due to the changes done in this PR it should be fine now.

mpg

Thanks for addressing my comments. Looks good to me.

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

mpg

Looking pretty good to me! I could only find minor issues in comments :)

include/mbedtls/pk.h

library/pk_internal.h

library/pkparse.c

library/pk_wrap.c

library/debug.c

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

mprse

Changes looks good.
Left few comments and propositions for minor improvements.

mprse · 2023-05-22T08:48:47Z

library/pk.c

+        return ret;
+    }
+
+    pk->ec_family = mbedtls_ecc_group_to_psa(ecp_keypair->grp.id,


I think we should check here if the conversion was successful and if not then return an error.
It is checked in ecdsa_verify_wrap function.

mprse · 2023-05-22T10:22:30Z

library/pkparse.c

+    } else {
+        /* Uncompressed format */
+        if ((end - *p) > MBEDTLS_PK_MAX_EC_PUBKEY_RAW_LEN) {
+            return MBEDTLS_ERR_PK_BAD_INPUT_DATA;


Suggested change

return MBEDTLS_ERR_PK_BAD_INPUT_DATA;

return MBEDTLS_ERR_PK_BUFFER_TOO_SMALL;

mprse · 2023-05-22T10:43:49Z

include/mbedtls/pk.h

+     * - the following fields are used for all public key operations: signature
+     *   verify, key pair check and key write.
+     * Of course, when MBEDTLS_PK_USE_PSA_EC_DATA is not enabled, the legacy
+     * ecp_keypair structure is used for storing the public key and perform


Suggested change

* ecp_keypair structure is used for storing the public key and perform

* ecp_keypair structure is used for storing the public key and performing

mprse · 2023-05-22T10:47:23Z

library/pk_wrap.c

    size_t curve_bits;
    const psa_ecc_family_t curve =
-        mbedtls_ecc_group_to_psa(prv_ctx->grp.id, &curve_bits);
+        mbedtls_ecc_group_to_psa(mbedtls_pk_ec_ro(*prv)->grp.id, &curve_bits);


I think we should check here if the conversion was successful and if not then return an error.

mprse · 2023-05-22T10:47:39Z

tests/suites/test_suite_pk.function

    psa_key_attributes_t key_attr = PSA_KEY_ATTRIBUTES_INIT;
    mbedtls_svc_key_id_t key_id = MBEDTLS_SVC_KEY_ID_INIT;
    size_t curve_bits;
-    psa_ecc_family_t curve = mbedtls_ecc_group_to_psa(grp->id,
-                                                      &curve_bits);
+    psa_ecc_family_t curve = mbedtls_ecc_group_to_psa(grp_id, &curve_bits);


I think we should check here if the conversion was successful and if not then return an error.

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

mpg

LGTM

mpg · 2023-05-23T08:13:51Z

@valeriosetti I think now would be a good time to prepare the partial backport (just the test_suite_debug fix), so we can merge this quickly once @mprse has re-reviewed.

valeriosetti · 2023-05-23T08:54:23Z

@valeriosetti I think now would be a good time to prepare the partial backport (just the test_suite_debug fix), so we can merge this quickly once @mprse has re-reviewed.

Done! Please see #7642

mprse

LGTM

valeriosetti requested a review from mpg May 5, 2023 07:33

valeriosetti self-assigned this May 5, 2023

valeriosetti changed the title ~~Issue7073 reshape~~ Avoid parse/unparse public ECC keys in PK with USE_PSA May 5, 2023

valeriosetti mentioned this pull request May 5, 2023

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

Closed

3 tasks

valeriosetti force-pushed the issue7073-reshape branch from bbd9afa to eb7662b Compare May 5, 2023 13:58

valeriosetti removed the needs-preceding-pr Requires another PR to be merged first label May 5, 2023

valeriosetti force-pushed the issue7073-reshape branch 2 times, most recently from 6989284 to e25e9c0 Compare May 8, 2023 11:31

mpg removed the needs-ci Needs to pass CI tests label May 9, 2023

valeriosetti mentioned this pull request May 9, 2023

Avoid parse/unparse private ECC keys in PK with USE_PSA when !ECP_C #7438

Merged

3 tasks

mpg requested changes May 9, 2023

View reviewed changes

valeriosetti requested a review from mpg May 9, 2023 15:48

valeriosetti requested a review from mprse May 10, 2023 07:32

mpg previously approved these changes May 10, 2023

View reviewed changes

valeriosetti added 2 commits May 17, 2023 15:38

test_suite_debug: fix USE_PSA_INIT/DONE guards in a test

92c3f36

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

tests: fixes for using the new public key raw format

483738e

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

valeriosetti force-pushed the issue7073-reshape branch from a69811b to 483738e Compare May 17, 2023 13:38

valeriosetti requested a review from mpg May 17, 2023 14:00

valeriosetti force-pushed the issue7073-reshape branch from e380c69 to fdc59a4 Compare May 17, 2023 15:36

pk: minor fixes (guards and a wrong assignment)

c1541cb

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

valeriosetti force-pushed the issue7073-reshape branch from fdc59a4 to c1541cb Compare May 17, 2023 17:23

mpg requested changes May 19, 2023

View reviewed changes

pk: fixing and improving comments

f57007d

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

valeriosetti requested a review from mpg May 19, 2023 11:55

mprse reviewed May 22, 2023

View reviewed changes

valeriosetti added 2 commits May 22, 2023 18:39

pk: add checks for the returned ECC family

a7cb845

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

pk: fix a return value and a typo in comment

016264b

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

valeriosetti requested a review from mprse May 22, 2023 16:41

mpg approved these changes May 23, 2023

View reviewed changes

mpg added the needs-backports Backports are missing or are pending review and approval. label May 23, 2023

valeriosetti mentioned this pull request May 23, 2023

Backport: add missing USE_PSA_INIT/DONE in test_suite_debug #7642

Merged

3 tasks

mprse approved these changes May 23, 2023

View reviewed changes

mpg added the approved Design and code approved - may be waiting for CI or backports label May 23, 2023

mprse removed the needs-review Every commit must be reviewed by at least two team members, label May 23, 2023

mpg removed the needs-backports Backports are missing or are pending review and approval. label May 23, 2023

mpg merged commit 9dc9204 into Mbed-TLS:development May 23, 2023

valeriosetti mentioned this pull request May 23, 2023

PK: add support for private key writing with "opaque" EC keys #7533

Merged

3 tasks

valeriosetti mentioned this pull request Jun 15, 2023

driver-only ECC: TLS: avoid use of mbedtls_ecp_point_write_binary() (with USE_PSA) #7405

Closed

valeriosetti deleted the issue7073-reshape branch December 6, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

valeriosetti commented May 5, 2023 •

edited by mpg

Loading

valeriosetti commented May 5, 2023

mpg commented May 9, 2023

mpg left a comment

mpg commented May 9, 2023

valeriosetti commented May 9, 2023

mpg left a comment

mpg left a comment

mprse left a comment

mprse May 22, 2023

mprse May 22, 2023

mprse May 22, 2023

mprse May 22, 2023

mprse May 22, 2023

mpg left a comment

mpg commented May 23, 2023

valeriosetti commented May 23, 2023

mprse left a comment

	return MBEDTLS_ERR_PK_BAD_INPUT_DATA;
	return MBEDTLS_ERR_PK_BUFFER_TOO_SMALL;

	* ecp_keypair structure is used for storing the public key and perform
	* ecp_keypair structure is used for storing the public key and performing

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

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

Conversation

valeriosetti commented May 5, 2023 • edited by mpg Loading

PR checklist

valeriosetti commented May 5, 2023

mpg commented May 9, 2023

mpg left a comment

Choose a reason for hiding this comment

mpg commented May 9, 2023

valeriosetti commented May 9, 2023

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 May 22, 2023

Choose a reason for hiding this comment

mprse May 22, 2023

Choose a reason for hiding this comment

mprse May 22, 2023

Choose a reason for hiding this comment

mprse May 22, 2023

Choose a reason for hiding this comment

mprse May 22, 2023

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented May 23, 2023

valeriosetti commented May 23, 2023

mprse left a comment

Choose a reason for hiding this comment

valeriosetti commented May 5, 2023 •

edited by mpg

Loading