TLS record HMAC 2a: variable-time computations

[mpg]

Perform all variable-time HMAC computations related to TLS record protection using psa_mac instead of mbedtls_md_hmac, using the key slots referenced in the transform (see #5176), when MBEDTLS_USE_PSA_CRYPTO is defined.

• In mbedtls_ssl_encrypt_buf() when mode == MBEDTLS_MODE_STREAM or CBC without encrypt-then-mac → use psa_mac_sign APIs.
• In mbedtls_ssl_encrypt_buf() when mode is CBC and encrypt-then-MAC is active → use psa_mac_sign APIs.
• In mbedtls_ssl_decrypt_buf() when mode is CBC and encrypt-then-MAC is active → use psa_mac_verify APIs; remove the mac_expect buffer and associated debug messages.

Note: each computation will need its own temporary psa_mac_operation_t object. Previously the md_ctx_xxx from the transform serve both for key storage and multi-part context; now only the key is referenced in the transform, and the multi-part context is local to the function doing the computation.

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

[superna9999]

@mpg I have an issue with psa_mac_verify_finish with the test_suite_ssl and short tag, EtM tests.

The results of short tag, EtM tests is that the verify length passed to psa_mac_verify_finish is fixed to 10 instead of the mac key length.

Using the psa_mac_sign_setup/psa_mac_sign_finish only for this case passes the test_suite_ssl tests.

I can't find anything in the PSA API about the mac_length limitation whatoever, but the PSA code explicitly checks:

if( operation->mac_size != mac_length )                                                         
{                                                                                               
         status = PSA_ERROR_INVALID_SIGNATURE;                                                       
         goto exit;                                                                                  
 }

[gilles-peskine-arm]

@superna9999 I'm not sure what the problem is here, but do note that if you're using a truncated MAC algorithm, you have to pass something like PSA_ALG_TRUNCATED_MAC(PSA_ALG_HMAC(PSA_ALG_SHA_256), 10) to psa_mac_{sign,verify}{,_setup}, and you have to use PSA_ALG_TRUNCATED_MAC or PSA_ALG_AT_LEAST_THIS_LENGTH_MAC for the key policy.

[mpg]

Hmm, support for truncated HMAC was removed in Mbed TLS 3.0, so these tests might be obsolete. I'll check tomorrow and confirm, but my initial impression is we should remove those tests rather than jump through hoops to make them pass.

[superna9999]

@gilles-peskine-arm thanks, I added support for TRUNCATED_MAC in the test_suite_ssl build_transforms() mbedtls_ssl_transform setup code and all the tests passes right now.

@mpg the impact of using truncated HMAC is really reduced and only impacts code in test_suite_ssl, the risk to keep it seems low

[mpg]

Ok, I checked, and on second thought... I still think those tests are obsolete and should be removed. Specifically, all tests that are using tag_mode == 1 with a CBC or NULL cipher are obsolete. I think one simple way to identify all those tests is (starting from a state where everything passes), to modify build_transforms() and remove the case 1: block from the case MBEDTLS_MODE_CBC: part (which also covers MBEDTLS_MODE_STREAM which in the context of TLS 1.2 is equivalent to NULL these days). Then all tests that start failing after than change are obsolete and can be removed.

[superna9999]

@mpg remove or disable with PSA_CRYPTO ?

[mpg]

Oh, we've been posting at the same time.

@mpg the impact of using truncated HMAC is really reduced and only impacts code in test_suite_ssl, the risk to keep it seems low

Sure, but these tests are covering are about a feature that's no longer present in the library. When preparing Mbed TLS 3.0, we took a number of shortcuts, in particular when removing features we didn't go and check which tests could be removed, which could be simplified, etc. That's the only reason those tests are still here.

So, remove entirely. And that's also why I don't think it's worth spending even the tiniest amount of code, even if it's only in test_suite_ssl to support that. These tests should already be gone, in an ideal world they would have been removed before you started this task - but we don't live in that world so the second best thing is to remove them now.

@mpg remove or disable with PSA_CRYPTO ?

In general we should really avoid disabling tests with USE_PSA_CRYTO, unless we have an equivalent test specific to USE_PSA_CRYPTO (for example, some functions return a different status in some error cases depending on whether USE_PSA_CRYPTO is enabled, and then we have two test cases, one depending on !USE_PSA_CRYPTO and the other on USE_PSA_CRYPTO). In the future USE_PSA_CRYPTO will become the default, then the only option, so it really needs to have test coverage that's at least as good as !USE_PSA_CRYPTO.

[gilles-peskine-arm]

@mpg Why are these tests passing if they're testing something that's been removed? Are they testing an internal function that's no longer used with the given parameters?

[mpg]

Because they're building the ssl_transform object themselves instead of using the populate_transform() function from the library. That's also why Neil could get the tests to pass by making changes only in test_suite_ssl.function, not in ssl_tls.c.