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

Remove support for the RSA-decryption key exchange in TLS 1.2 #8170

Closed
daverodgman opened this issue Sep 7, 2023 · 12 comments
Closed

Remove support for the RSA-decryption key exchange in TLS 1.2 #8170

daverodgman opened this issue Sep 7, 2023 · 12 comments
Labels
api-break This issue/PR breaks the API and must wait for a new major version component-tls size-m Estimated task size: medium (~1w)

Comments

@daverodgman
Copy link
Contributor

daverodgman commented Sep 7, 2023

Task: evaluate how important retaining RSA key exchange is - are people using this? Are there protocols that require it? - and what the impact of removing it is - does it reduce maintenance (e.g., simplify the codebase, remove potentially-vulnerable bits of code, etc) or is it limited benefit?

As per #6792 (comment)

This is RSA and RSA-PSK, as opposed to ECDHE-RSA (or currently DHE-RSA). Using forward-secure key exchanges has been officially recommended since at least 2015 (RFC 7525 again). [Edit: see this comment before extrapolating.]

Edit: now there's even a WG draft to formally deprecate it, as in "MUST NOT negotiate".

Also, this key exchange requires care to avoid timing attacks (see all the timing-based variants of Bleichenbacher), and would also cause trouble (perhaps even more than CBC) when implementing isolation of session secrets with PSA.

(OTOH, the mandatory-to-implement ciphersuite for TLS 1.2 is TLS_RSA_WITH_AES_128_CBC_SHA, which uses this key exchange, but RFC 7525 (which has official Best Current Practices status) encourages implementors to ignore that.)

Mailing list thread: https://lists.trustedfirmware.org/archives/list/[email protected]/thread/LGLLENH54LTP64SUNIKLHJ5GFQMQAVHL/

Follow-up: #8459

@dimakuv
Copy link

dimakuv commented Jun 17, 2024

Hello!

The Gramine project currently uses MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256 ciphersuite. Note that we use Pre-Shared Key (PSK) TLS sessions, because the underlying Intel SGX technology that Gramine uses has HW-based means to establish a shared key.

As per the recommendations, we would like to move to a PSK-based ciphersuite that has Elliptic Curve DHE (ECDHE) for key exchange.

Unfortunately, I do not observe any ciphersuite that would have the following characteristics:

  1. PSK
  2. ECDHE
  3. AES-GCM

This is the list of PSK-ECDHE-based ciphersuites that seem to be supported in mbedTLS v3.6.0: https://github.com/Mbed-TLS/mbedtls/blob/v3.6.0/include/mbedtls/ssl_ciphersuites.h#L128-L134

Duplicating this list here:

#define MBEDTLS_TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA       0xC035
#define MBEDTLS_TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA       0xC036
#define MBEDTLS_TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA256    0xC037
#define MBEDTLS_TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA384    0xC038
#define MBEDTLS_TLS_ECDHE_PSK_WITH_NULL_SHA              0xC039
#define MBEDTLS_TLS_ECDHE_PSK_WITH_NULL_SHA256           0xC03A
#define MBEDTLS_TLS_ECDHE_PSK_WITH_NULL_SHA384           0xC03B

When I try to use e.g. MBEDTLS_TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA256, I observe significant performance drop in Gramine workloads (up to 2x runtime overhead). I attribute this performance overhead to the CBC mode used.

Are there plans to introduce PSK + ECDHE + AES-GCM ciphersuites in mbedTLS?

P.S. I am no crypto expert, so maybe my understanding of some details is wrong. Please correct me if I wrote something blatantly wrong.

@dimakuv
Copy link

dimakuv commented Jun 27, 2024

Quick update from a crypto expert:

I think the best performance you could hope for with forward secrecy would be ECDHE with AES_128_GCM, but this combination doesn't seem to be supported by mbedTLS. Maybe you could ask for it to be added? Elliptic curve diffie hellman gets the same security level with smaller field elements, so it saves on communication/computation costs versus DHE, and AES-GCM benefits from hardware acceleration and provides built-in ciphertext integrity, saving the cost of separately computing MACs on messages compared to AES-CBC (mentioned in the alternative proposal). Even if mbedTLS adds ECDHE with AES-GCM, there will still be a performance hit.

