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

MD: HMAC in DTLS cookies #5174

Closed
5 tasks done
mpg opened this issue Nov 16, 2021 · 2 comments · Fixed by #5602
Closed
5 tasks done

MD: HMAC in DTLS cookies #5174

mpg opened this issue Nov 16, 2021 · 2 comments · Fixed by #5602
Assignees
Labels
enhancement size-s Estimated task size: small (~2d)

Comments

@mpg
Copy link
Contributor

mpg commented Nov 16, 2021

In ssl_cookie.c, when MBEDTLS_USE_PSA_CRYPTO is enabled, move from using the mbedtls_md_hmac API to the psa_mac API, and from using the user-provided RNG for key generation to using psa_generate_key().

  • In struct mbedtls_ssl_cookie_ctx, store the key as a mbedtls_svc_key_id_t instead of a mbedtls_md_context_t, with corresponding adaptations in mbedtls_ssl_cookie_init() / mbedtls_ssl_cookie_free().
  • In mbedtls_ssl_cookie_setup(), set up appropriate attributes and use psa_generate_key().
  • In ssl_cookie_hmac(), use psa_mac_sign_setup(), psa_mac_update() and psa_mac_sign_finish().

Note: the hmac_ctx in the cookie_ctx serves two purposes: (1) store the key, and (2) provide a context for multi-part operations. With PSA those roles are distinct; only the key needs to be in the context, the psa_mac_operation_t object should be internal to ssl_cookie_hmac().

Possible follow-ups (either do them in the same PR if easy enough, or open (an) issue(s) to track):

  • When MBEDTLS_USE_PSA_CRYPTO is enabled, we no longer need a mutex in the cookie_ctx to protect the mbedtls_md_context_t, since the psa_mac_operation_t object is now local to the function that uses it. OTOH, when a serial field is present, incrementing it should be protected by the mutex (this is a pre-existing bug).
  • The current implementation only uses psa_mac_sign and, for verification, does the constant-time memcmp() manually. Would it be better to use psa_mac_verify? Perhaps not (less code sharing between generation and verification). We need to make a decision.
@mpg mpg added enhancement Product Backlog size-s Estimated task size: small (~2d) labels Nov 16, 2021
@mpg mpg added this to Incoming Items in OBSOLETE - SEE https://github.com/orgs/Mbed-TLS/projects/3 via automation Nov 16, 2021
@mpg mpg moved this from 2.x pre-LTS work to Use PSA Crypto More in OBSOLETE - PLEASE SEE https://github.com/orgs/Mbed-TLS/projects/1 Nov 16, 2021
@mpg mpg changed the title Use PSA for HMAC in DTLS cookies HMAC: DTLS cookies Dec 7, 2021
@mpg mpg changed the title HMAC: DTLS cookies MD: HMAC in DTLS cookies Dec 7, 2021
@mpg mpg moved this from Use PSA Crypto more - part 1 to Use PSA Crypto more - part 2 in OBSOLETE - PLEASE SEE https://github.com/orgs/Mbed-TLS/projects/1 Dec 15, 2021
@superna9999 superna9999 self-assigned this Mar 4, 2022
@superna9999
Copy link
Contributor

@mpg I implemented both follow-ups, and I have some remarks:

  • mutex: while removing it for PSA API usage is right, indeed serial should be protected. Is there an issue linked to that ?
  • psa_mac_verify_XX: there is no clean way to share code with non-PSA while using psa_mac_verify_XX API, but adding the PSA verify code in mbedtls_ssl_cookie_check() isn't as ugly as I thought, and I think the ssl_cookie_hmac() should be non-PSA only and an inline psa_mac_sign_XX code should replace the only remaining call to ssl_cookie_hmac()

@mpg
Copy link
Contributor Author

mpg commented Mar 21, 2022

mutex: while removing it for PSA API usage is right, indeed serial should be protected. Is there an issue linked to that ?

Not that I know of (I've looked a bit and couldn't find any issue). Would you be interested in creating a PR to fix that once #5602 is merged? That would mean also creating a PR for 2.28 as bug fixes need backporting. If you'd rather not and prefer to focus on PSA work instead, no problem of course, I'll just open an issue to track this.

@mpg mpg closed this as completed in #5602 Mar 23, 2022
@daverodgman daverodgman added this to Use PSA Crypto more - part 2 in EPICs for Mbed TLS Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants