From 5c379a437e904eb1f94296fa9d45276cd6e4f5a9 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Tue, 13 Aug 2019 16:29:01 -0400 Subject: [PATCH] [release-branch.go1.13] net/http: update bundled golang.org/x/net/http2 to import security fix Update golang.org/x/net to v0.0.0-20190813141303-74dc4d7220e7 to import the following security fix. commit 74dc4d7220e7acc4e100824340f3e66577424772 Author: Filippo Valsorda Date: Sun Aug 11 02:12:18 2019 -0400 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 This change was generated with cmd/go and cmd/bundle: $ go get -u golang.org/x/net $ go mod tidy $ go mod vendor $ go generate net/http Fixes CVE-2019-9512 and CVE-2019-9514 Fixes #33606 Change-Id: I464baf96175006aa101d65d3b0f6494f28a626ab Reviewed-on: https://go-review.googlesource.com/c/go/+/190137 Reviewed-by: Dmitri Shuralyov (cherry picked from commit 145e193131eb486077b66009beb051aba07c52a5) Reviewed-on: https://go-review.googlesource.com/c/go/+/191618 Run-TryBot: Filippo Valsorda TryBot-Result: Gobot Gobot --- src/go.mod | 2 +- src/go.sum | 4 +- src/net/http/h2_bundle.go | 56 +++++++++++++++---- .../x/net/lif/zsys_solaris_amd64.go | 2 +- .../golang.org/x/net/route/zsys_darwin.go | 2 +- .../golang.org/x/net/route/zsys_dragonfly.go | 2 +- .../x/net/route/zsys_freebsd_386.go | 2 +- .../x/net/route/zsys_freebsd_amd64.go | 2 +- .../x/net/route/zsys_freebsd_arm.go | 2 +- .../golang.org/x/net/route/zsys_netbsd.go | 2 +- .../golang.org/x/net/route/zsys_openbsd.go | 2 +- src/vendor/modules.txt | 2 +- 12 files changed, 58 insertions(+), 22 deletions(-) diff --git a/src/go.mod b/src/go.mod index 0d7d70f0141a7..90af2a7ea0f80 100644 --- a/src/go.mod +++ b/src/go.mod @@ -4,7 +4,7 @@ go 1.12 require ( golang.org/x/crypto v0.0.0-20190611184440-5c40567a22f8 - golang.org/x/net v0.0.0-20190607181551-461777fb6f67 + golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7 golang.org/x/sys v0.0.0-20190529130038-5219a1e1c5f8 // indirect golang.org/x/text v0.3.2 // indirect ) diff --git a/src/go.sum b/src/go.sum index 363ee7ae23ea6..e358118e4cbd6 100644 --- a/src/go.sum +++ b/src/go.sum @@ -2,8 +2,8 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk golang.org/x/crypto v0.0.0-20190611184440-5c40567a22f8 h1:1wopBVtVdWnn03fZelqdXTqk7U7zPQCb+T4rbU9ZEoU= golang.org/x/crypto v0.0.0-20190611184440-5c40567a22f8/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= -golang.org/x/net v0.0.0-20190607181551-461777fb6f67 h1:rJJxsykSlULwd2P2+pg/rtnwN2FrWp4IuCxOSyS0V00= -golang.org/x/net v0.0.0-20190607181551-461777fb6f67/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7 h1:fHDIZ2oxGnUZRN6WgWFCbYBjH9uqVPRCUVUDhs0wnbA= +golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20190529130038-5219a1e1c5f8 h1:2WjIC11WRITGlVWmyLXKjzIVj1ZwoWZ//tadeUUV6/o= diff --git a/src/net/http/h2_bundle.go b/src/net/http/h2_bundle.go index 173622fc8b804..53cc5bd1b8cd4 100644 --- a/src/net/http/h2_bundle.go +++ b/src/net/http/h2_bundle.go @@ -3611,10 +3611,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 ( @@ -3722,6 +3723,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{} @@ -4065,6 +4075,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 @@ -4456,6 +4467,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. @@ -4657,6 +4676,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() @@ -4774,10 +4801,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. @@ -4805,6 +4830,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 } @@ -5097,6 +5125,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 @@ -7451,7 +7481,7 @@ func (cc *http2ClientConn) roundTrip(req *Request) (res *Response, gotErrAfterRe req.Method != "HEAD" { // Request gzip only, not deflate. Deflate is ambiguous and // not as universally supported anyway. - // See: http://www.gzip.org/zlib/zlib_faq.html#faq38 + // See: https://zlib.net/zlib_faq.html#faq39 // // Note that we don't request this for HEAD requests, // due to a bug in nginx: @@ -9445,7 +9475,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) } @@ -9489,6 +9519,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 { diff --git a/src/vendor/golang.org/x/net/lif/zsys_solaris_amd64.go b/src/vendor/golang.org/x/net/lif/zsys_solaris_amd64.go index b5e999bec3a81..d7a70d4ed94a5 100644 --- a/src/vendor/golang.org/x/net/lif/zsys_solaris_amd64.go +++ b/src/vendor/golang.org/x/net/lif/zsys_solaris_amd64.go @@ -1,4 +1,4 @@ -// Created by cgo -godefs - DO NOT EDIT +// Code generated by cmd/cgo -godefs; DO NOT EDIT. // cgo -godefs defs_solaris.go package lif diff --git a/src/vendor/golang.org/x/net/route/zsys_darwin.go b/src/vendor/golang.org/x/net/route/zsys_darwin.go index 4e2e1ab090ca4..19e4133f7d191 100644 --- a/src/vendor/golang.org/x/net/route/zsys_darwin.go +++ b/src/vendor/golang.org/x/net/route/zsys_darwin.go @@ -1,4 +1,4 @@ -// Created by cgo -godefs - DO NOT EDIT +// Code generated by cmd/cgo -godefs; DO NOT EDIT. // cgo -godefs defs_darwin.go package route diff --git a/src/vendor/golang.org/x/net/route/zsys_dragonfly.go b/src/vendor/golang.org/x/net/route/zsys_dragonfly.go index 719c88d11f89e..8ed2d4d550c15 100644 --- a/src/vendor/golang.org/x/net/route/zsys_dragonfly.go +++ b/src/vendor/golang.org/x/net/route/zsys_dragonfly.go @@ -1,4 +1,4 @@ -// Created by cgo -godefs - DO NOT EDIT +// Code generated by cmd/cgo -godefs; DO NOT EDIT. // cgo -godefs defs_dragonfly.go package route diff --git a/src/vendor/golang.org/x/net/route/zsys_freebsd_386.go b/src/vendor/golang.org/x/net/route/zsys_freebsd_386.go index b03bc01f6543e..f36aaadb59f71 100644 --- a/src/vendor/golang.org/x/net/route/zsys_freebsd_386.go +++ b/src/vendor/golang.org/x/net/route/zsys_freebsd_386.go @@ -1,4 +1,4 @@ -// Created by cgo -godefs - DO NOT EDIT +// Code generated by cmd/cgo -godefs; DO NOT EDIT. // cgo -godefs defs_freebsd.go package route diff --git a/src/vendor/golang.org/x/net/route/zsys_freebsd_amd64.go b/src/vendor/golang.org/x/net/route/zsys_freebsd_amd64.go index 0b675b3d3f9d6..4c639b82e4b3a 100644 --- a/src/vendor/golang.org/x/net/route/zsys_freebsd_amd64.go +++ b/src/vendor/golang.org/x/net/route/zsys_freebsd_amd64.go @@ -1,4 +1,4 @@ -// Created by cgo -godefs - DO NOT EDIT +// Code generated by cmd/cgo -godefs; DO NOT EDIT. // cgo -godefs defs_freebsd.go package route diff --git a/src/vendor/golang.org/x/net/route/zsys_freebsd_arm.go b/src/vendor/golang.org/x/net/route/zsys_freebsd_arm.go index 58f8ea16f2511..710c1472b6439 100644 --- a/src/vendor/golang.org/x/net/route/zsys_freebsd_arm.go +++ b/src/vendor/golang.org/x/net/route/zsys_freebsd_arm.go @@ -1,4 +1,4 @@ -// Created by cgo -godefs - DO NOT EDIT +// Code generated by cmd/cgo -godefs; DO NOT EDIT. // cgo -godefs defs_freebsd.go package route diff --git a/src/vendor/golang.org/x/net/route/zsys_netbsd.go b/src/vendor/golang.org/x/net/route/zsys_netbsd.go index e0df45e8b5538..b4f66ca6cbc81 100644 --- a/src/vendor/golang.org/x/net/route/zsys_netbsd.go +++ b/src/vendor/golang.org/x/net/route/zsys_netbsd.go @@ -1,4 +1,4 @@ -// Created by cgo -godefs - DO NOT EDIT +// Code generated by cmd/cgo -godefs; DO NOT EDIT. // cgo -godefs defs_netbsd.go package route diff --git a/src/vendor/golang.org/x/net/route/zsys_openbsd.go b/src/vendor/golang.org/x/net/route/zsys_openbsd.go index db8c8efb49b29..1021b4cea4f3d 100644 --- a/src/vendor/golang.org/x/net/route/zsys_openbsd.go +++ b/src/vendor/golang.org/x/net/route/zsys_openbsd.go @@ -1,4 +1,4 @@ -// Created by cgo -godefs - DO NOT EDIT +// Code generated by cmd/cgo -godefs; DO NOT EDIT. // cgo -godefs defs_openbsd.go package route diff --git a/src/vendor/modules.txt b/src/vendor/modules.txt index 20f261bf836b9..453a3126613e7 100644 --- a/src/vendor/modules.txt +++ b/src/vendor/modules.txt @@ -7,7 +7,7 @@ golang.org/x/crypto/hkdf golang.org/x/crypto/internal/chacha20 golang.org/x/crypto/internal/subtle golang.org/x/crypto/poly1305 -# golang.org/x/net v0.0.0-20190607181551-461777fb6f67 +# golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7 golang.org/x/net/dns/dnsmessage golang.org/x/net/http/httpguts golang.org/x/net/http/httpproxy