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>

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) {

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 {