crypto/tls: enforce TLS 1.3 (and TLS 1.2) downgrade protection checks

Fixes #37763

Change-Id: Ic6bcc9af0d164966f4ae31087998e5b546540038
Reviewed-on: https://go-review.googlesource.com/c/go/+/231038
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 9121148ee853d3ab6e0573602146255b5d8fe49c..9bd7005fc1ab2d4fca44ef4d980213df2a20ad86 100644 (file)
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -207,6 +207,10 @@ const (
        downgradeCanaryTLS11 = "DOWNGRD\x00"
 )
 
+// testingOnlyForceDowngradeCanary is set in tests to force the server side to
+// include downgrade canaries even if it's using its highers supported version.
+var testingOnlyForceDowngradeCanary bool
+
 // ConnectionState records basic TLS details about the connection.
 type ConnectionState struct {
        Version                     uint16                // TLS version used by the connection (e.g. VersionTLS12)

diff --git a/src/crypto/tls/handshake_client.go b/src/crypto/tls/handshake_client.go
index 64be82e88c714d87e1cbf5ddb68e35316bb03b8b..210eece26dd4f9e0826e6a762978e72f5b96dccd 100644 (file)
--- a/src/crypto/tls/handshake_client.go
+++ b/src/crypto/tls/handshake_client.go
@@ -54,7 +54,7 @@ func (c *Conn) makeClientHello() (*clientHelloMsg, ecdheParameters, error) {
                return nil, nil, errors.New("tls: no supported versions satisfy MinVersion and MaxVersion")
        }
 
-       clientHelloVersion := supportedVersions[0]
+       clientHelloVersion := config.maxSupportedVersion()
        // 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.
@@ -181,6 +181,18 @@ func (c *Conn) clientHandshake() (err error) {
                return err
        }
 
+       // 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()
+       tls12Downgrade := string(serverHello.random[24:]) == downgradeCanaryTLS12
+       tls11Downgrade := string(serverHello.random[24:]) == downgradeCanaryTLS11
+       if maxVers == VersionTLS13 && c.vers <= VersionTLS12 && (tls12Downgrade || tls11Downgrade) ||
+               maxVers == VersionTLS12 && c.vers <= VersionTLS11 && tls11Downgrade {
+               c.sendAlert(alertIllegalParameter)
+               return errors.New("tls: downgrade attempt detected, possibly due to a MitM attack or a broken middlebox")
+       }
+
        if c.vers == VersionTLS13 {
                hs := &clientHandshakeStateTLS13{
                        c:           c,

diff --git a/src/crypto/tls/handshake_client_test.go b/src/crypto/tls/handshake_client_test.go
index 8632d98697a7b7168b4045cd5726862bb2461689..35d7136fc674c228263e1d41198710eb1ab681b2 100644 (file)
--- a/src/crypto/tls/handshake_client_test.go
+++ b/src/crypto/tls/handshake_client_test.go
@@ -1984,3 +1984,48 @@ func TestCloseClientConnectionOnIdleServer(t *testing.T) {
                t.Errorf("Error expected, but no error returned")
        }
 }
+
+func testDowngradeCanary(t *testing.T, clientVersion, serverVersion uint16) error {
+       defer func() { testingOnlyForceDowngradeCanary = false }()
+       testingOnlyForceDowngradeCanary = true
+
+       clientConfig := testConfig.Clone()
+       clientConfig.MaxVersion = clientVersion
+       serverConfig := testConfig.Clone()
+       serverConfig.MaxVersion = serverVersion
+       _, _, err := testHandshake(t, clientConfig, serverConfig)
+       return err
+}
+
+func TestDowngradeCanary(t *testing.T) {
+       if err := testDowngradeCanary(t, VersionTLS13, VersionTLS12); err == nil {
+               t.Errorf("downgrade from TLS 1.3 to TLS 1.2 was not detected")
+       }
+       if testing.Short() {
+               t.Skip("skipping the rest of the checks in short mode")
+       }
+       if err := testDowngradeCanary(t, VersionTLS13, VersionTLS11); err == nil {
+               t.Errorf("downgrade from TLS 1.3 to TLS 1.1 was not detected")
+       }
+       if err := testDowngradeCanary(t, VersionTLS13, VersionTLS10); err == nil {
+               t.Errorf("downgrade from TLS 1.3 to TLS 1.0 was not detected")
+       }
+       if err := testDowngradeCanary(t, VersionTLS12, VersionTLS11); err == nil {
+               t.Errorf("downgrade from TLS 1.2 to TLS 1.1 was not detected")
+       }
+       if err := testDowngradeCanary(t, VersionTLS12, VersionTLS10); err == nil {
+               t.Errorf("downgrade from TLS 1.2 to TLS 1.0 was not detected")
+       }
+       if err := testDowngradeCanary(t, VersionTLS13, VersionTLS13); err != nil {
+               t.Errorf("server unexpectedly sent downgrade canary for TLS 1.3")
+       }
+       if err := testDowngradeCanary(t, VersionTLS12, VersionTLS12); err != nil {
+               t.Errorf("client didn't ignore expected TLS 1.2 canary")
+       }
+       if err := testDowngradeCanary(t, VersionTLS11, VersionTLS11); err != nil {
+               t.Errorf("client unexpectedly reacted to a canary in TLS 1.1")
+       }
+       if err := testDowngradeCanary(t, VersionTLS10, VersionTLS10); err != nil {
+               t.Errorf("client unexpectedly reacted to a canary in TLS 1.0")
+       }
+}

diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go
index b16415a03c238326a3004f43c1df8eb206f7529e..35ac7b852a656ecab112cf375e647ed722820672 100644 (file)
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -193,7 +193,7 @@ func (hs *serverHandshakeState) processClientHello() error {
        serverRandom := hs.hello.random
        // Downgrade protection canaries. See RFC 8446, Section 4.1.3.
        maxVers := c.config.maxSupportedVersion()
-       if maxVers >= VersionTLS12 && c.vers < maxVers {
+       if maxVers >= VersionTLS12 && c.vers < maxVers || testingOnlyForceDowngradeCanary {
                if c.vers == VersionTLS12 {
                        copy(serverRandom[24:], downgradeCanaryTLS12)
                } else {