crypto/tls: don't include supported_versions extension if MaxVersion < 1.3 #53384

jameshartig · 2022-06-15T00:22:58Z

What version of Go are you using (`go version`)?

$ go version
go version go1.18.3 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\james\AppData\Local\go-build
set GOENV=C:\Users\james\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\james\go\pkg\mod
set GONOPROXY=gerrit.levenlabs.com,gitlab.com/levenlabs
set GONOSUMDB=gerrit.levenlabs.com,gitlab.com/levenlabs
set GOOS=windows
set GOPATH=C:\Users\james\go;
set GOPRIVATE=gerrit.levenlabs.com,gitlab.com/levenlabs
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.18.3
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\james\Dropbox\aftermath\backend\vigilante\go.mod
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\james\AppData\Local\Temp\go-build1041663077=/tmp/go-build -gno-record-gcc-switches

What did you do?

If you make a TLS 1.2 ClientHello to some servers and include a supported_versions extension the server will report back a handshake failure. I was comparing why Go failed but openssl s_client ... -tls1_2 -cipher 'ECDHE-RSA-AES128-GCM-SHA256' worked and I figured out that the difference is that Go sends the supported_versions and openssl doesn't.

It seems that supported_versions is a TLS 1.3 extension so we probably shouldn't be including it when the MaxVersion is less than 1.3.

Here's a way to reproduce it against google.com:

package main

import (
	"crypto/tls"
	"net"
	"net/http"
	"time"
)

func main() {
	hc := &http.Client{
		Transport: &http.Transport{
			DialContext: (&net.Dialer{
				Timeout:   10 * time.Second,
				KeepAlive: 10 * time.Second,
			}).DialContext,
			MaxIdleConns:          1,
			IdleConnTimeout:       30 * time.Second,
			TLSHandshakeTimeout:   30 * time.Second,
			ExpectContinueTimeout: 1 * time.Second,
			TLSClientConfig: &tls.Config{
				MinVersion: tls.VersionTLS12,
				MaxVersion: tls.VersionTLS12,
				CipherSuites: []uint16{
					tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
					tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
				},
			},
		},
	}
	resp, err := hc.Get("https://google.com/")
	if err != nil {
		panic(err)
	}
	resp.Body.Close()
}

I realize the above is a contrived example but the fact that Google has this problem (and all GCP load balancers) seems to justify that it's not a random server that's misbehaving.

What did you expect to see?

I expected the program not to panic.

What did you see instead?

Instead I get panic: Get "https://google.com/": remote error: tls: handshake failure.

If I change marshal in handshake_messages.go to:

if len(m.supportedVersions) > 0 && m.supportedVersions[0] >= VersionTLS13 {

Then the above program no longer errors.

The text was updated successfully, but these errors were encountered:

mengzhuo · 2022-06-15T02:02:16Z

For refs, RFC8446:

4.2.1. Supported Versions
......
A server which negotiates a version of TLS prior to TLS 1.3 MUST set
ServerHello.version and MUST NOT send the "supported_versions"
extension.

jameshartig · 2022-06-15T02:27:32Z

This is technically the client so I don't think that applies. But I noticed it does say:

Servers MUST be prepared to receive ClientHellos that include this extension but do not include 0x0304 in the list of versions.

So it appears that Google is in the wrong here since they support TLS 1.2 but don't expect to negotiate 1.2 if that extension is present.

However, if a server doesn't support TLS 1.3, they wouldn't know about this extension.

mengzhuo · 2022-06-15T06:03:14Z

@rolandshoemaker @golang/security

FiloSottile · 2022-06-15T07:16:07Z

This looks like server intolerance for the supported_versions extension in TLS 1.2 in BoringSSL. @davidben @agl can you have a look?

davidben · 2022-06-15T15:21:16Z

Odd! I can repro it with the script, but I'm pretty sure we test this case with BoringSSL, and it doesn't repro with other servers that use BoringSSL. This may be a bug in some code we have in Google's server. Searching around there...

davidben · 2022-06-15T15:36:02Z

Ah, I see the issue. It's a combination of sending supported_versions, not including the ECDSA version of those ciphers, but including ECDSA in signature algorithms. (Doesn't look like Go lets you configure that one.) You should probably be including the ECDSA versions anyway, so hopefully that'll resolve it for you? Though I acknowledge the google.com behavior isn't great.

