From e152b01a468a1c18a290bf9aec52ccea7693c7f2 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 12 Aug 2019 16:59:30 -0400 Subject: [release-branch.go1.11-security] net/http: update bundled http2 to import security fix Apply the following unpublished golang.org/x/net commit. commit b1cc14aba47abf96f96818003fa4caad3a4b4e86 Author: Filippo Valsorda Date: Sun Aug 11 02:12:18 2019 -0400 [release-branch.go1.11] http2: limit number of control frames in server send queue An attacker could cause servers to queue an unlimited number of PING ACKs or RST_STREAM frames by soliciting them and not reading them, until the program runs out of memory. Limit control frames in the queue to a few thousands (matching the limit imposed by other vendors) by counting as they enter and exit the scheduler, so the protection will work with any WriteScheduler. Once the limit is exceeded, close the connection, as we have no way to communicate with the peer. Change-Id: I842968fc6ed3eac654b497ade8cea86f7267886b Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/525552 Reviewed-by: Brad Fitzpatrick (cherry picked from commit 589ad6cc5321fb68a90370348a241a5da0a2cc80) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/526070 Reviewed-by: Dmitri Shuralyov Fixes CVE-2019-9512 and CVE-2019-9514 Updates #33606 Change-Id: Iecedf1cc63ec7a1cd75661ec591d91ebc911cc64 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/526072 Reviewed-by: Dmitri Shuralyov --- src/net/http/h2_bundle.go | 54 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/src/net/http/h2_bundle.go b/src/net/http/h2_bundle.go index 2cd2b86df2..61824950f4 100644 --- a/src/net/http/h2_bundle.go +++ b/src/net/http/h2_bundle.go @@ -3835,10 +3835,11 @@ func (p *http2pipe) Done() <-chan struct{} { } const ( - http2prefaceTimeout = 10 * time.Second - http2firstSettingsTimeout = 2 * time.Second // should be in-flight with preface anyway - http2handlerChunkWriteSize = 4 << 10 - http2defaultMaxStreams = 250 // TODO: make this 100 as the GFE seems to? + http2prefaceTimeout = 10 * time.Second + http2firstSettingsTimeout = 2 * time.Second // should be in-flight with preface anyway + http2handlerChunkWriteSize = 4 << 10 + http2defaultMaxStreams = 250 // TODO: make this 100 as the GFE seems to? + http2maxQueuedControlFrames = 10000 ) var ( @@ -3946,6 +3947,15 @@ func (s *http2Server) maxConcurrentStreams() uint32 { return http2defaultMaxStreams } +// maxQueuedControlFrames is the maximum number of control frames like +// SETTINGS, PING and RST_STREAM that will be queued for writing before +// the connection is closed to prevent memory exhaustion attacks. +func (s *http2Server) maxQueuedControlFrames() int { + // TODO: if anybody asks, add a Server field, and remember to define the + // behavior of negative values. + return http2maxQueuedControlFrames +} + type http2serverInternalState struct { mu sync.Mutex activeConns map[*http2serverConn]struct{} @@ -4254,6 +4264,7 @@ type http2serverConn struct { sawFirstSettings bool // got the initial SETTINGS frame after the preface needToSendSettingsAck bool unackedSettings int // how many SETTINGS have we sent without ACKs? + queuedControlFrames int // control frames in the writeSched queue clientMaxStreams uint32 // SETTINGS_MAX_CONCURRENT_STREAMS from client (our PUSH_PROMISE limit) advMaxStreams uint32 // our SETTINGS_MAX_CONCURRENT_STREAMS advertised the client curClientStreams uint32 // number of open streams initiated by the client @@ -4644,6 +4655,14 @@ func (sc *http2serverConn) serve() { } } + // If the peer is causing us to generate a lot of control frames, + // but not reading them from us, assume they are trying to make us + // run out of memory. + if sc.queuedControlFrames > sc.srv.maxQueuedControlFrames() { + sc.vlogf("http2: too many control frames in send queue, closing connection") + return + } + // Start the shutdown timer after sending a GOAWAY. When sending GOAWAY // with no error code (graceful shutdown), don't start the timer until // all open streams have been completed. @@ -4845,6 +4864,14 @@ func (sc *http2serverConn) writeFrame(wr http2FrameWriteRequest) { } if !ignoreWrite { + if wr.isControl() { + sc.queuedControlFrames++ + // For extra safety, detect wraparounds, which should not happen, + // and pull the plug. + if sc.queuedControlFrames < 0 { + sc.conn.Close() + } + } sc.writeSched.Push(wr) } sc.scheduleFrameWrite() @@ -4962,10 +4989,8 @@ func (sc *http2serverConn) wroteFrame(res http2frameWriteResult) { // If a frame is already being written, nothing happens. This will be called again // when the frame is done being written. // -// If a frame isn't being written we need to send one, the best frame -// to send is selected, preferring first things that aren't -// stream-specific (e.g. ACKing settings), and then finding the -// highest priority stream. +// If a frame isn't being written and we need to send one, the best frame +// to send is selected by writeSched. // // If a frame isn't being written and there's nothing else to send, we // flush the write buffer. @@ -4993,6 +5018,9 @@ func (sc *http2serverConn) scheduleFrameWrite() { } if !sc.inGoAway || sc.goAwayCode == http2ErrCodeNo { if wr, ok := sc.writeSched.Pop(); ok { + if wr.isControl() { + sc.queuedControlFrames-- + } sc.startFrameWrite(wr) continue } @@ -5285,6 +5313,8 @@ func (sc *http2serverConn) processSettings(f *http2SettingsFrame) error { if err := f.ForeachSetting(sc.processSetting); err != nil { return err } + // TODO: judging by RFC 7540, Section 6.5.3 each SETTINGS frame should be + // acknowledged individually, even if multiple are received before the ACK. sc.needToSendSettingsAck = true sc.scheduleFrameWrite() return nil @@ -9476,7 +9506,7 @@ type http2WriteScheduler interface { // Pop dequeues the next frame to write. Returns false if no frames can // be written. Frames with a given wr.StreamID() are Pop'd in the same - // order they are Push'd. + // order they are Push'd. No frames should be discarded except by CloseStream. Pop() (wr http2FrameWriteRequest, ok bool) } @@ -9520,6 +9550,12 @@ func (wr http2FrameWriteRequest) StreamID() uint32 { return wr.stream.id } +// isControl reports whether wr is a control frame for MaxQueuedControlFrames +// purposes. That includes non-stream frames and RST_STREAM frames. +func (wr http2FrameWriteRequest) isControl() bool { + return wr.stream == nil +} + // DataSize returns the number of flow control bytes that must be consumed // to write this entire frame. This is 0 for non-DATA frames. func (wr http2FrameWriteRequest) DataSize() int { -- cgit v1.2.3-54-g00ecf From c1d9ca70995dc232a2145e3214f94e03409f6fcc Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Tue, 6 Aug 2019 19:32:16 -0400 Subject: [release-branch.go1.11-security] net/url: make Hostname and Port predictable for invalid Host values When Host is not valid per RFC 3986, the behavior of Hostname and Port was wildly unpredictable, to the point that Host could have a suffix that didn't appear in neither Hostname nor Port. This is a security issue when applications are applying checks to Host and expecting them to be meaningful for the contents of Hostname. To reduce disruption, this change only aims to guarantee the following two security-relevant invariants. * Host is either Hostname or [Hostname] with Port empty, or Hostname:Port or [Hostname]:Port. * Port is only decimals. The second invariant is the one that's most likely to cause disruption, but I believe it's important, as it's conceivable an application might do a suffix check on Host and expect it to be meaningful for the contents of Hostname (if the suffix is not a valid port). There are three ways to ensure it. 1) Reject invalid ports in Parse. Note that non-numeric ports are already rejected if and only if the host starts with "[". 2) Consider non-numeric ports as part of Hostname, not Port. 3) Allow non-numeric ports, and hope they only flow down to net/http, which will reject them (#14353). This change adopts both 1 and 2. We could do only the latter, but then these invalid hosts would flow past port checks, like in http_test.TestTransportRejectsAlphaPort. Non-numeric ports weren't fully supported anyway, because they were rejected after IPv6 literals, so this restores consistency. We could do only the former, but at this point 2) is free and might help with manually constructed Host values (or if we get something wrong in Parse). Note that net.SplitHostPort and net.Dial explicitly accept service names in place of port numbers, but this is an URL package, and RFC 3986, Section 3.2.3, clearly specifies ports as a number in decimal. net/http uses a mix of net.SplitHostPort and url.Parse that would deserve looking into, but in general it seems that it will still accept service names in Addr fields as they are passed to net.Listen, while rejecting them in URLs, which feels correct. This leaves a number of invalid URLs to reject, which however are not security relevant once the two invariants above hold, so can be done in Go 1.14: IPv6 literals without brackets (#31024), invalid IPv6 literals, hostnames with invalid characters, and more. Tested with 200M executions of go-fuzz and the following Fuzz function. u, err := url.Parse(string(data)) if err != nil { return 0 } h := u.Hostname() p := u.Port() switch u.Host { case h + ":" + p: return 1 case "[" + h + "]:" + p: return 1 case h: fallthrough case "[" + h + "]": if p != "" { panic("unexpected Port()") } return 1 } panic("Host is not a variant of [Hostname]:Port") Fixes CVE-2019-14809 Updates #29098 Change-Id: I7ef40823dab28f29511329fa2d5a7fb10c3ec895 Reviewed-on: https://go-review.googlesource.com/c/go/+/189258 Reviewed-by: Ian Lance Taylor (cherry picked from commit 61bb56ad63992a3199acc55b2537c8355ef887b6) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/526408 Reviewed-by: Dmitri Shuralyov (cherry picked from commit 3226f2d492963d361af9dfc6714ef141ba606713) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/526409 --- src/net/http/transport.go | 2 ++ src/net/http/transport_test.go | 2 +- src/net/url/url.go | 54 +++++++++++++++--------------- src/net/url/url_test.go | 76 ++++++++++++++++++++++-------------------- 4 files changed, 69 insertions(+), 65 deletions(-) diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 40947baf87..0a9812eeda 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -640,6 +640,8 @@ func resetProxyConfig() { } func (t *Transport) connectMethodForRequest(treq *transportRequest) (cm connectMethod, err error) { + // TODO: the validPort check is redundant after CL 189258, as url.URL.Port + // only returns valid ports now. golang.org/issue/33600 if port := treq.URL.Port(); !validPort(port) { return cm, fmt.Errorf("invalid URL port %q", port) } diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index aa8beb9357..b2036dfc24 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -4111,7 +4111,7 @@ func TestTransportRejectsAlphaPort(t *testing.T) { t.Fatalf("got %#v; want *url.Error", err) } got := ue.Err.Error() - want := `invalid URL port "123foo"` + want := `invalid port ":123foo" after host` if got != want { t.Errorf("got error %q; want %q", got, want) } diff --git a/src/net/url/url.go b/src/net/url/url.go index 8d2a856699..b13677ca8a 100644 --- a/src/net/url/url.go +++ b/src/net/url/url.go @@ -636,6 +636,11 @@ func parseHost(host string) (string, error) { } return host1 + host2 + host3, nil } + } else if i := strings.LastIndex(host, ":"); i != -1 { + colonPort := host[i:] + if !validOptionalPort(colonPort) { + return "", fmt.Errorf("invalid port %q after host", colonPort) + } } var err error @@ -1033,44 +1038,39 @@ func (u *URL) RequestURI() string { return result } -// Hostname returns u.Host, without any port number. +// Hostname returns u.Host, stripping any valid port number if present. // -// If Host is an IPv6 literal with a port number, Hostname returns the -// IPv6 literal without the square brackets. IPv6 literals may include -// a zone identifier. +// If the result is enclosed in square brackets, as literal IPv6 addresses are, +// the square brackets are removed from the result. func (u *URL) Hostname() string { - return stripPort(u.Host) + host, _ := splitHostPort(u.Host) + return host } // Port returns the port part of u.Host, without the leading colon. -// If u.Host doesn't contain a port, Port returns an empty string. +// +// If u.Host doesn't contain a valid numeric port, Port returns an empty string. func (u *URL) Port() string { - return portOnly(u.Host) + _, port := splitHostPort(u.Host) + return port } -func stripPort(hostport string) string { - colon := strings.IndexByte(hostport, ':') - if colon == -1 { - return hostport - } - if i := strings.IndexByte(hostport, ']'); i != -1 { - return strings.TrimPrefix(hostport[:i], "[") - } - return hostport[:colon] -} +// splitHostPort separates host and port. If the port is not valid, it returns +// the entire input as host, and it doesn't check the validity of the host. +// Unlike net.SplitHostPort, but per RFC 3986, it requires ports to be numeric. +func splitHostPort(hostport string) (host, port string) { + host = hostport -func portOnly(hostport string) string { - colon := strings.IndexByte(hostport, ':') - if colon == -1 { - return "" - } - if i := strings.Index(hostport, "]:"); i != -1 { - return hostport[i+len("]:"):] + colon := strings.LastIndexByte(host, ':') + if colon != -1 && validOptionalPort(host[colon:]) { + host, port = host[:colon], host[colon+1:] } - if strings.Contains(hostport, "]") { - return "" + + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { + host = host[1 : len(host)-1] } - return hostport[colon+len(":"):] + + return } // Marshaling interface implementations. diff --git a/src/net/url/url_test.go b/src/net/url/url_test.go index 369ea6cbd2..babdaec0af 100644 --- a/src/net/url/url_test.go +++ b/src/net/url/url_test.go @@ -422,10 +422,10 @@ var urltests = []URLTest{ }, // worst case host, still round trips { - "scheme://!$&'()*+,;=hello!:port/path", + "scheme://!$&'()*+,;=hello!:1/path", &URL{ Scheme: "scheme", - Host: "!$&'()*+,;=hello!:port", + Host: "!$&'()*+,;=hello!:1", Path: "/path", }, "", @@ -1420,11 +1420,13 @@ func TestParseErrors(t *testing.T) { {"http://[::1]", false}, {"http://[::1]:80", false}, {"http://[::1]:namedport", true}, // rfc3986 3.2.3 + {"http://x:namedport", true}, // rfc3986 3.2.3 {"http://[::1]/", false}, {"http://[::1]a", true}, {"http://[::1]%23", true}, {"http://[::1%25en0]", false}, // valid zone id {"http://[::1]:", false}, // colon, but no port OK + {"http://x:", false}, // colon, but no port OK {"http://[::1]:%38%30", true}, // not allowed: % encoding only for non-ASCII {"http://[::1%25%41]", false}, // RFC 6874 allows over-escaping in zone {"http://[%10::1]", true}, // no %xx escapes in IP address @@ -1616,46 +1618,46 @@ func TestURLErrorImplementsNetError(t *testing.T) { } } -func TestURLHostname(t *testing.T) { +func TestURLHostnameAndPort(t *testing.T) { tests := []struct { - host string // URL.Host field - want string + in string // URL.Host field + host string + port string }{ - {"foo.com:80", "foo.com"}, - {"foo.com", "foo.com"}, - {"FOO.COM", "FOO.COM"}, // no canonicalization (yet?) - {"1.2.3.4", "1.2.3.4"}, - {"1.2.3.4:80", "1.2.3.4"}, - {"[1:2:3:4]", "1:2:3:4"}, - {"[1:2:3:4]:80", "1:2:3:4"}, - {"[::1]:80", "::1"}, + {"foo.com:80", "foo.com", "80"}, + {"foo.com", "foo.com", ""}, + {"foo.com:", "foo.com", ""}, + {"FOO.COM", "FOO.COM", ""}, // no canonicalization + {"1.2.3.4", "1.2.3.4", ""}, + {"1.2.3.4:80", "1.2.3.4", "80"}, + {"[1:2:3:4]", "1:2:3:4", ""}, + {"[1:2:3:4]:80", "1:2:3:4", "80"}, + {"[::1]:80", "::1", "80"}, + {"[::1]", "::1", ""}, + {"[::1]:", "::1", ""}, + {"localhost", "localhost", ""}, + {"localhost:443", "localhost", "443"}, + {"some.super.long.domain.example.org:8080", "some.super.long.domain.example.org", "8080"}, + {"[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:17000", "2001:0db8:85a3:0000:0000:8a2e:0370:7334", "17000"}, + {"[2001:0db8:85a3:0000:0000:8a2e:0370:7334]", "2001:0db8:85a3:0000:0000:8a2e:0370:7334", ""}, + + // Ensure that even when not valid, Host is one of "Hostname", + // "Hostname:Port", "[Hostname]" or "[Hostname]:Port". + // See https://golang.org/issue/29098. + {"[google.com]:80", "google.com", "80"}, + {"google.com]:80", "google.com]", "80"}, + {"google.com:80_invalid_port", "google.com:80_invalid_port", ""}, + {"[::1]extra]:80", "::1]extra", "80"}, + {"google.com]extra:extra", "google.com]extra:extra", ""}, } for _, tt := range tests { - u := &URL{Host: tt.host} - got := u.Hostname() - if got != tt.want { - t.Errorf("Hostname for Host %q = %q; want %q", tt.host, got, tt.want) + u := &URL{Host: tt.in} + host, port := u.Hostname(), u.Port() + if host != tt.host { + t.Errorf("Hostname for Host %q = %q; want %q", tt.in, host, tt.host) } - } -} - -func TestURLPort(t *testing.T) { - tests := []struct { - host string // URL.Host field - want string - }{ - {"foo.com", ""}, - {"foo.com:80", "80"}, - {"1.2.3.4", ""}, - {"1.2.3.4:80", "80"}, - {"[1:2:3:4]", ""}, - {"[1:2:3:4]:80", "80"}, - } - for _, tt := range tests { - u := &URL{Host: tt.host} - got := u.Port() - if got != tt.want { - t.Errorf("Port for Host %q = %q; want %q", tt.host, got, tt.want) + if port != tt.port { + t.Errorf("Port for Host %q = %q; want %q", tt.in, port, tt.port) } } } -- cgit v1.2.3-54-g00ecf From df86e1945e96829e62b77518d03e0fc726d2d48f Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Tue, 13 Aug 2019 10:27:29 -0400 Subject: [release-branch.go1.11-security] doc: document Go 1.11.13 Change-Id: I0daab6cd347e1fc0066e516f02c33f1b63e3f1a3 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/526992 Reviewed-by: Filippo Valsorda (cherry picked from commit 685bfb1adec3d9fcb589f35eb2bc0b99d2f84bf0) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/526994 --- doc/devel/release.html | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/doc/devel/release.html b/doc/devel/release.html index 69a5741cbb..c24de8e344 100644 --- a/doc/devel/release.html +++ b/doc/devel/release.html @@ -120,6 +120,13 @@ See the Go 1.11.12 milestone on our issue tracker for details.

+

+go1.11.13 (released 2019/08/13) includes security fixes to the +net/http and net/url packages. +See the Go +1.11.13 milestone on our issue tracker for details. +

+

go1.10 (released 2018/02/16)

-- cgit v1.2.3-54-g00ecf From b2967c0e5c5271bb4469e1f615fb85879ebd8a57 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Tue, 13 Aug 2019 11:23:32 -0400 Subject: [release-branch.go1.11-security] go1.11.13 Change-Id: Idf5f9d00388b0da77f2c2ce3650eb65271bd9e68 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/526996 Reviewed-by: Filippo Valsorda --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index f7231621df..4776251005 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.11.12 \ No newline at end of file +go1.11.13 \ No newline at end of file -- cgit v1.2.3-54-g00ecf