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

TLS record HMAC 2b: constant-time computation #5178

Closed
mpg opened this issue Nov 16, 2021 · 3 comments · Fixed by #5573
Closed

TLS record HMAC 2b: constant-time computation #5178

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

Comments

@mpg
Copy link
Contributor

mpg commented Nov 16, 2021

Provide an implementation of mbedtls_ct_hmac() using only PSA APIs, when MBEDTLS_USE_PSA_CRYPTO is defined.

Depends on: #5176 - to have the keys available in struct mbedtls_ssl_transform.
Related: #5177
Follow-up: #5178

@mpg
Copy link
Contributor Author

mpg commented Feb 24, 2022

@superna9999 I initially assigned this to me, partially to save myself the trouble of writing a detailed description as I did for the other tasks, but it's now clear that you'll be able to start this way before I do, so here's a quick dump of what I had in mind, hoping it helps.

The current implementation of mbedtls_ct_hmac() already breaks the HMAC abstraction and re-implements part of HMAC: it uses mbedtls_md_xxx() functions, not mbedtls_md_hmac_xxx() functions (see the comments in the code). The PSA-based version is going to do that too: use psa_hash_xxx() functions instead of psa_mac_xxx()). It will actually go further, in that instead of picking up ikey and okey from the context, we're going to have to compute them ourselves from the key (see the RFC or the code of mbedtls_md_hmac_starts()).

That will require, unfortunately, making the key exportable when we are creating it in populate_tranform(). Then in mbedtls_ct_hmac() we can export the key, compute ikey/okey and do like the existing implementation.

Then there are generic differences between MD and PSA, not specific to what this function does: MD has a "context" that both holds the key and the state of the current operation, while in PSA those are distinct objects: the key (held in ssl_tranform, which we're going to export) and the psa_hash_operation_t, of which we'll have two, local to the function.

The signature of the function will have to change, to accept a PSA key as it's first argument instead of a mbedtls_md_context, so we'll probably end up with a declaration somewhat like this:

#if !defined(MBEDTLS_USE_PSA_CRYPTO)
int mbedtls_ct_hmac( mbedtls_md_context_t *ctx,
 /* other arguments */ );
#else  
int mbedtls_ct_hmac( mbedtls_svc_key_id_t key,
 /* other arguments */ );

(or something to that effect).

Final point, regarding testing: this function is unit-tested by ssl_cf_hmac() in test_suite_ssl. This test function depends on MBEDTLS_TEST_HOOKS, but also is only fully effective if MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN or MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND is used - see component_test_memsan_constant_flow() and component_test_valgrind_constant_flow() in tests/scripts/all.sh.

I hope this makes sense, if you have any question I'm still here tomorrow morning, after that I'll be on holiday for one week, but other team members can help.

@mpg mpg assigned superna9999 and unassigned mpg Feb 24, 2022
@superna9999
Copy link
Contributor

@mpg Thanks, for now I replaced the mbedtls_ct_hmac() with PSA MAC API calls to finish replacinc MD by PSA API, this will certainly help me finish this task !

@mpg
Copy link
Contributor Author

mpg commented Feb 24, 2022

Yes, replacing with a call to psa_mac_sign() (or rather the multi-part equivalent) is functionally correct and just doesn't have constant-time behaviour, which means it should allow all tests to pass except the two test_xxx_constant_flow() components of all.sh, which is a good intermediate step.

@mpg mpg closed this as completed in #5573 Mar 21, 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