Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mpg opened this issue Feb 9, 2023 · 2 comments · Fixed by #7554
Closed

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

mpg opened this issue Feb 9, 2023 · 2 comments · Fixed by #7554
Assignees
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Feb 9, 2023

Background: see #6009 and in particular #6009 (comment).

When USE_PSA_CRYPTO is enabled, the PK module should:

  • at parsing time, store public ECC keys as a byte array (suitable as input to psa_import_key()) in addition to storing them as an ecp_keypair
  • when using the key (pk_verify()) or writing it out (pk_write_pubkey*()) use that array in preference to the ecp_keypair structure.

This reduces the reliance on ECP (and ultimately) bignum functions. Note: for now we're keeping the ecp_keypair structure for compatibility reasons, due to the existence of the public API function mbedtls_pk_ec(). In the future we'll probably remove it when ECP_C is disabled.

Suggested course of action:

  1. Add two new members (pointer, length) to mbedtls_pk_context to hold the serialized key. Adapt pk_init() and pk_free() accordingly.
  2. Parsing: all possible ECC public key formats end up calling pk_get_ecpubkey(), which takes as input exactly the EC Point that psa_import_key() wants. Make this function copy its input to the new members of the mbedtls_pk_context. (Note: actually this only works for uncompressed points (ie, input starts with 0x04). For compressed points, until PSA supports them, we have no choice but to call mbedtls_ecp_write_point_binary() to get the uncompressed form from key->Q. This leave one parse-serialize cycle, but at least it's isolated in pk_get_ecpubkey().
  3. Verify: in ecdsa_verify_wrap() we no longer need to call mbedtls_ecp_point_write_binary() and can directly get the input to psa_import_key() from the new members of pk_context.
  4. Write: similarly to parse, all possible formats end up calling pk_write_ec_pubkey(). Again, instead of calling mbedtls_ecp_point_write_binary() we can just copy from the new members of pk_context.

Depends on:

Follow-up: #7074 same for private keys.

@valeriosetti
Copy link
Contributor

After the study work done with PR #7202 and once all PRs related to issue #7460 will be addressed, here is how the (new) solving PR should be shaped (as suggested here):

  • Add the new fields and their management in init()/free().
  • Populate the new fields during parsing.
  • Populate the new fields in the key generation function in tests.
  • Start using the new fields when writing out the key.
  • Start using the new fields when doing crypto operations.

@valeriosetti
Copy link
Contributor

valeriosetti commented May 11, 2023

Discussion in #7570 contains extra details to be implemented in solving PR, In particular refer to this and this comments.
I'm also updating the title accordingly

@valeriosetti valeriosetti changed the title Avoid parse/unparse public ECC keys in PK with USE_PSA Extend pk_context structure to include public key in raw format when USE_PSA is enabled May 11, 2023
@valeriosetti valeriosetti changed the title Extend pk_context structure to include public key in raw format when USE_PSA is enabled Avoid parse/unparse public ECC keys in PK with USE_PSA when !ECP_C May 15, 2023
@mpg mpg closed this as completed in #7554 May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-m Estimated task size: medium (~1w)
Projects
None yet
2 participants