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

crypto/tls: disable RSA key exchange cipher suites by default #63413

Closed
FiloSottile opened this issue Oct 6, 2023 · 7 comments
Closed

crypto/tls: disable RSA key exchange cipher suites by default #63413

FiloSottile opened this issue Oct 6, 2023 · 7 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@FiloSottile
Copy link
Contributor

FiloSottile commented Oct 6, 2023

The "RSA key exchange" cipher suites (not to be confused with ECDHE cipher suites that use RSA certificates), or non-ECDHE cipher suites, work dramatically differently from the ECDHE ones: instead of using the certificate to sign an ephemeral key exchange, they encrypt the session key with the certificate's public key.

This has two major downsides:

  1. there is no forward secrecy, as if at any point the certificate key becomes compromised, all previous and future connections can be passively decrypted;
  2. this forces the server to decrypt attacker-provided PKCS#1 v1.5 ciphertexts with the certificate private key, which needs to be done in perfectly constant time to avoid Bleichenbacher '98 padding oracles attacks, which we have the extremely dangerous rsa.DecryptPKCS1v15SessionKey API for, but which the Marvin attack suggests we might still not be doing correctly.

Thanks to automatic cipher suite ordering, we don't necessarily have to disable less secure cipher suites, confident that they will be only picked as a last resort. However, even as a last resort (1) and (2) are problematic: (1) delivers a meaningfully different security property for some connections, in a way that is mostly obscure to applications and not opt-in; (2) means that even just supporting the cipher suite makes it possible for attackers to mount potential attacks to retroactively decrypt connections, or even forge signatures.

Concretely, I propose we move the following cipher suites to disabled by default in Go 1.22 behind a godebug.

TLS_RSA_WITH_AES_128_GCM_SHA256,
TLS_RSA_WITH_AES_256_GCM_SHA384,
TLS_RSA_WITH_AES_128_CBC_SHA,
TLS_RSA_WITH_AES_256_CBC_SHA,
TLS_RSA_WITH_3DES_EDE_CBC_SHA

Unfortunately, we have even less data than #62459, because Cloudflare does not publish cipher suite stats.

In theory, we could prioritize disabling the server side, which is affected by the Marvin attack, but we are generally more aggressive in modernizing the client anyway (because clients can have a stronger expectation of competence from servers than the other way around), so might as well keep it simple and disable them on both sides.

/cc @rolandshoemaker @golang/security

@FiloSottile FiloSottile added Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Oct 6, 2023
@gopherbot gopherbot added this to the Proposal milestone Oct 6, 2023
@rolandshoemaker
Copy link
Member

Google sees an extremely negligible, but non-zero, amount of traffic for RSA based kex's. Disabling these seems like a general security win for the vast majority of users.

@FiloSottile
Copy link
Contributor Author

According to the SSLLabs recorded client handshakes, these are the clients that don't support ECDHE.

Android 2.3.7
BingPreview Dec 2013
BingPreview Jun 2014
Firefox 21 Fedora 19
IE 6 XP
IE 8 XP
Java 6u45
OpenSSL 0.9.8y
Opera 12.15 Win 7
YandexBot 3.0
YandexBot May 2014

They are all ancient and unsupported, so I think we can go ahead in Go 1.22 (with a godebug).

These are also a strict subset of the clients that don't support TLS 1.2, so we could avoid doing two breaking changes and bundle this with #62459, if we feel like that can ship in Go 1.22, too.

@rsc
Copy link
Contributor

rsc commented Nov 1, 2023

So GODEBUG=tlsrsakex=1 would re-enable, and GODEBUG=tlsrsakex=0 would be the new default in some future Go version.

@rsc rsc changed the title proposal: crypto/tls: disable legacy non-ECDHE cipher suites by default proposal: crypto/tls: disable RSA key exchange cipher suites by default Nov 2, 2023
@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Alongside #62459, Go 1.22 would also introduce a new default GODEBUG=tlsrsakex=0 that disables these RSA-only (non-ECDHE) cipher suites:

TLS_RSA_WITH_AES_128_GCM_SHA256,
TLS_RSA_WITH_AES_256_GCM_SHA384,
TLS_RSA_WITH_AES_128_CBC_SHA,
TLS_RSA_WITH_AES_256_CBC_SHA,
TLS_RSA_WITH_3DES_EDE_CBC_SHA

As usual, the new GODEBUG would only be set in go 1.22 programs. Setting GODEBUG=tlsrsakex=1 would re-enable these suites.

@rsc
Copy link
Contributor

rsc commented Nov 10, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Alongside #62459, Go 1.22 would also introduce a new default GODEBUG=tlsrsakex=0 that disables these RSA-only (non-ECDHE) cipher suites:

TLS_RSA_WITH_AES_128_GCM_SHA256,
TLS_RSA_WITH_AES_256_GCM_SHA384,
TLS_RSA_WITH_AES_128_CBC_SHA,
TLS_RSA_WITH_AES_256_CBC_SHA,
TLS_RSA_WITH_3DES_EDE_CBC_SHA

As usual, the new GODEBUG would only be set in go 1.22 programs. Setting GODEBUG=tlsrsakex=1 would re-enable these suites.

@rsc rsc changed the title proposal: crypto/tls: disable RSA key exchange cipher suites by default crypto/tls: disable RSA key exchange cipher suites by default Nov 10, 2023
@rsc rsc modified the milestones: Proposal, Backlog Nov 10, 2023
@gopherbot
Copy link

Change https://go.dev/cl/541517 mentions this issue: crypto/tls: remove RSA KEX ciphers from the default list

@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Nov 14, 2023
@gopherbot
Copy link

Change https://go.dev/cl/544336 mentions this issue: crypto/tls: mark RSA KEX cipher suites insecure

gopherbot pushed a commit that referenced this issue Nov 21, 2023
Updates #63413

Change-Id: I31fc2f9728582524cac5d101d0011093dbd05ed3
Reviewed-on: https://go-review.googlesource.com/c/go/+/544336
Auto-Submit: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

5 participants