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

x/net/http2: expose which TLS cipher suites are HTTP/2 safe #41068

Open
noneymous opened this issue Aug 27, 2020 · 16 comments
Open

x/net/http2: expose which TLS cipher suites are HTTP/2 safe #41068

noneymous opened this issue Aug 27, 2020 · 16 comments
Assignees
Milestone

Comments

@noneymous
Copy link

@noneymous noneymous commented Aug 27, 2020

Launching a web server with the SSL ciphers list taken from the TLS package does not work. I get the following error, that I can't get around programatically:

http2: TLSConfig.CipherSuites index 5 contains an HTTP/2-approved cipher suite (0x1301), but it comes after unapproved cipher suites. With this configuration, clients that don't support previous, approved cipher suites may be given an unapproved one and reject the connection.

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

1.14

Does this issue reproduce with the latest release?

yes (1.15)

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

go env Output
set GOHOSTARCH=amd64
set GOHOSTOS=windows

What did you do?

I tried to launch a web server with a custom TLS config, with ciphers taken from tls.CipherSuites(). I do have one of these 2 dead ends:

  • Ciphers returned by tls.CipherSuites() are not in a suitable order with HTTP/2-approved ciphers first (I guess, they are not intended to be).
  • http2isBadCipher() in /net/http/h2_bundle.go is not exposed, so I can't sort ciphers accordingly on my own (I don't want to copy that function, because it seems it could get updated over time).

Here is a sample snippet:

		// Prepare list of accepted cipher suites
		var ciphers []uint16
		for _, cipher := range tls.CipherSuites() {
			if cipher.Insecure == false {
				ciphers = append(ciphers, cipher.ID)
			}
		}

		// Create TLS config
		tlsConf :=  &tls.Config{
			MinVersion:         tls.VersionTLS13,
			MaxVersion:         tls.VersionTLS13,
			CipherSuites:       ciphers,
		}

		// Create TLS web server
		server := &http.Server{Addr: listen, Handler: mux, TLSConfig: tlsConf}

		// Start TLS web server
		_ = server.ListenAndServeTLS("cert.crt", "cert.key")

What did you expect to see?

I just couldn't find a way to programatically order the ciphers as required, with the means available.

What did you see instead?

A critical error and program termination.

@FiloSottile FiloSottile changed the title /net/http: TLSConfig.CipherSuites index 5 contains an HTTP/2-approved cipher suite crypto/tls: no way to know what CipherSuites are HTTP/2 safe Aug 27, 2020
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Aug 27, 2020

We might consider adding an HTTP2 bool field to the CipherSuite type, but why are you doing this in the first place? You are effectively setting CipherSuites to the default list, but with worse ordering. Moreover, you are using TLS 1.3, so CipherSuites is ignored because TLS 1.3 suites are not configurable.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Aug 27, 2020

That error does need some tweaking though, because the order doesn't matter unless PreferServerCipherSuites is set, and it could also just ignore the suites if MinVersion is VersionTLS13.

@cagedmantis cagedmantis added this to the Backlog milestone Aug 28, 2020
@noneymous
Copy link
Author

@noneymous noneymous commented Sep 2, 2020

(MinVersion should/could be 1.2, my bad)

Could you please point me to the place where the default list of ciphers is selected/generated (and their order is decided), if CipherSuites is not set by me? I could not find it, so I didn't trust it and tried to set it on my own...

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Sep 2, 2020

