Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.54k stars 2.6k forks source link

TLS failures in configurations with ECDH but not ECC key generation #9481

Open gilles-peskine-arm opened 3 months ago

gilles-peskine-arm commented 3 months ago

Since Mbed TLS 3.5.0, it is possible to configure the library with support for ECDH but not ECC key generation. It would be unusual, because generating an ECC key is very easy (the code size gain from disabling ECC generation is extremely small). Generating ECC requires randomness, but our own ECDH implementation also needs randomness anyway (for blinding), and TLS needs randomness anyway.

In TLS 1.2 code, that means static ECDH should be supported, but not ephemeral ECDH. (TLS 1.3 does not have static ECDH.)

In such a configuration, test_suite_ssl has many failures. I think there are at least two problems:

See https://github.com/Mbed-TLS/mbedtls/pull/9472/commits/40b932ee1348443f58b5aec77ba454a44c56d071 for how I discovered this and how to reproduce.

I haven't investigated the details of the failures. This looks like a lot of effort for a niche scenario, so I expect we aren't going to fix this unless there's demand from users.

mpg commented 2 weeks ago

I haven't investigated the details of the failures. This looks like a lot of effort for a niche scenario, so I expect we aren't going to fix this unless there's demand from users.

I fully agree about the scenario being pretty niche and the fix not being urgent unless we hear from actual users about this.

However I don't think it will be a lot of effort after we've removed static ECDH. After that I think the fix should be as simple as introducing a macro like MBEDTLS_SSL_HAVE_ECDH, that's defined when we have not only PSA_WANT_ALG_ECDH but also key generation, and then s/PSA_WANT_ALG_ECDH/MBEDTLS_SSL_HAVE_ECDH/ in all SSL files (except of course the definition of that new macro).

Do you think I'm missing something?

gilles-peskine-arm commented 2 weeks ago

However I don't think it will be a lot of effort after we've removed static ECDH.

Without static ECDH, I'm not even sure we have a bug outside of check_config.h. TLS requires ephemeral ECDH, which includes key generation.

The hard case is 3.6, where, if we want to fix the bug, we have to make the distinction between “can do static ECDH” and “can do ephemeral ECDH” in TLS code.

mpg commented 2 weeks ago

However I don't think it will be a lot of effort after we've removed static ECDH.

Without static ECDH, I'm not even sure we have a bug outside of check_config.h.

We do. Imagine a config where we have FFDH including key generation and ECDH excluding key generation (or the other way round). It would be overly strict for check_config.h to reject it, because in principle TLS 1.3 can to ephemeral key exchange that way - it should just negotiate the use of an FFDH group. However currently TLS 1.3 will happily negotiate use of a curve for the key exchange, and fail when it's time to generate a key for ECDH. That's because all the group negotiation code is using the wrong guards when deciding which groups to negotiate (only WANT_ALG when it should also require key generation, or in practice use a dedicated macro).

Of course that would be a very weird config, so again I'm really not arguing that this is an important bug. I'm just saying that in development I think it's pretty easy to fix. (Then again, considering the very low importance of the bug, that still might not be worth it.)

The hard case is 3.6, where, if we want to fix the bug, we have to make the distinction between “can do static ECDH” and “can do ephemeral ECDH” in TLS code.

Yes, that part would be a lot of work and IMO we shouldn't even think about it unless it turns out we really have to.