Why this happens is that determining ECDSA support in TLS is bit of a mess and has migrated over time:

In TLS 1.0 and 1.1, it was based on a combination of the cipher suite list and the supported_curves extension
In TLS 1.2, it is also was all of the above and also the signature_algorithms extension
In TLS 1.3, we separated all these back out. The cipher suite is now just the AEAD+PRF. supported_curves (now supported_groups) is just the key exchange. signature_algorithms is just the signing key and now has the curve folded in. So you only need to look at the last one.

The TLS 1.3 behavior is cleaner but the combination of all three modes leaves a bit of a mess: google.com resolves SNI, ECDSA, etc., all at once, when we see the ClientHello. At that point, it's not quite yet known which TLS version we'll be using. So we assume that any client which sends supported_versions is at least new enough for the signature_algorithms extension to be accurate and assume the TLS 1.3 check is sufficient. So, with this ClientHello, we see ecdsa_secp256r1_sha256 in the signature algorithms extension and assume this must be a new enough client to speak ECDSA and configure the ECDSA certificate.

From there, the handshake continues, we negotiate TLS 1.2 via supported_versions, and find there are no acceptable cipher suites for an ECDSA certificate. Thus the handshake fails with handshake_failure. More robust would be for BoringSSL to support configuring both certificates and internally select it, since the selection logic is far too messy to expect a caller to implement.

jameshartig · 2022-06-15T16:22:26Z

You should probably be including the ECDSA versions anyway, so hopefully that'll resolve it for you?

Ah that's good to know! I appreciate the through investigation. For this one case, we actually are purposefully not including ECDSA so we can test the RSA certificate we've deployed to a GCP load balancer. We have 2 different configs one that includes ECDSA ciphers and one that doesn't.

Not sure where we go from here. Should we be able to configure the signature algorithms in Go or is this squarely a bug on Google's end and there's no Go change necessary?

Besides Google, do we envision problems with TLS 1.2 servers by sending supported_versions?

FiloSottile · 2022-06-15T17:03:25Z

Thank you for the investigation @davidben! The certificate compatibility logic is indeed a major mess, our own implementation was one of the most painful things I had to write.

We could add some logic to not send ECDSA in signature_algorithms when not sending ECDSA cipher suites and not offering TLS 1.3, but I feel like that's propagating complexity in the wrong direction. I would argue that this is a server bug, as the server is negotiating TLS 1.2 so it should check the intersection of cipher suites, signature algorithms, supported curves, and point compression, like it would if there was no supported_versions.

As usual, Go doing something a little different is helpful in greasing the ecosystem, if we can afford it :)

mengzhuo added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 15, 2022

mengzhuo added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jun 15, 2022

seankhliao added this to the Unplanned milestone Aug 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto/tls: don't include supported_versions extension if MaxVersion < 1.3 #53384

crypto/tls: don't include supported_versions extension if MaxVersion < 1.3 #53384

jameshartig commented Jun 15, 2022

mengzhuo commented Jun 15, 2022

jameshartig commented Jun 15, 2022

mengzhuo commented Jun 15, 2022

FiloSottile commented Jun 15, 2022 •

edited

davidben commented Jun 15, 2022

davidben commented Jun 15, 2022

jameshartig commented Jun 15, 2022

FiloSottile commented Jun 15, 2022

crypto/tls: don't include supported_versions extension if MaxVersion < 1.3 #53384

crypto/tls: don't include supported_versions extension if MaxVersion < 1.3 #53384

Comments

jameshartig commented Jun 15, 2022

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

mengzhuo commented Jun 15, 2022

jameshartig commented Jun 15, 2022

mengzhuo commented Jun 15, 2022

FiloSottile commented Jun 15, 2022 • edited

davidben commented Jun 15, 2022

davidben commented Jun 15, 2022

jameshartig commented Jun 15, 2022

FiloSottile commented Jun 15, 2022

What version of Go are you using (`go version`)?

What operating system and processor architecture are you using (`go env`)?

FiloSottile commented Jun 15, 2022 •

edited