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

valeriosetti · 2023-03-02T18:18:10Z

Description

This PR tries to avoid the usage of ECP/MPI functions for parsing/writing public keys in PK module by keeping its content in raw format inside the mbedtls_pk_context structure. Besides this raw format also fits well with what PSA expexts for import.

Resolves #7073

Depends on #7387 for a possible code improvement explained here

Note

According to the description of #7074 this seems to be an initial attempt which will include in the future also private keys. As a consequence I expect it will undergo a deep review process, so I keep it drafted until I got some initial feedback from @mpg and the CI passing all the tests.

Gatekeeper checklist

changelog not provided - will be added in driver-only ECC: ECPf wrap-up #7452
backport not required
tests provided (maybe more are required for the new functions I added?)

mprse

This is just partial review. I will be off till the end of the week so I'm posting my current review state.

Few general comments:

I think we do not need to use MBEDTLS_PRIVATE in library code as MBEDTLS_ALLOW_PRIVATE_ACCESS is enabled for library.
for result checks we usually use (ret != 0) not (ret < 0) I think we should stick to this form.
Not sure if we should check null pointers. For sure we are not doing this for PSA. Needs to be clarified.

tests/suites/test_suite_pk.function

library/pkwrite.c

library/pk.c

library/pk_wrap.c

tests/suites/test_suite_debug.data

valeriosetti · 2023-03-16T10:36:02Z

Rebased to solve conflicts with development. I work on the required changes on following commits

mprse

Thanks for addressing last comments.
Added few more comments.

General comments from previous review still stand:

There are still places where if (ret < 0) checks are used.
Accessing private fields in library can be done without MBEDTLS_PRIVATE (should be confirmed by @mpg)
NULL checks (should be confirmed by @mpg)

tests/suites/test_suite_pk.function

tests/suites/test_suite_x509parse.data

library/pkwrite.c

library/pkparse.c

library/pk.c

library/pkparse.c

valeriosetti · 2023-03-20T15:21:53Z

General comments from previous review still stand:

@mprse Sorry, I forgot to fix those. A part from the "NULL point check", now the other 2 points should be addressed now

mprse · 2023-03-24T08:57:21Z

Seems that Travis is unhappy. test_suite_debug test failed in full config.

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

In this way the new mechanism is embraced by USE_PSA symbols and it's more suitable for future changes (where the ECP dependency is going to be dropped or at least make optional). Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

Differently from pkparse though in this case we cannot keep simultaneously the "old" keypair and the "new" raw format at the same time. Therefore the USE_PSA symbol is used to select from which side the public key is to be taken. Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

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

mpg · 2023-04-19T08:36:15Z

Add missing USE_PSA_INIT()/DONE() in tests - actually while at it we could do this in a systematic way: according to our own documentation all PK, X.509 and SSL tests should have it (unless they already have MD_OR_USE_PSA_INIT()/DONE() of course, or a plain PSA_INIT()/PSA_DONE() for 1.3 tests). (Note: there's one in mbedtls_test_ssl_perform_handshake(), so test functions that are merely a wrapper around that don't need it.)

Actually could you create a separate PR for that? This is something we'll want to backport.

Note: #7443 is doing the equivalent with programs, so after #7443 and the PR I'm requesting above, this class of problems should be fixed for good.

mpg

I think the general design is good. I didn't review all the details, since as discussed before, we're going to use new PRs in order to finalize this.

mpg · 2023-04-19T08:28:44Z

library/pk_wrap.c

