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: TLS13 handshake does not fail if no cipherSuites overlap between server and client #30109

Closed
mkumatag opened this issue Feb 6, 2019 · 5 comments

Comments

@mkumatag
Copy link

@mkumatag mkumatag commented Feb 6, 2019

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

$ go version
go version devel +ccd9d9d4ce Tue Feb 5 21:10:06 2019 +0000 darwin/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
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/manjunath/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/manjunath/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/manjunath/golang_ws/src/github.com/golang/go"
GOTMPDIR=""
GOTOOLDIR="/Users/manjunath/golang_ws/src/github.com/golang/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/rf/z88sdmdd33x0wvw5_2c5rl2r0000gn/T/go-build653561422=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Server is started with following tls config:

	cfg := &tls.Config{
		ClientAuth: tls.RequireAndVerifyClientCert,
		ClientCAs:  caCertPool,
        CipherSuites: []uint16{
            tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
            tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
        },
	}

client:

	client := &http.Client{
		Transport: &http.Transport{
			TLSClientConfig: &tls.Config{
				RootCAs:      caCertPool,
				Certificates: []tls.Certificate{cert},
                CipherSuites: []uint16{
                    tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
                    tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
                    tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
                    tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
                },
			},
		},
	}

in the above code snippit, there is no overlap of CipherSuites between server and client connection, actually handshake should fail with following error but does not fail in latest golang(master)

2019/02/06 19:32:16 http: TLS handshake error from 127.0.0.1:58899: tls: no cipher suite supported by both client and server

Complete test code available at https://github.com/mkumatag/golang-https-example
Prereq:

openssl req \
    -newkey rsa:2048 \
    -nodes \
    -days 3650 \
    -x509 \
    -keyout ca.key \
    -out ca.crt \
    -subj "/CN=*"
openssl req \
    -newkey rsa:2048 \
    -nodes \
    -keyout server.key \
    -out server.csr \
    -subj "/C=GB/ST=London/L=London/O=Global Security/OU=IT Department/CN=*"
openssl x509 \
    -req \
    -days 365 \
    -sha256 \
    -in server.csr \
    -CA ca.crt \
    -CAkey ca.key \
    -CAcreateserial \
    -out server.crt \
    -extfile <(echo subjectAltName = IP:127.0.0.1)
openssl req \
    -x509 \
    -nodes \
    -newkey rsa:2048 \
    -keyout client.key \
    -out client.crt \
    -days 3650 \
    -subj "/C=GB/ST=London/L=London/O=Global Security/OU=IT Department/CN=*"

server code: https://github.com/mkumatag/golang-https-example/blob/master/https_server.go
client code: https://github.com/mkumatag/golang-https-example/blob/master/https_client.go

What did you expect to see?

2019/02/06 19:32:16 http: TLS handshake error from 127.0.0.1:58899: tls: no cipher suite supported by both client and server

What did you see instead?

Instead I don't see any errors and handshake goes through.

One possible fix can be fix this issue is:

diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go
index fd65ac1..711e33f 100644
--- a/src/crypto/tls/handshake_server_tls13.go
+++ b/src/crypto/tls/handshake_server_tls13.go
@@ -149,11 +149,11 @@ func (hs *serverHandshakeStateTLS13) processClientHello() error {

        var preferenceList, supportedList []uint16
        if c.config.PreferServerCipherSuites {
-               preferenceList = defaultCipherSuitesTLS13()
+               preferenceList = c.config.cipherSuites()
                supportedList = hs.clientHello.cipherSuites
        } else {
                preferenceList = hs.clientHello.cipherSuites
-               supportedList = defaultCipherSuitesTLS13()
+               supportedList = c.config.cipherSuites()
        }
        for _, suiteID := range preferenceList {
                hs.suite = mutualCipherSuiteTLS13(supportedList, suiteID)

but started seeing lot regression issues. Copied the logic from handshake_server.go file.

@mkumatag

This comment has been minimized.

Copy link
Author

@mkumatag mkumatag commented Feb 6, 2019

@mkumatag

This comment has been minimized.

Copy link
Author

@mkumatag mkumatag commented Feb 6, 2019

/cc @bradfitz

@nussjustin

This comment has been minimized.

Copy link
Contributor

@nussjustin nussjustin commented Feb 6, 2019

From the Go 1.12 Release Notes:

TLS 1.3 cipher suites are not configurable. All supported cipher suites are safe, and if PreferServerCipherSuites is set in Config the preference order is based on the available hardware.

@crvv

This comment has been minimized.

Copy link
Contributor

@crvv crvv commented Feb 6, 2019

Looks like a duplicate of #29349

@mkumatag

This comment has been minimized.

Copy link
Author

@mkumatag mkumatag commented Feb 6, 2019

closing this issued based on above 2 comments, thanks @nussjustin @crvv

@mkumatag mkumatag closed this Feb 6, 2019
@golang golang locked and limited conversation to collaborators Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.