crypto/tls: Failure to handshake when ClientHello lacks Point Formats Extension

[cipherboy]

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

$ go version
go version go1.18.4 linux/amd64

Does this issue reproduce with the latest release?

Yes; it is present on master.

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cipherboy/.cache/go-build"
GOENV="/home/cipherboy/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/cipherboy/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cipherboy/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3518916122=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This was discovered during testing between a partner and Hashicorp. This partner's TLSv1.2-only (third-party) client refused to connect to a Vault Go TLS server.

Analysis of the Client Hello sent by this program showed that it sent the supported groups extension (expecting to enable forward-secrecy via ECDHE), but elided the (RFC 8422) Client Hello Point Format extension. This client was configured to only send ECDHE algorithms (e.g., TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384), so the handshake failed with a cipher suite mismatch handshake exception. However, this client supports the uncompressed point format and so its expected the handshake should succeed.

In replicating this behavior against other popular open TLS servers (NSS, GnuTLS, and OpenSSL), Go was alone in refusing the handshake. All the others would accept the handshake and use uncompressed point format for ECHDE negotiation.

Per RFC 8422 Section 5.1.2:

RFC 4492 specified that if this extension is missing, it means that only the uncompressed point format is supported, so interoperability with implementations that support the uncompressed format should work with or without the extension.

The reference to RFC 8422 and RFC 4492 comes via the Go source, implying that this behavior should've been supported by Go.

In particular, in this helper function (refactored from the pre-Go 1.14 code which had the same bug):

go/src/crypto/tls/handshake_server.go

Lines 306 to 326 in 4248146

	// supportsECDHE returns whether ECDHE key exchanges can be used with this
	// pre-TLS 1.3 client.
	func supportsECDHE(c *Config, supportedCurves []CurveID, supportedPoints []uint8) bool {
	supportsCurve := false
	for _, curve := range supportedCurves {
	if c.supportsCurve(curve) {
	supportsCurve = true
	break
	}
	}
	supportsPointFormat := false
	for _, pointFormat := range supportedPoints {
	if pointFormat == pointFormatUncompressed {
	supportsPointFormat = true
	break
	}
	}
	return supportsCurve && supportsPointFormat
	}

we see that the value for supportsPointFormat is strictly computed from the extension if it is non-empty and present. However, if the extension is elided altogether, it retains its default value of false. This is incorrect per the referenced RFCs.

This is reproducible via the following Go program:

package main

import (
	"flag"
	"log"
	"net/http"
)

func main() {
	cert := flag.String("cert", "localhost.crt", "Server certificate in PEM format")
	key := flag.String("key", "localhost.key", "Server certificate's corresponding key, in PEM format")
	addr := flag.String("addr", ":443", "Server's listening address")

	flag.Parse()

	log.Printf("Running with cert: %v / key: %v", *cert, *key)

	http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
		w.Header().Set("Content-Type", "text/plain")
		w.Write([]byte("go-server@tlsfuzzer\n"))
		log.Println("Replied to request")
	})

	err := http.ListenAndServeTLS(*addr, *cert, *key, nil)
	if err != nil {
		log.Fatalf("Got error while serving (on %v): %v", *addr, err)
	}
}

With the packet stream (see attached pcap from tlsfuzzer scenario above):

tlsfuzzer-repo-pcapng.zip

$ go run ./main.go -addr ":8443" &
$ echo "1603000099010000950303000000000000000000000000000000000000000000000000000000000000000000000ac009c013c02bc02f00ff01000062000a0006000400170100000d00280026080b080a08090806080508040601050104010301020101010603050304030303020308080807003200280026080b080a08090806080508040601050104010301020101010603050304030303020308080807" | xxd -r -p | nc localhost 8443 | xxd -p
15030300020228

(the hex result decodes to alert / Handshake Failure).

What did you expect to see?

Handshake success.

What did you see instead?

Handshake failure.

Possible Fixes

As I understand it, there's two ways of fixing this: we could use presence of Supported Groups (+ECDHE suites) as sufficient evidence towards ECDHE support for uncompressed point formats (easy) -- or prefer to negotiate a non-ECDHE cipher ahead of a ECDHE-cipher.

1. In the former, case this looks like changing the helper above to define supportsPointFormat := len(supportedPoints) == 0, to allow an unspecified extension to accept ECDHE negotiation automatically. This would fix the value for hs.ecdheOk and allow ECDHE to succeed unconditionally. The downside of this approach is that we could choose a ECDHE cipher when the client doesn't actually support Uncompressed point format, which I think is rarely the case (especially given both RFC 8422 and 4492 above). The client would then fail the handshake, and could probably provide justification to fix the client.
   • n.b.: during unmarshalling, we already check that the wire supportedPoints length is non-empty in the extension, so the len check is correct.
2. Otherwise, we could also condition it around having only ECDHE (and DHE? Hmmmm... ) cipher suites in the first case (i.e., something like len(supportedPoints) == 0 && len(nonECDHEClientHelloCipherSuites) == 0. The downside of this approach is that we could elide forward secrecy in the scenario when DHE isn't negotiated (impossible with a Go server AFAICT?) and the client was hoping for ECDHE. Instead, we'd end up on a legacy suite without Forward Secrecy, which is definitely less than ideal.

IMO, I'd prefer the first approach.

Either way, I'm happy to write a patch with test cases for this scenario. Let me know what the thoughts are w.r.t. approach.

[seankhliao]

Duplicate of #49126