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: reject TLS 1.3 ciphers in Config.CipherSuites #29349

Closed
crvv opened this issue Dec 20, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@crvv
Copy link
Contributor

commented Dec 20, 2018

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

go version devel +e3b4b7baad Tue Dec 18 23:01:06 2018 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes if 1.12beta1 is the latest release.

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

go env Output
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/crvv/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/crvv/Develop/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/crvv/Develop/goroot"
GOTMPDIR=""
GOTOOLDIR="/Users/crvv/Develop/goroot/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/x1/1pp78x6d3n99gpx9f7rz2_rh0000gn/T/go-build965128833=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Run the following code with GODEBUG=http2server=0 go run main.go

package main

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

func main() {
	server := http.Server{
		Addr: "localhost:2443",
		TLSConfig: &tls.Config{
			CipherSuites: []uint16{tls.TLS_CHACHA20_POLY1305_SHA256},
			MinVersion:   tls.VersionTLS13,
		},
	}
	log.Fatal(server.ListenAndServeTLS("localhost.pem", "localhost-key.pem"))
}

and openssl s_client -connect localhost:2443

What did you expect to see?

A TLS 1.3 connection with TLS_CHACHA20_POLY1305_SHA256 cipher.

What did you see instead?

SSL handshake has read 1603 bytes and written 391 bytes
Verification error: unable to verify the first certificate
---
New, TLSv1.3, Cipher is TLS_AES_256_GCM_SHA384

It looks like Config.CipherSuites isn't used in TLS 1.3.
If this is the desired behavior, I think it should be documented in https://tip.golang.org/pkg/crypto/tls/

@agnivade

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

From the 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 Author

commented Dec 20, 2018

I understand the reason to make it not configurable.

But there are only two kinds of CipherSuites.

[]uint16{TLS_AES_128_GCM_SHA256, TLS_CHACHA20_POLY1305_SHA256, TLS_AES_256_GCM_SHA384}

and

[]uint16{TLS_CHACHA20_POLY1305_SHA256, TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384}

That is to say, if the server and client are both written in Go, TLS_AES_256_GCM_SHA384 can never be used, which I think is kind of strange.

Besides the documentation, I think this can be reconsidered or maybe tls.Listen should return an error if Config.CipherSuites contains TLS 1.3 ciphers.

@agnivade

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

@ulikunitz

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

The TLS 1.3 implementation should at least allow the developer to select cipher suites from the supported set of cipher suites and change the priority of the supported cipher suites.

Why? It is obvious that the security assessment of the ciphers defined by TLS 1.3 might change. I'm old enough to know that SHA1 was the recommended hash algorithm for quite some time. Allowing the developer to exclude an algorithm gives him a chance to react for instance to meet internal compliance requirements while the cipher suite must still be supported in general to support users with legacy software.

The capability to mask interoperability issues with another TLS implementation instantly, is another reason to provide developers with the capability to change priorities of the supported algorithms.

@FiloSottile FiloSottile changed the title crypto/tls: Config.CipherSuites doesn't work for TLS 1.3 crypto/tls: reject TLS 1.3 ciphers in Config.CipherSuites Dec 20, 2018

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Configuration is necessary if the ecosystem has issues that require manual mitigation. The TLS 1.3 ecosystem thankfully doesn't yet, so until it's necessary we will not add a config option. Cipher suites happen to be something that required configuration in TLS 1.2, but by the argument that everything should be configurable in case it becomes necessary later, there are half a dozen axes in TLS 1.3 (cipher suites, groups, signature algorithms, certificate signature algorithms, PSK modes...) and we are not adding a config option for each.

Returning an error if Config.CipherSuites contains TLS 1.3 ciphers sounds like a good idea, though.

@FiloSottile FiloSottile self-assigned this Dec 21, 2018

@FiloSottile FiloSottile added this to the Go1.12 milestone Dec 21, 2018

@ulikunitz

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Yes, the usage of cryptogragraphic methods must be configurable. Beside the two reasons, change of security assessment of a cryptographic algorithm and handling of interoperability issues, I have already given, there is a third one: compliance to government or internal policies. Such a policy might forbid ChaCha20, because the algorithm is not supported by government standards. How does a developer address such a requirement?

Not everybody shares your assessment that ciphers in TLS 1.3 have no issues. Dan Bernstein and Tanja Lange regard P256 as unsafe as documented in https://safecurves.cr.yp.to/. Apparently the Go implementation shares that assessment, because it prefers X25519 over P256. Burt what about developers that want to disable P256 completely?

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

@ulikunitz curves have nothing to do with cipher suites in TLS 1.3.

tls.Listen-time check are probably not worth it (#29779 (comment)) so I'm going to add a note to the Config.CipherSuites docs, and close this.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 18, 2019

Change https://golang.org/cl/158637 mentions this issue: crypto/tls: expand Config.CipherSuites docs

@gopherbot gopherbot closed this in 6f93f86 Jan 18, 2019

@ulikunitz

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2019

@FiloSottile

Let me write at first that I'm aware that you were deeply involved in the development of TLS 1.3. I watched your presentation at the Chaos Communication Congress regarding it. I appreciate the work you put in the development in the new algorithm specifically the work on TLS 1.3.

Let me clarify the point regarding P265. In my opinion every TLS implementation should at least provide the capability to disable any cryptographic algorithm for which there are multiple alternatives available independent how they are negotiated in the protocol. So my ask is actually independent of the cipher suites.

The rationale is:

  1. A lot of organisations have policies in place that define the set of algorithms including important parameters like key lengths that can be supported in the organisation. Not providing a capability to disable certain algorithms will create issues for the use of the Go language in those organisations.

  2. In the practice there are issues for specific algorithms in TLS implementations. We observe regularly TLS connection resets at a rate of ca. 1/3000 in our production environment with Oracle's Java implementation of ECDHE for TLS 1.2. Since Java supports the disablement of crypto algorithms for TLS, we are able to work around the issue until it can be resolved with Oracle.

  3. In the case of the identification of a vulnerability or general cryptographic problem of the TLS 1.3 Golang implementation there is only the option of updating the software. In my experience a software update triggers more dependencies and requires more testing effort then a software configuration change. The time frame differs here between minutes to sevaral hours for the configuration change and days to weeks for the software update.

Since I'm not providing patches to do what I see as necessary, I will stop discussing the point any further. I'm convinced that you and I want the best for the Golang community while we disagree that this
includes configurability of the cryptographic algorithms for the Golang TLS 1.3 implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.