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 CBC Ciphers by default #13385

Open
pquerna opened this issue Nov 24, 2015 · 9 comments
Assignees
Labels
Milestone

Comments

@pquerna
Copy link

@pquerna pquerna commented Nov 24, 2015

Per @agl on Twitter, the only way to be safe with Go and TLS if you are worried about Lucky13-style attacks is to disable CBC mode ciphers:

https://twitter.com/agl__/status/669182140244824064

Currently none of the CBC ciphers are marked with suiteDefaultOff: https://github.com/golang/go/blob/master/src/crypto/tls/cipher_suites.go#L75-L95

TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
TLS_RSA_WITH_AES_128_CBC_SHA
TLS_RSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
TLS_RSA_WITH_3DES_EDE_CBC_SHA

It seems wrong for Go to have insecure defaults for a class of attacks that is becoming more well documented. If there is no willingness to implement countermeasures, shouldn't the vulnerable class of ciphers be disabled by default?

@pquerna pquerna changed the title Disable CBC Ciphers for TLS by default crypto/tls: Disable CBC Ciphers by default Nov 24, 2015
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Nov 24, 2015
@timcooijmans

This comment has been minimized.

Copy link
Contributor

@timcooijmans timcooijmans commented Nov 24, 2015

Disabling CBC so will break compatibility with many operating systems and devices. In practice all public-facing Go-services would have to re-enable the CBC-ciphersuites. So the question is wether implementing countermeasures isn't the only viable solution.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Nov 24, 2015

Perhaps it just needs to be easier to change the global, default ciphersuites without re-wiring up a *tls.Config, *http.Server, *http.Transport, etc.

@pquerna

This comment has been minimized.

Copy link
Author

@pquerna pquerna commented Nov 24, 2015

@bradfitz such a method would be a good start; http2 is already has a blacklist of ciphers, making a way to more easily control that behavior in the various invocations of cipher configurations / global default would be a great start.

@DemiMarie

This comment has been minimized.

Copy link

@DemiMarie DemiMarie commented Dec 6, 2015

If disabling CBC is not an option, Go needs to implement the appropriate countermeasures in its cryptography (or call out to a third-party library that does so).

Also AEAD ciphers should automatically be preferred when possible.

@timcooijmans

This comment has been minimized.

Copy link
Contributor

@timcooijmans timcooijmans commented Dec 8, 2015

I agree, as long as CBC is needed to support all commonly used browsers and libraries, fixing CBC is the only solution, no matter how ugly and complex it may be. While disabling CBC ensures a safe-default. The fact that in practice any public service needs to re-enable it, still leads to unsafe situations that could lead to breaches and negative publicity for Go.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 8, 2015

CL https://golang.org/cl/17532 mentions this issue.

bradfitz added a commit that referenced this issue Dec 14, 2015
Updates #13385

Change-Id: I9c2edf8c02adc388c48760b29e63dfa2966262d6
Reviewed-on: https://go-review.googlesource.com/17532
Reviewed-by: Tim Cooijmans <timcooijmans@gmail.com>
Reviewed-by: Adam Langley <agl@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 23, 2015

CL https://golang.org/cl/18130 mentions this issue.

@bradfitz bradfitz added the Security label May 19, 2016
gopherbot pushed a commit that referenced this issue Oct 4, 2016
The aim is to make the decrypt() timing profile constant, irrespective of
the CBC padding length or correctness.  The old algorithm, on valid padding,
would only MAC bytes up to the padding length threshold, making CBC
ciphersuites vulnerable to plaintext recovery attacks as presented in the
"Lucky Thirteen" paper.

The new algorithm Write()s to the MAC all supposed payload, performs a
constant time Sum()---which required implementing a constant time Sum() in
crypto/sha1, see the "Lucky Microseconds" paper---and then Write()s the rest
of the data. This is performed whether the padding is good or not.

This should have no explicit secret-dependent timings, but it does NOT
attempt to normalize memory accesses to prevent cache timing leaks.

Updates #13385

Change-Id: I15d91dc3cc6eefc1d44f317f72ff8feb0a9888f7
Reviewed-on: https://go-review.googlesource.com/18130
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 29, 2016

CL https://golang.org/cl/33665 mentions this issue.

gopherbot pushed a commit that referenced this issue Nov 29, 2016
Some countermeasures were implemented in https://golang.org/cl/18130

Updates #13385

Change-Id: I723e1e3be0fa6d13767b65b145d90c89e92b2774
Reviewed-on: https://go-review.googlesource.com/33665
Reviewed-by: Adam Langley <agl@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 17, 2017

CL https://golang.org/cl/35290 mentions this issue.

gopherbot pushed a commit that referenced this issue Jan 17, 2017
As is, they were fully vulnerable to the Lucky13 attack. The SHA1
variants implement limited countermeasures (see f28cf83) but the
SHA256 ones are apparently used rarely enough (see 8741504) that
it's not worth the extra code.