(taken from gramineproject/gramine#1918 (comment))

Another update on Gramine's case:

But I don't think we gain anything from forward secrecy? The symmetric keys we use are ephemeral anyways, and exchanged via hardware means. As far as I understand we just got forced into it, because mbedtls is dropping all the combinations without forward secrecy, right? So, if we want to request anything from them, then it would be to preserve some non-forward-secrecy PSK combination.

(taken from gramineproject/gramine#1918 (comment))

@gilles-peskine-arm
Copy link
Contributor

ECDHE with AES_128_GCM, but this combination doesn't seem to be supported by mbedTLS. Maybe you could ask for it to be added?

We do support cipher suites with ECDHE and AES-128-GCM. Ah, looking at the context, this is about PSK cipher suites with a key exchange. Hmmm, strange, we don't support any ECDHE-PSK-CCM or ECDHE-PSK-GCM or RSA-PSK-CCM cipher suite, but we do support RSA-PSK-GCM. I don't think there's a deep reason for it, I suspect a historical reason that CCM, PSK and ECC were added to TLS 1.0/1.2 specifications in different strands and not all combinations have made their way into Mbed TLS. This is in our backlog: #1729

@mpg
Copy link
Contributor

mpg commented Jun 28, 2024

For reference, I'd like to avoid a possible misunderstanding: this issue is specifically about the RSA and RSA-PSK key exchange, and should not be interpreted as an indication that we are dropping all key exchanges that are not offering forward secrecy. RSA and RSA-PSK have a number of issues on top of that, and it's the sum of those reasons that led to them being on their way to official deprecation and planned for removal from mbed TLS.

In particular, I don't see the (pure) PSK key exchange being removed any time in the foreseeable future based on its lack of forward secrecy: this is the only key exchange that doesn't use asymmetric crypto, which in some use cases is prohibitively expensive. So, it offers a valuable trade-off: performance & footprint vs forward secrecy - as opposed to key exchanges like RSA, RSA-PSK, static (EC)DH which incur all the costs of asymmetric crypto without the benefit of forward secrecy.

It should be noted that TLS 1.3, which removed a lot of old & unwanted stuff, did absolutely keep PSK (while merging it with session resumption, which if anything makes it much more prominent), and that we do implement the (non-ephemeral) PSK key exchange mode in Mbed TLS.

@dimakuv
Copy link

dimakuv commented Jun 28, 2024

[...] this issue is specifically about the RSA and RSA-PSK key exchange, and should not be interpreted as an indication that we are dropping all key exchanges that are not offering forward secrecy

@mpg Thank you for this great and concise summary! This was exactly my misunderstanding, and I apologize for understanding the original posting incorrectly.

Thanks again for clearing this up. Please then ignore my comments about the Gramine project's needs. Gramine will continue using MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256.

@mpg
Copy link
Contributor

mpg commented Jun 28, 2024

No worries! Reading the issue description again, I could see how it could give the wrong impression, so I wanted to clarify that to avoid causing trouble to other people as well.

And thanks for reaching out!

@mpg
Copy link
Contributor

mpg commented Aug 6, 2024

Note: we have tests in ssl-opt.sh that use plain RSA key exchange to test other things, for example, handling of the keyUsage extension in certificates. To my knowledge, RSA was the only key exchange using the KeyEncipherment bit, so positive tests about this bit will probably just be removed without a replacement.

(If we also remove static ECDH suites as we're planning to then all cert-based key exchanges will be needing the digitalSignature bit, so we can only have positive testing of that bit and negative testing of its absence - so TLS 1.2 will become aligned with TLS 1.3 on this point. I'm talking about testing above, but probably the library code about keyUsage in TLS can be simplified as well.)

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Aug 7, 2024

Architectural decision: we are removing RSA key exchanges using RSA decryption from Mbed TLS 4.0, i.e. MBEDTLS_KEY_EXCHANGE_RSA and MBEDTLS_KEY_EXCHANGE_RSA_PSK. (To be clear, key exchanges using RSA signature are staying.)

Note: it's likely that some tests are currently using RSA decryption to test some other feature, because RSA decryption used to be the default thing. Those tests should not be removed, but adapted to use a different cipher suite.

@gilles-peskine-arm gilles-peskine-arm changed the title Study: Consider removing support for the RSA key exchange in TLS 1.2 Remove support for the RSA key exchange in TLS 1.2 Aug 14, 2024
@gilles-peskine-arm gilles-peskine-arm changed the title Remove support for the RSA key exchange in TLS 1.2 Remove support for the RSA-decryption key exchange in TLS 1.2 Aug 14, 2024
@yanesca yanesca moved this to 4.0 - Prepare High Level Crypto in Mbed TLS Backlog Aug 27, 2024
@github-project-automation github-project-automation bot moved this to Mbed TLS 4.0 SHOULD in Backlog for Mbed TLS Aug 30, 2024
@mschulz-at-hilscher
Copy link
Contributor

We still need TLS_RSA for OPCUA: https://reference.opcfoundation.org/Core/Part7/v104/docs/6.6.159

@mschulz-at-hilscher
Copy link
Contributor

On the other hand, they are moving forward and are planning to use more secure ciphers in the future:
https://mantis.opcfoundation.org/view.php?id=9814

@mpg
Copy link
Contributor

mpg commented Sep 5, 2024

As mentioned above, there is a TLS working group draft that, when it becomes an RFC, will formally deprecate both of the ciphersuites that are currently mandatory in OPCUA:

Clients MUST NOT offer and servers MUST NOT select FFDHE cipher suites in TLS 1.2 connections

Clients MUST NOT offer and servers MUST NOT select RSA cipher suites in TLS 1.2 connections.

For the avoidance of doubt, the ciphersuites TLS_DHE_RSA_WITH_AES_256_CBC_SHA256 and TLS_RSA_WITH_AES_256_CBC_SHA256 are listed explicitly in appendices C and D respectively.

The draft has a list of reasons why these key exchanges are being deprecated, if it helps justifying the update in OPCUA.

@gilles-peskine-arm gilles-peskine-arm closed this as not planned Won't fix, can't repro, duplicate, stale Nov 4, 2024
@github-project-automation github-project-automation bot moved this from Design needed to Done in Mbed TLS 4.0 planning Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break This issue/PR breaks the API and must wait for a new major version component-tls size-m Estimated task size: medium (~1w)
Projects
Status: Mbed TLS 4.0 SHOULD
Status: Done
Status: No status
Development

No branches or pull requests

6 participants