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: The server should not select a banned cipher if it selects h2 #12895

Closed
jay opened this issue Oct 9, 2015 · 3 comments
Closed

x/net/http2: The server should not select a banned cipher if it selects h2 #12895

jay opened this issue Oct 9, 2015 · 3 comments
Assignees

Comments

@jay
Copy link

@jay jay commented Oct 9, 2015

An HTTP/2 capable server that enforces the optional TLS 1.2 cipher suite blacklist from RFC 7540 should not select one of those ciphers if the protocol selection is h2. A client may advertise both h2 and blacklisted ciphers because it doesn't know anything about the server.

For example consider a client such as libcurl that uses nghttp2 and OpenSSL 1.0.2. If HTTP2 is enabled it will send a ClientHello that advertises protocols h2, http/1.1 and ciphers such as [... TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA ...]. The go server currently replies with a ServerHello selecting h2 and the aforementioned blacklisted cipher, which it later bans by default.

TLSv1.2 Record Layer: Handshake Protocol: Server Hello
    Content Type: Handshake (22)
    Version: TLS 1.2 (0x0303)
    Length: 58
    Handshake Protocol: Server Hello
        Handshake Type: Server Hello (2)
        Length: 54
        Version: TLS 1.2 (0x0303)
        Random
            GMT Unix Time: Jan 21, 2084 01:52:46.000000000 Eastern Standard Time
            Random Bytes: 9dddda38a42111e70f3b934867a054c7a17df8aa27ef5139...
        Session ID Length: 0
        Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)
        Compression Method: null (0)
        Extensions Length: 14
        Extension: renegotiation_info
            Type: renegotiation_info (0xff01)
            Length: 1
            Renegotiation Info extension
                Renegotiation info extension length: 0
        Extension: Application Layer Protocol Negotiation
            Type: Application Layer Protocol Negotiation (0x0010)
            Length: 5
            ALPN Extension Length: 3
            ALPN Protocol
                ALPN string length: 2
                ALPN Next Protocol: h2

Ref curl/curl#406

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 9, 2015

/cc @agl, as this might require more cooperation between the http2 and crypto/tls packages.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Oct 10, 2015

The intended configuration for this is that the server sets PreferServerCipherSuites in the tls.Config and moves the HTTP/2 compatible cipher suites to the top of CipherSuites

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 15, 2015

CL https://golang.org/cl/15860 mentions this issue.

@bradfitz bradfitz self-assigned this Oct 15, 2015
bradfitz added a commit that referenced this issue Oct 20, 2015
… error

In https://golang.org/cl/15860 http2.ConfigureServer was changed to
return an error if explicit CipherSuites are listed and they're not
compliant with the HTTP/2 spec.

This is the net/http side of the change, to look at the return value
from ConfigureServer and propagate it in Server.Serve.

h2_bundle.go will be updated in a future CL. There are too many other
http2 changes pending to be worth updating it now. Instead,
h2_bundle.go is minimally updated by hand in this CL so at least the
net/http change will compile.

Updates #12895

Change-Id: I4df7a097faff2d235742c2d310c333bd3fd5c08e
Reviewed-on: https://go-review.googlesource.com/16065
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Oct 24, 2016
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
…rrors

Fixes golang/go#12895

Change-Id: I7cf6e63b1bbdf1f4e8974c00bdaed69b74f6db49
Reviewed-on: https://go-review.googlesource.com/15860
Reviewed-by: Adam Langley <agl@golang.org>
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.