net/http: Ensure that CONNECT proxied requests respect MaxResponseHeaderBytes

Currently, CONNECT proxied requests use an unlimited Reader. As a
result, a malicious or misbehaving proxy server can send an unlimited
number of bytes to a client; causing the client to indefinitely receive bytes
until it runs out of memory.

To prevent this, we now use a LimitedReader that limits the number of
bytes according to MaxResponseHeaderBytes in Transport. If
MaxResponseHeaderBytes is not provided, we use the default value of 10
MB that has historically been used (see #26315).

Fixes #74633

Change-Id: I0b03bb354139dbc64318874402f7f29cc0fb42ce
Reviewed-on: https://go-review.googlesource.com/c/go/+/698915
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index b860eb95b043da5a14a869aaf9efab30bc074dff..572c16a6d838d828b3809a683a0864e59659df97 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -325,6 +325,13 @@ func (t *Transport) readBufferSize() int {
        return 4 << 10
 }
 
+func (t *Transport) maxHeaderResponseSize() int64 {
+       if t.MaxResponseHeaderBytes > 0 {
+               return t.MaxResponseHeaderBytes
+       }
+       return 10 << 20 // conservative default; same as http2
+}
+
 // Clone returns a deep copy of t's exported fields.
 func (t *Transport) Clone() *Transport {
        t.nextProtoOnce.Do(t.onceSetNextProtoDefaults)
@@ -1871,7 +1878,7 @@ func (t *Transport) dialConn(ctx context.Context, cm connectMethod) (pconn *pers
                        }
                        // Okay to use and discard buffered reader here, because
                        // TLS server will not speak until spoken to.
-                       br := bufio.NewReader(conn)
+                       br := bufio.NewReader(&io.LimitedReader{R: conn, N: t.maxHeaderResponseSize()})
                        resp, err = ReadResponse(br, connectReq)
                }()
                select {
@@ -2108,10 +2115,7 @@ type persistConn struct {
 }
 
 func (pc *persistConn) maxHeaderResponseSize() int64 {
-       if v := pc.t.MaxResponseHeaderBytes; v != 0 {
-               return v
-       }
-       return 10 << 20 // conservative default; same as http2
+       return pc.t.maxHeaderResponseSize()
 }
 
 func (pc *persistConn) Read(p []byte) (n int, err error) {

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 9762f058867dc17a6ece731a6d7f913af3acf98b..810f21f3a517e6a1f083fdc8f2a3b9436cf02c92 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -1550,6 +1550,72 @@ func TestTransportProxy(t *testing.T) {
        }
 }
 
+// Issue 74633: verify that a client will not indefinitely read a response from
+// a proxy server that writes an infinite byte of stream, rather than
+// responding with 200 OK.
+func TestProxyWithInfiniteHeader(t *testing.T) {
+       defer afterTest(t)
+
+       ln := newLocalListener(t)
+       defer ln.Close()
+       cancelc := make(chan struct{})
+       defer close(cancelc)
+
+       // Simulate a malicious / misbehaving proxy that writes an unlimited number
+       // of bytes rather than responding with 200 OK.
+       go func() {
+               c, err := ln.Accept()
+               if err != nil {
+                       t.Errorf("Accept: %v", err)
+                       return
+               }
+               defer c.Close()
+               // Read the CONNECT request
+               br := bufio.NewReader(c)
+               cr, err := ReadRequest(br)
+               if err != nil {
+                       t.Errorf("proxy server failed to read CONNECT request")
+                       return
+               }
+               if cr.Method != "CONNECT" {
+                       t.Errorf("unexpected method %q", cr.Method)
+                       return
+               }
+
+               // Keep writing bytes until the test exits.
+               for {
+                       // runtime.Gosched() is needed here. Otherwise, this test might
+                       // livelock in environments like WASM, where the one single thread
+                       // we have could be hogged by the infinite loop of writing bytes.
+                       runtime.Gosched()
+                       select {
+                       case <-cancelc:
+                               return
+                       default:
+                               c.Write([]byte("infinite stream of bytes"))
+                       }
+               }
+       }()
+
+       c := &Client{
+               Transport: &Transport{
+                       Proxy: func(*Request) (*url.URL, error) {
+                               return url.Parse("http://" + ln.Addr().String())
+                       },
+                       // Limit MaxResponseHeaderBytes so the test returns quicker.
+                       MaxResponseHeaderBytes: 1024,
+               },
+       }
+       req, err := NewRequest("GET", "https://golang.fake.tld/", nil)
+       if err != nil {
+               t.Fatal(err)
+       }
+       _, err = c.Do(req)
+       if err == nil {
+               t.Errorf("unexpected Get success")
+       }
+}
+
 func TestOnProxyConnectResponse(t *testing.T) {
 
        var tcases = []struct {