Instead, disable them by default and update the warning.

Updates #13385
Updates #15487

Change-Id: I45b8b716001e2fa0811b17e25be76e2512e5abb2
Reviewed-on: https://go-review.googlesource.com/35290
Reviewed-by: Adam Langley <alangley@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Matt Layher <mdlayher@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@harshavardhana harshavardhana referenced this issue Nov 28, 2017
3 of 9 tasks complete
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Updates golang#13385

Change-Id: I9c2edf8c02adc388c48760b29e63dfa2966262d6
Reviewed-on: https://go-review.googlesource.com/17532
Reviewed-by: Tim Cooijmans <timcooijmans@gmail.com>
Reviewed-by: Adam Langley <agl@golang.org>
FiloSottile added a commit to FiloSottile/go that referenced this issue Oct 12, 2018
The aim is to make the decrypt() timing profile constant, irrespective of
the CBC padding length or correctness.  The old algorithm, on valid padding,
would only MAC bytes up to the padding length threshold, making CBC
ciphersuites vulnerable to plaintext recovery attacks as presented in the
"Lucky Thirteen" paper.

The new algorithm Write()s to the MAC all supposed payload, performs a
constant time Sum()---which required implementing a constant time Sum() in
crypto/sha1, see the "Lucky Microseconds" paper---and then Write()s the rest
of the data. This is performed whether the padding is good or not.

This should have no explicit secret-dependent timings, but it does NOT
attempt to normalize memory accesses to prevent cache timing leaks.

Updates golang#13385

Change-Id: I15d91dc3cc6eefc1d44f317f72ff8feb0a9888f7
Reviewed-on: https://go-review.googlesource.com/18130
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Some countermeasures were implemented in https://golang.org/cl/18130

Updates golang#13385

Change-Id: I723e1e3be0fa6d13767b65b145d90c89e92b2774
Reviewed-on: https://go-review.googlesource.com/33665
Reviewed-by: Adam Langley <agl@golang.org>
FiloSottile added a commit to FiloSottile/go that referenced this issue Oct 12, 2018
As is, they were fully vulnerable to the Lucky13 attack. The SHA1
variants implement limited countermeasures (see f28cf83) but the
SHA256 ones are apparently used rarely enough (see 8741504) that
it's not worth the extra code.

Instead, disable them by default and update the warning.

Updates golang#13385
Updates golang#15487

Change-Id: I45b8b716001e2fa0811b17e25be76e2512e5abb2
Reviewed-on: https://go-review.googlesource.com/35290
Reviewed-by: Adam Langley <alangley@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Matt Layher <mdlayher@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Updates golang#13385

Change-Id: I9c2edf8c02adc388c48760b29e63dfa2966262d6
Reviewed-on: https://go-review.googlesource.com/17532
Reviewed-by: Tim Cooijmans <timcooijmans@gmail.com>
Reviewed-by: Adam Langley <agl@golang.org>
FiloSottile added a commit to FiloSottile/go that referenced this issue Oct 12, 2018
The aim is to make the decrypt() timing profile constant, irrespective of
the CBC padding length or correctness.  The old algorithm, on valid padding,
would only MAC bytes up to the padding length threshold, making CBC
ciphersuites vulnerable to plaintext recovery attacks as presented in the
"Lucky Thirteen" paper.

The new algorithm Write()s to the MAC all supposed payload, performs a
constant time Sum()---which required implementing a constant time Sum() in
crypto/sha1, see the "Lucky Microseconds" paper---and then Write()s the rest
of the data. This is performed whether the padding is good or not.

This should have no explicit secret-dependent timings, but it does NOT
attempt to normalize memory accesses to prevent cache timing leaks.

Updates golang#13385

Change-Id: I15d91dc3cc6eefc1d44f317f72ff8feb0a9888f7
Reviewed-on: https://go-review.googlesource.com/18130
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Some countermeasures were implemented in https://golang.org/cl/18130

Updates golang#13385

Change-Id: I723e1e3be0fa6d13767b65b145d90c89e92b2774
Reviewed-on: https://go-review.googlesource.com/33665
Reviewed-by: Adam Langley <agl@golang.org>
FiloSottile added a commit to FiloSottile/go that referenced this issue Oct 12, 2018
As is, they were fully vulnerable to the Lucky13 attack. The SHA1
variants implement limited countermeasures (see f28cf83) but the
SHA256 ones are apparently used rarely enough (see 8741504) that
it's not worth the extra code.

Instead, disable them by default and update the warning.

Updates golang#13385
Updates golang#15487

Change-Id: I45b8b716001e2fa0811b17e25be76e2512e5abb2
Reviewed-on: https://go-review.googlesource.com/35290
Reviewed-by: Adam Langley <alangley@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Matt Layher <mdlayher@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.