Remove ecdh.c #582
Remove ecdh.c #582
Conversation
mpg
added
needs-preceding-pr
mpg
added
needs-review
|
Measurements for M55 in
Nearly 1K of savings, nice! |
| } | ||
|
|
||
| #if defined(MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED) | ||
| static psa_status_t ecdh_everest_shared_secret( |
There was a problem hiding this comment.
Should we still be checking for NULL buffers? I seem to remember we agreed we didn't need to have test coverage of them, however I can't remember if we agreed to allow the potential segfault or check it prior to writing?
There was a problem hiding this comment.
As a general rule, we don't check for NULL pointers passed as arguments, and are OK with segfault or whatever the platform is doing. (There are exceptions with legacy code, for example in PK, and we're not actively removing those checks, but new code doesn't have them.)
| { | ||
| if (key_buffer_size < MBEDTLS_X25519_KEY_SIZE_BYTES || | ||
| peer_key_length < MBEDTLS_X25519_KEY_SIZE_BYTES) { | ||
| return PSA_ERROR_INVALID_ARGUMENT; |
There was a problem hiding this comment.
Do we need to test these cases, in the standard test suite lcov indicates they are not covered with MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED.
There was a problem hiding this comment.
Ah good point, and thanks for running lcov as part of your review!
Taking a closer look:
- I don't think the first one is something that can happen: this function is only called in one place, behind a test that the curve is 25519. Also, the key buffer comes from the keystore. So, I think the only way
key_buffer_size < MBEDTLS_X25519_KEY_SIZE_BYTEScan be true here is if the keystore is inconsistent (attributes say the curve is 25519 but key buffer doesn't match). I don't think that's something that can happen, so we don't need a test case for it. However I'm going to remove the check and replace it with a comment explaining why we don't need it. - For the second one though,
peer_key_lengthcomes quite directly from user input, so it definitely seems possible and highly desirable to test with it being invalid. I'll add a test case for that.
There was a problem hiding this comment.
For the second one though,
peer_key_lengthcomes quite directly from user input, so it definitely seems possible and highly desirable to test with it being invalid. I'll add a test case for that.
It was so desirable to test that the test revealed an issue in interruptible ECDH: #624
bjwtaylor
left a comment
bjwtaylor
left a comment
There was a problem hiding this comment.
Just one question on the buffers, as I can't remember what we agreed
mpg
added
needs-preceding-pr
drivers/builtin/src/psa_crypto_ecp.c
Outdated
| /* This static function is only called when we know the curve is x25519, | ||
| * so we know key_buffer_size is correct unless the keystore is corrupted. | ||
| */ | ||
| (void) key_buffer_size; |
There was a problem hiding this comment.
@gilles-peskine-arm I'd like your opinion on this: is it correct to skip this check? Previously we were checking that key_buffer_size was large enough, but Ben pointed out that was not covered by tests. I reasoned that we can't hit this case unless the keystore has inconsistent information (a key whose attributes say it's x25519 but the key buffer is of the wrong size), which I think cannot happen, so went on to remove the check. But I'd like an extra pair or eyes on this change just to be sure.
There was a problem hiding this comment.
Check the key size in a driver against something that should have been enforced during key creation is a second line of defense. It's not absolutely mandatory, but it's better to have it. It's mostly due to persistent keys.
Suppose an older version of Mbed TLS of TF-PSA-Crypto had a bug that allowed some unexpected keys to be created. Then we fix that bug and we're happy. But users may have such unexpected in the key store. So we'd need to keep track of the former existence of that bug and make sure that we keep this particular check somewhere.
Another thing that can happen with persistent keys is if the storage is not cryptographically protected, and gets corrupted. It's not our job to defend against corrupted storage, but it's better if that can merely result in bad operation and possibly security holes (e.g. if a MAC key used for verification reads out as 0000) but not all the way to a buffer overwrite (though for keys I guess it would only be a buffer overread which is no worse than other things you could easily do with a corrupted key store).
There was a problem hiding this comment.
Ok, so I'll add the check back, with a comment explaining why we're not expecting it to be covered by tests, but we want to have it anyway.
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Using ECP directly is less complex and reduces dependencies. (This is similar to how FFDH uses BIGNUM directly rather than DHM_C.) This saves 1kB of code size on a config similar to TF-M medium except not using the p256-m driver. (That is, comment out MBEDTLS_PSA_P256M_DRIVER_ENABLED in configs/ext/tfm_mbedcrypto_config_profile_medium.h before running mtest -S). Before this commit: 54193 After this commit: 53117 Saved: 1076 Another benefit is that once ecdh.h is no longer part of the public interface, we can completely remove ecdh.c and ecdh.h, and with them a significant amount of complexity. Currently this breaks support for Everest in PSA, as this relied on the special dispatch implemented in ecdh.c. Support for Everest in PSA will later be restored in the proper way: by making in a PSA driver. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
This is a bit of a hack, but a smaller one than in ecdh.c, so it's still progress even if not fully satisfying. The proper way remains to turn Everest into an actual PSA driver. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Was the "high-level" part of the glue layer for ecdh.c Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
This was the "low-level" half of the glue layer for ecdh.c, now used by psa_crypto_ecp.c directly. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Also, check for exact length. Inequality is enough to avoid buffer overreads, but unless something's wrong we should have equality, so let's test for that - doesn't cost more anyway. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
4 participants
Description
Based on #578(merged). For a green CI, also needs:Remove some useless private includes mbedtls#10523(merged)Avoid references to ecdh.o mbedtls#10528(merged)Adjust macros used in generated config tests mbedtls-framework#254(merged)Depends on:
Fix "Simplify interruptible ECDH dispatch" #624 to start from a clean state.(merged)TODO: revisit Ben's comment about length checks.crypto: generate_config_checks.py: use current data #619 to avoid introducing a hack to pacify check-namesResolves Mbed-TLS/mbedtls#1962
Resolves Mbed-TLS/mbedtls#4440
Resolves Mbed-TLS/mbedtls#6048
Resolves Mbed-TLS/mbedtls#6049
Resolves Mbed-TLS/mbedtls#6050
Resolves Mbed-TLS/mbedtls#10346 (for development - still want Mbed-TLS/mbedtls#10347 for 3.6).
Follow-up (or prerequisite?): add (back) FF/ECDH benchmarking based on PSA Crypto - TODO: create issue!
PR checklist