@@ -1430,20 +1422,21 @@ static int rsa_alt_check_pair(const void *pub, const void *prv,
    size_t sig_len = 0;
    int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;

-    if (rsa_alt_get_bitlen(prv) != rsa_get_bitlen(pub)) {
+    if (rsa_alt_get_bitlen((mbedtls_pk_context *) prv) !=


I don't think a function that accepts a const pointer to cast it to non-const internally. Either all the other functions called can be made to take a const argument as well, and then we should do it, or the function should take a non-const argument.

(This can be acceptable for a prototype, but will need fixing when finalizing things.)

Note: I happened to comment here because I was reading things out of order, but that holds for all other functions.

I also noticed that there is a little mix between const and non-const parameters in pk_wrap functions. I would tend to go for the non-const solution since this might take some castings away and simplify code readings.
However I will investigate further before taking any direction when I'll have to work on this part

Yes, I think we should definitely start with everything non-const, get the CI to pass (perhaps even merge), and only then try adding const everywhere and see if that works or if we need to be more clever - or perhaps abandon the whole const idea.

mpg · 2023-04-19T08:36:46Z

programs/ssl/ssl_context_info.c

@@ -937,6 +937,10 @@ int main(int argc, char *argv[])
     * file name is correct */
    parse_arguments(argc, argv);

+#if defined(MBEDTLS_USE_PSA_CRYPTO)
+    psa_crypto_init();


Note: this is superseded by #7443

mpg · 2023-04-19T08:41:49Z

library/pk.c

+        return MBEDTLS_PK_NONE;
+    }
+    /* RSA does not support raw public keys inside the pk_context structure,
+     * so we quit silently in this case */


Like mbedtls_pk_update_public_key_from_keypair() above, we should list types that are supported instead.

mpg · 2023-04-19T08:47:47Z

library/pk_wrap.c

-    ret = mbedtls_ecp_point_write_binary(&ctx->grp, &ctx->Q,
-                                         MBEDTLS_ECP_PF_UNCOMPRESSED,
-                                         &key_len, buf, sizeof(buf));
+    ret = mbedtls_pk_get_public_key(pk, buf, sizeof(buf), &key_len);


Minor/optimisation: Do we really need to make a copy here?

Right, this is totally useless (also the mbedtls_pk_get_public_key function itself since it used only here). I think I got slightly confused during the development of this PR by "who" is accessing internal data of the pk_context structure. I mean, if it's from "inside" the library, then there is no need for an extra function to get that kind of data: we can simply read the raw buffer directly.

Clearly if it's from within PK itself, we can and should read the raw buffer directly. If it's from higher-level modules (X.509, TLS), then accessing struct members directly is technically possible, but we should think whether it's the right thing to do or if an internal API would be better - loose coupling and all that.

Probably depends on the situation, but in the past we've erred too much on the side of high-level modules relying on internal details of very low-level modules such as ECP or Bignum, so please do not blindly follow what existing code is doing when it comes to that kind of question :)

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

This PR is reshaped in #7554 so I'm closing this one

valeriosetti requested a review from mpg March 2, 2023 18:18

valeriosetti self-assigned this Mar 2, 2023

valeriosetti added enhancement needs-work component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-m Estimated task size: medium (~1w) labels Mar 2, 2023

valeriosetti marked this pull request as ready for review March 8, 2023 14:13

valeriosetti force-pushed the issue7073 branch 3 times, most recently from 8296bed to 3db22f9 Compare March 10, 2023 13:36

mprse self-requested a review March 14, 2023 11:42

mprse reviewed Mar 15, 2023

View reviewed changes

valeriosetti removed the needs-ci Needs to pass CI tests label Mar 16, 2023

valeriosetti force-pushed the issue7073 branch from 690aa53 to b103855 Compare March 16, 2023 10:35

valeriosetti force-pushed the issue7073 branch from c3b722e to 86ed005 Compare March 16, 2023 13:04

valeriosetti requested a review from mprse March 16, 2023 13:04

valeriosetti removed the needs-work label Mar 16, 2023

valeriosetti force-pushed the issue7073 branch from 86ed005 to a7fef86 Compare March 17, 2023 08:01

mprse reviewed Mar 20, 2023

View reviewed changes

valeriosetti force-pushed the issue7073 branch 4 times, most recently from cc7296e to 3f361e1 Compare March 24, 2023 08:11

valeriosetti added needs-review Every commit must be reviewed by at least two team members, and removed needs-reviewer This PR needs someone to pick it up for review labels Mar 24, 2023

valeriosetti added 15 commits April 14, 2023 13:55

pk: replace platform_zeroize with memset

7565547

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

pk: check for correct key type when trying the new public key management

5cb8084

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

test: use USE_PSA_INIT/DONE macros instead of USE_PSA/DONE

2088ab7

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

pk_wrap: eckey_check_pair_psa() to use raw buffer for public key

43584b5

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

test: fix missing USE_PSA_INIT/DONE

10b4fff

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

remove extra white spaces

89aba1c

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

remove some leftover changes from previous design

a0011bd

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

pk: move internal function out of public header file

14b8094

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

test: improve pk_genkey_ec() function

71c7eb9

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

pk: use the same calling convention for all key types

9bedc2b

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

removing leftovers from initial development

0ea4c87

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

revert change to mbedtls_pk_rsa()

5a02721

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

pk: use same calling convention for pk_wrap functions

337a348

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

valeriosetti force-pushed the issue7073 branch from e8bab6a to 337a348 Compare April 17, 2023 09:37

valeriosetti requested a review from mpg April 17, 2023 09:38

pk: fix some rebase issue related to ECP_LIGHT

d838adb

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

mpg reviewed Apr 19, 2023

View reviewed changes

mpg mentioned this pull request Apr 19, 2023

Read and write RFC8410 keys #6838

Merged

3 tasks

valeriosetti mentioned this pull request Apr 19, 2023

Improve pk_wrap calling convention and check for remaning ECP dependencies in PK files #7460

Closed

3 tasks

minor optimizations

0edd108

valeriosetti force-pushed the issue7073 branch from 4652c20 to 0edd108 Compare April 19, 2023 10:59

This was referenced Apr 20, 2023

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

Closed

Check remaning dependencies on ECP in PK module #7465

Merged

valeriosetti mentioned this pull request May 5, 2023

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

Merged

3 tasks

valeriosetti closed this May 5, 2023

valeriosetti deleted the issue7073 branch December 6, 2024 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

valeriosetti commented Mar 2, 2023 •

edited

Loading

mprse left a comment

valeriosetti commented Mar 16, 2023 •

edited

Loading

mprse left a comment

valeriosetti commented Mar 20, 2023

mprse commented Mar 24, 2023 •

edited

Loading

mpg commented Apr 19, 2023

mpg left a comment

mpg Apr 19, 2023

valeriosetti Apr 19, 2023

mpg Apr 19, 2023

mpg Apr 19, 2023

mpg Apr 19, 2023

mpg Apr 19, 2023

valeriosetti Apr 19, 2023

mpg Apr 19, 2023

valeriosetti commented May 5, 2023

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

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

Conversation

valeriosetti commented Mar 2, 2023 • edited Loading

Description

Note

Gatekeeper checklist

mprse left a comment

Choose a reason for hiding this comment

valeriosetti commented Mar 16, 2023 • edited Loading

mprse left a comment

Choose a reason for hiding this comment

valeriosetti commented Mar 20, 2023

mprse commented Mar 24, 2023 • edited Loading

mpg commented Apr 19, 2023

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriosetti commented May 5, 2023

valeriosetti commented Mar 2, 2023 •

edited

Loading

valeriosetti commented Mar 16, 2023 •

edited

Loading

mprse commented Mar 24, 2023 •

edited

Loading