func initDefaultCipherSuites() {

It's not documented because it can change and evolve for security and performance reasons.

@noneymous
Copy link
Author

@noneymous noneymous commented Sep 2, 2020

Hm, okay, I see... many places where things are defined/decided and generally little control, other than hardcoding / maintaining a custom list and always using the latest Golang version to compile, etc...

Somehow I'm starting to doubt whether ciphers hardcoded/compiled into the executable are a good idea... If something changes (e.g. a vulnerability comes up) one needs to have the source code and recompile the program...

I guess this issue can be closed, thank you!

@noneymous
Copy link
Author

@noneymous noneymous commented Sep 2, 2020

One additional question:

"varDefaultCipherSuites" (i guess you've got a typo in that variable name, but luckily it's private ^^) is the default list in every tls.Config, right? So also third party packages, consuming tls.Config would use those good ciphers (unless explicitly set differently)...?

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Sep 2, 2020

Hm, okay, I see... many places where things are defined/decided and generally little control, other than hardcoding / maintaining a custom list and always using the latest Golang version to compile, etc...

Most developers want us to make the decisions on specific algorithms and to keep those up to date.

If something changes (e.g. a vulnerability comes up) one needs to have the source code and recompile the program...

If a vulnerability comes up developers are expected to obtained a patched version and recompile, like any other vulnerability.

I guess this issue can be closed, thank you!

I am going to keep it open to expose a HTTP2 bool in CipherSuite. Thank you!

"varDefaultCipherSuites" (i guess you've got a typo in that variable name, but luckily it's private ^^) is the default list in every tls.Config, right? So also third party packages, consuming tls.Config would use those good ciphers (unless explicitly set differently)...?

Correct! What's the typo?

@noneymous
Copy link
Author

@noneymous noneymous commented Sep 2, 2020

If a vulnerability comes up developers are expected to obtained a patched version and recompile, like any other vulnerability.

Well, not if cipher suites aren't part of the application, but instead outsourced to OpenSSL. Then OpenSSL (ideally) maintains a good and secure cipher set. And application adminsitrators can update OpenSSL and change the applied ciphers via application configuration... no need to re-compile.

What's the typo?

Why is "var" part of the variable name?

var (
	once                        sync.Once
	varDefaultCipherSuites      []uint16
	varDefaultCipherSuitesTLS13 []uint16
)

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Sep 2, 2020

Well, not if cipher suites aren't part of the application, but instead outsourced to OpenSSL. Then OpenSSL (ideally) maintains a good and secure cipher set. And application adminsitrators can update OpenSSL and change the applied ciphers via application configuration... no need to re-compile.

I was referring to any vulnerability in Go, which compiles to static binaries.

Why is "var" part of the variable name?

Not pretty, but it's a somewhat common pattern to hide the global var of a sync.Once based function (which is the symbol name without "var").

@FiloSottile FiloSottile removed this from the Backlog milestone Oct 5, 2020
@FiloSottile FiloSottile added this to the Go1.16 milestone Oct 5, 2020
@FiloSottile FiloSottile self-assigned this Oct 5, 2020
@Bsmalhi
Copy link

@Bsmalhi Bsmalhi commented Oct 27, 2020

@FiloSottile Any new updates to when you would expose a HTTP2 bool in CipherSuite? Appreciate any timelines.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Oct 29, 2020

@rolandshoemaker, can you mail a CL adding an HTTP2 bool to the CIpherSuite struct?

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 29, 2020

Change https://golang.org/cl/266297 mentions this issue: crypto/tls: add HTTP2 bool to CipherSuite struct

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Oct 29, 2020

Sorry to sound like am bikeshedding but I just added a review comment to the CL, and asked perhaps for UsableWithHTTP2 instead of just HTTP2, because that field is a read only property, and not a toggle to enable HTTP/2, hence let’s try to make it more descriptive.

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Nov 5, 2020

After talking through this with the security team we're going to take a different path than was initially suggested here. Since this metadata is not a property of cipher suites themselves, but rather a property of the HTTP/2 protocol (by which I mean certain cipher suites don't support HTTP/2, HTTP/2 supports certain cipher suites) we're going to opt to expose this in x/net/http2 instead.

x/net/http2 already has a private function, isBadCipher, which provides this functionality. Current proposal is just to make this function public. Given we've already hit the freeze, and this extends the API surface for a package which is vendored into the standard library, this is likely to get pushed to 1.17.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Dec 25, 2020

Thank you @rolandshoemaker for relaying back the new direction. Punting accordingly to Go1.17. Happy holidays!

@odeke-em odeke-em removed this from the Go1.16 milestone Dec 25, 2020
@odeke-em odeke-em added this to the Go1.17 milestone Dec 25, 2020
@FiloSottile FiloSottile changed the title crypto/tls: no way to know what CipherSuites are HTTP/2 safe x/net/http2: expose which TLS cipher suites are HTTP/2 safe Mar 17, 2021
@FiloSottile FiloSottile removed their assignment Mar 17, 2021
@FiloSottile FiloSottile removed this from the Go1.17 milestone Mar 17, 2021
@FiloSottile FiloSottile added this to the Unreleased milestone Mar 17, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 5, 2021

Change https://golang.org/cl/307369 mentions this issue: http2: expose method for checking blacklisted ciphers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants