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.

Sign up for GitHub

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

Jump to bottom

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
Closed

crypto/tls: TLS13 handshake does not fail if no cipherSuites overlap between server and client #30109

mkumatag opened this issue Feb 6, 2019 · 5 comments
Labels
FrozenDueToAge

Comments

@mkumatag
Copy link

mkumatag commented Feb 6, 2019 •
edited

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
Copy link
Author

mkumatag commented Feb 6, 2019

/cc @FiloSottile

@mkumatag
Copy link
Author

mkumatag commented Feb 6, 2019

/cc @bradfitz

@nussjustin
Copy link
Contributor

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
Copy link
Contributor

crvv commented Feb 6, 2019

Looks like a duplicate of #29349

@mkumatag
Copy link
Author

mkumatag commented Feb 6, 2019

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

@mkumatag mkumatag closed this as completed Feb 6, 2019
@mkumatag mkumatag mentioned this issue Feb 7, 2019
Closed
@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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@crvv @nussjustin @gopherbot @mkumatag