[go] crypto/tls: set default minimum client version to TLS 1.2
139 views
Skip to first unread message
Filippo Valsorda (Gerrit)
Oct 31, 2021, 11:14:31 PM10/31/21
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Filippo Valsorda would like Roland Shoemaker to review this change.
crypto/tls: set default minimum client version to TLS 1.2
Updates #45428
Change-Id: I5d70066d4091196ec6f8bfc2edf3d78fdc0520c1
---
M src/crypto/tls/common.go
M src/crypto/tls/handshake_server.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_test.go
M src/crypto/tls/handshake_server_test.go
M src/crypto/tls/handshake_server_tls13.go
6 files changed, 70 insertions(+), 17 deletions(-)
diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 610a516..b375391 100644
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -20,6 +20,7 @@
"fmt"
"io"
"net"
+ "os"
"strings"
"sync"
"time"
@@ -682,11 +683,20 @@
ClientSessionCache ClientSessionCache
// MinVersion contains the minimum TLS version that is acceptable.
- // If zero, TLS 1.0 is currently taken as the minimum.
+ //
+ // By default, TLS 1.2 is currently used as the minimum when acting as a
+ // client, and TLS 1.0 when acting as a server. TLS 1.0 is the minimum
+ // supported by this package, both as a client and as a server.
+ //
+ // The client-side default can temporarily be reverted to TLS 1.0 by
+ // including the value "x509sha1=1" in the GODEBUG environment variable.
+ // Note that this option will be removed in Go 1.19 (but it will still be
+ // possible to set this field to VersionTLS10 explicitly).
MinVersion uint16
// MaxVersion contains the maximum TLS version that is acceptable.
- // If zero, the maximum version supported by this package is used,
+ //
+ // By default, the maximum version supported by this package is used,
// which is currently TLS 1.3.
MaxVersion uint16
@@ -964,9 +974,21 @@
VersionTLS10,
}
-func (c *Config) supportedVersions() []uint16 {
+// debugEnableTLS10 enables TLS 1.0. See issue 45428.
+var debugEnableTLS10 = strings.Contains(os.Getenv("GODEBUG"), "tls10default=1")
+
+const (
+ roleClient = true
+ roleServer = false
+)
+
+func (c *Config) supportedVersions(role bool) []uint16 {
versions := make([]uint16, 0, len(supportedVersions))
for _, v := range supportedVersions {
+ if (c == nil || c.MinVersion == 0) && !debugEnableTLS10 &&
+ role == roleClient && v < VersionTLS12 {
+ continue
+ }
if c != nil && c.MinVersion != 0 && v < c.MinVersion {
continue
}
@@ -978,8 +1000,8 @@
return versions
}
-func (c *Config) maxSupportedVersion() uint16 {
- supportedVersions := c.supportedVersions()
+func (c *Config) maxSupportedVersion(role bool) uint16 {
+ supportedVersions := c.supportedVersions(role)
if len(supportedVersions) == 0 {
return 0
}
@@ -1020,8 +1042,8 @@
// mutualVersion returns the protocol version to use given the advertised
// versions of the peer. Priority is given to the peer preference order.
-func (c *Config) mutualVersion(peerVersions []uint16) (uint16, bool) {
- supportedVersions := c.supportedVersions()
+func (c *Config) mutualVersion(role bool, peerVersions []uint16) (uint16, bool) {
+ supportedVersions := c.supportedVersions(role)
for _, peerVersion := range peerVersions {
for _, v := range supportedVersions {
if v == peerVersion {
@@ -1100,7 +1122,7 @@
if config == nil {
config = &Config{}
}
- vers, ok := config.mutualVersion(chi.SupportedVersions)
+ vers, ok := config.mutualVersion(roleServer, chi.SupportedVersions)
if !ok {
return errors.New("no mutually supported protocol versions")
}
diff --git a/src/crypto/tls/handshake_client.go b/src/crypto/tls/handshake_client.go
index 4af3d99..2ae6f3f 100644
--- a/src/crypto/tls/handshake_client.go
+++ b/src/crypto/tls/handshake_client.go
@@ -52,12 +52,12 @@
return nil, nil, errors.New("tls: NextProtos values too large")
}
- supportedVersions := config.supportedVersions()
+ supportedVersions := config.supportedVersions(roleClient)
if len(supportedVersions) == 0 {
return nil, nil, errors.New("tls: no supported versions satisfy MinVersion and MaxVersion")
}
- clientHelloVersion := config.maxSupportedVersion()
+ clientHelloVersion := config.maxSupportedVersion(roleClient)
// The version at the beginning of the ClientHello was capped at TLS 1.2
// for compatibility reasons. The supported_versions extension is used
// to negotiate versions now. See RFC 8446, Section 4.2.1.
@@ -194,7 +194,7 @@
// If we are negotiating a protocol version that's lower than what we
// support, check for the server downgrade canaries.
// See RFC 8446, Section 4.1.3.
- maxVers := c.config.maxSupportedVersion()
+ maxVers := c.config.maxSupportedVersion(roleClient)
tls12Downgrade := string(serverHello.random[24:]) == downgradeCanaryTLS12
tls11Downgrade := string(serverHello.random[24:]) == downgradeCanaryTLS11
if maxVers == VersionTLS13 && c.vers <= VersionTLS12 && (tls12Downgrade || tls11Downgrade) ||
@@ -362,7 +362,7 @@
peerVersion = serverHello.supportedVersion
}
- vers, ok := c.config.mutualVersion([]uint16{peerVersion})
+ vers, ok := c.config.mutualVersion(roleClient, []uint16{peerVersion})
if !ok {
c.sendAlert(alertProtocolVersion)
return fmt.Errorf("tls: server selected unsupported protocol version %x", peerVersion)
diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go
index 43f30e2..5cb1527 100644
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -156,7 +156,7 @@
if len(clientHello.supportedVersions) == 0 {
clientVersions = supportedVersionsFromMax(clientHello.vers)
}
- c.vers, ok = c.config.mutualVersion(clientVersions)
+ c.vers, ok = c.config.mutualVersion(roleServer, clientVersions)
if !ok {
c.sendAlert(alertProtocolVersion)
return nil, fmt.Errorf("tls: client offered only unsupported versions: %x", clientVersions)
@@ -191,7 +191,7 @@
hs.hello.random = make([]byte, 32)
serverRandom := hs.hello.random
// Downgrade protection canaries. See RFC 8446, Section 4.1.3.
- maxVers := c.config.maxSupportedVersion()
+ maxVers := c.config.maxSupportedVersion(roleServer)
if maxVers >= VersionTLS12 && c.vers < maxVers || testingOnlyForceDowngradeCanary {
if c.vers == VersionTLS12 {
copy(serverRandom[24:], downgradeCanaryTLS12)
@@ -354,7 +354,7 @@
for _, id := range hs.clientHello.cipherSuites {
if id == TLS_FALLBACK_SCSV {
// The client is doing a fallback connection. See RFC 7507.
- if hs.clientHello.vers < c.config.maxSupportedVersion() {
+ if hs.clientHello.vers < c.config.maxSupportedVersion(roleServer) {
c.sendAlert(alertInappropriateFallback)
return errors.New("tls: client using inappropriate protocol fallback")
}
diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go
index f61b4c8..5fb2ebb 100644
--- a/src/crypto/tls/handshake_server_test.go
+++ b/src/crypto/tls/handshake_server_test.go
@@ -385,13 +385,30 @@
}
clientConfig := &Config{
InsecureSkipVerify: true,
+ MinVersion: VersionTLS10,
}
state, _, err := testHandshake(t, clientConfig, serverConfig)
if err != nil {
t.Fatalf("handshake failed: %s", err)
}
if state.Version != VersionTLS11 {
- t.Fatalf("Incorrect version %x, should be %x", state.Version, VersionTLS11)
+ t.Fatalf("incorrect version %x, should be %x", state.Version, VersionTLS11)
+ }
+
+ clientConfig.MinVersion = 0
+ _, _, err = testHandshake(t, clientConfig, serverConfig)
+ if err == nil {
+ t.Fatalf("expected failure to connect with TLS 1.0/1.1")
+ }
+
+ defer func(old bool) { debugEnableTLS10 = old }(debugEnableTLS10)
+ debugEnableTLS10 = true
+ _, _, err = testHandshake(t, clientConfig, serverConfig)
+ if err != nil {
+ t.Fatalf("handshake failed: %s", err)
+ }
+ if state.Version != VersionTLS11 {
+ t.Fatalf("incorrect version %x, should be %x", state.Version, VersionTLS11)
}
}
@@ -472,6 +489,7 @@
InsecureSkipVerify: true,
ClientSessionCache: NewLRUClientSessionCache(1),
ServerName: "servername",
+ MinVersion: VersionTLS10,
}
// Establish a session at TLS 1.1.
diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go
index 08251b8..0b19502 100644
--- a/src/crypto/tls/handshake_server_tls13.go
+++ b/src/crypto/tls/handshake_server_tls13.go
@@ -110,7 +110,7 @@
if id == TLS_FALLBACK_SCSV {
// Use c.vers instead of max(supported_versions) because an attacker
// could defeat this by adding an arbitrary high version otherwise.
- if c.vers < c.config.maxSupportedVersion() {
+ if c.vers < c.config.maxSupportedVersion(roleServer) {
c.sendAlert(alertInappropriateFallback)
return errors.New("tls: client using inappropriate protocol fallback")
}
diff --git a/src/crypto/tls/handshake_test.go b/src/crypto/tls/handshake_test.go
index 90ac9bd..bacc8b7 100644
--- a/src/crypto/tls/handshake_test.go
+++ b/src/crypto/tls/handshake_test.go
@@ -363,6 +363,8 @@
Certificates: make([]Certificate, 2),
InsecureSkipVerify: true,
CipherSuites: allCipherSuites(),
+ MinVersion: VersionTLS10,
+ MaxVersion: VersionTLS13,
}
testConfig.Certificates[0].Certificate = [][]byte{testRSACertificate}
testConfig.Certificates[0].PrivateKey = testRSAPrivateKey
To view, visit change 359779. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 1, 2021, 3:34:33 PM11/1/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Patch set 1:Code-Review +2
2 comments:
File src/crypto/tls/common.go:
const (
roleClient = true
roleServer = false
)I (think) I get why a bool, but this is kind of confusing. Typed int or something would be somewhat clearer in method arguments.
File src/crypto/tls/handshake_server_test.go:
Patch Set #1, Line 381:
func TestVersion(t *testing.T) {My memory of how these tests work is always somewhat fuzzy, but do we need to regenerate the recorded tests for this or the other test changes?
To view, visit change 359779. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 5, 2021, 5:45:41 PM11/5/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Roland Shoemaker, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
2 comments:
File src/crypto/tls/common.go:
const (
roleClient = true
roleServer = false
)
I (think) I get why a bool, but this is kind of confusing. […]
I went back and forth a number of times. Typed int, besides stuttering because you end up with supportedVersions(role role), leaves open the question of what happens when you check for == roleClient and not == roleServer. Like, is the function always invoked with a valid value? This is really a case for adding enums to the language, but in the meantime I ended up liking the bool a little better. It's just a prettyer version of supportedVersions(isClient bool) at the callsite.
Switched back to supportedVersions(isClient bool) which hopefully is clearer at the definition too.
File src/crypto/tls/handshake_server_test.go:
Patch Set #1, Line 381:
func TestVersion(t *testing.T) {
My memory of how these tests work is always somewhat fuzzy, but do we need to regenerate the recorde […]
We only need to regenerate the OpenSSL based test recordings when the bytes on the wire would have changed. I sort of cheated here by running those with MinVersion set so they don't depend on the default, and the tests for this mechanism are just handshake tests, not OpenSSL tests.
To view, visit change 359779. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 5, 2021, 5:45:42 PM11/5/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Filippo Valsorda uploaded patch set #2 to this change.
crypto/tls: set default minimum client version to TLS 1.2
Updates #45428
Change-Id: I5d70066d4091196ec6f8bfc2edf3d78fdc0520c1
---
M src/crypto/tls/common.go
M src/crypto/tls/handshake_server.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_test.go
M src/crypto/tls/handshake_server_test.go
M src/crypto/tls/handshake_server_tls13.go
6 files changed, 70 insertions(+), 17 deletions(-)
To view, visit change 359779. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 5, 2021, 6:03:32 PM11/5/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, Roland Shoemaker, golang-co...@googlegroups.com
Filippo Valsorda submitted this change.
1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/crypto/tls/common.go
Insertions: 12, Deletions: 12.
@@ -18,9 +18,9 @@
"crypto/x509"
"errors"
"fmt"
+ "internal/godebug"
"io"
"net"
- "os"
"strings"
"sync"
"time"
@@ -975,18 +975,18 @@
}
// debugEnableTLS10 enables TLS 1.0. See issue 45428.
-var debugEnableTLS10 = strings.Contains(os.Getenv("GODEBUG"), "tls10default=1")
+var debugEnableTLS10 = godebug.Get("tls10default") == "1"
-const (
- roleClient = true
- roleServer = false
-)
+// roleClient and roleServer are meant to call supportedVersions and parents
+// with more readability at the callsite.
+const roleClient = true
+const roleServer = false
-func (c *Config) supportedVersions(role bool) []uint16 {
+func (c *Config) supportedVersions(isClient bool) []uint16 {
versions := make([]uint16, 0, len(supportedVersions))
for _, v := range supportedVersions {
if (c == nil || c.MinVersion == 0) && !debugEnableTLS10 &&
- role == roleClient && v < VersionTLS12 {
+ isClient && v < VersionTLS12 {
continue
}
if c != nil && c.MinVersion != 0 && v < c.MinVersion {
@@ -1000,8 +1000,8 @@
return versions
}
-func (c *Config) maxSupportedVersion(role bool) uint16 {
- supportedVersions := c.supportedVersions(role)
+func (c *Config) maxSupportedVersion(isClient bool) uint16 {
+ supportedVersions := c.supportedVersions(isClient)
if len(supportedVersions) == 0 {
return 0
}
@@ -1042,8 +1042,8 @@
// mutualVersion returns the protocol version to use given the advertised
// versions of the peer. Priority is given to the peer preference order.
-func (c *Config) mutualVersion(role bool, peerVersions []uint16) (uint16, bool) {
- supportedVersions := c.supportedVersions(role)
+func (c *Config) mutualVersion(isClient bool, peerVersions []uint16) (uint16, bool) {
+ supportedVersions := c.supportedVersions(isClient)
for _, peerVersion := range peerVersions {
for _, v := range supportedVersions {
if v == peerVersion {
```
Approvals:
Roland Shoemaker: Looks good to me, approved
Filippo Valsorda: Trusted; Run TryBots
Go Bot: TryBots succeeded
crypto/tls: set default minimum client version to TLS 1.2
Updates #45428
Change-Id: I5d70066d4091196ec6f8bfc2edf3d78fdc0520c1
Reviewed-on: https://go-review.googlesource.com/c/go/+/359779
Run-TryBot: Filippo Valsorda <fil...@golang.org>
TryBot-Result: Go Bot <go...@golang.org>
Trust: Filippo Valsorda <fil...@golang.org>
Reviewed-by: Roland Shoemaker <rol...@golang.org>
---
M src/crypto/tls/common.go
M src/crypto/tls/handshake_server.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_test.go
M src/crypto/tls/handshake_server_test.go
M src/crypto/tls/handshake_server_tls13.go
6 files changed, 75 insertions(+), 17 deletions(-)
diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 610a516..bb5bec3 100644
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -18,6 +18,7 @@
"crypto/x509"
"errors"
"fmt"
+ "internal/godebug"
"io"
"net"
"strings"
@@ -682,11 +683,20 @@
ClientSessionCache ClientSessionCache
// MinVersion contains the minimum TLS version that is acceptable.
- // If zero, TLS 1.0 is currently taken as the minimum.
+ //
+ // By default, TLS 1.2 is currently used as the minimum when acting as a
+ // client, and TLS 1.0 when acting as a server. TLS 1.0 is the minimum
+ // supported by this package, both as a client and as a server.
+ //
+ // The client-side default can temporarily be reverted to TLS 1.0 by
+ // including the value "x509sha1=1" in the GODEBUG environment variable.
+ // Note that this option will be removed in Go 1.19 (but it will still be
+ // possible to set this field to VersionTLS10 explicitly).
MinVersion uint16
// MaxVersion contains the maximum TLS version that is acceptable.
- // If zero, the maximum version supported by this package is used,
+ //
+ // By default, the maximum version supported by this package is used,
// which is currently TLS 1.3.
MaxVersion uint16
@@ -964,9 +974,21 @@
VersionTLS10,
}
-func (c *Config) supportedVersions() []uint16 {
+// debugEnableTLS10 enables TLS 1.0. See issue 45428.
+var debugEnableTLS10 = godebug.Get("tls10default") == "1"
+
+// roleClient and roleServer are meant to call supportedVersions and parents
+// with more readability at the callsite.
+const roleClient = true
+const roleServer = false
+
+func (c *Config) supportedVersions(isClient bool) []uint16 {
versions := make([]uint16, 0, len(supportedVersions))
for _, v := range supportedVersions {
+ if (c == nil || c.MinVersion == 0) && !debugEnableTLS10 &&
+ isClient && v < VersionTLS12 {
+ continue
+ }
if c != nil && c.MinVersion != 0 && v < c.MinVersion {
continue
}
@@ -978,8 +1000,8 @@
return versions
}
-func (c *Config) maxSupportedVersion() uint16 {
- supportedVersions := c.supportedVersions()
+func (c *Config) maxSupportedVersion(isClient bool) uint16 {
+ supportedVersions := c.supportedVersions(isClient)
if len(supportedVersions) == 0 {
return 0
}
@@ -1020,8 +1042,8 @@
// mutualVersion returns the protocol version to use given the advertised
// versions of the peer. Priority is given to the peer preference order.
-func (c *Config) mutualVersion(peerVersions []uint16) (uint16, bool) {
- supportedVersions := c.supportedVersions()
+func (c *Config) mutualVersion(isClient bool, peerVersions []uint16) (uint16, bool) {
+ supportedVersions := c.supportedVersions(isClient)
To view, visit change 359779. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Nov 5, 2021, 7:31:08 PM11/5/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Bryan C. Mills, Go Bot, Roland Shoemaker, golang-co...@googlegroups.com
1 comment:
Patchset:
This change appears to have broken the x/net builders.
To view, visit change 359779. To unsubscribe, or for help writing mail filters, visit settings.
Dirkjan Bussink (Gerrit)
Nov 6, 2021, 3:32:49 PM11/6/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Bryan C. Mills, Go Bot, Roland Shoemaker, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
1 comment:
File src/crypto/tls/common.go:
Patch Set #3, Line 692:
// including the value "x509sha1=1" in the GODEBUG environment variable.I think this should be tls10default and not x509sha1?
To view, visit change 359779. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages