lib/proxy: fix unbounded HTTP CONNECT headers causing OOM GHSA-xhf4-832v-7xcr CVE-PENDING

Before this change rclone read a proxy response with http.ReadResponse
over an unrestricted buffered reader. A malicious or compromised
configured proxy, or an active on-path actor controlling a plaintext
HTTP-proxy hop, can grow memory until the process fails.

This fixes the problem by restrincting the read to 1MB maximum.

lib/proxy/http.go

@@ -5,6 +5,7 @@ import (
 	"crypto/tls"
 	"encoding/base64"
 	"fmt"
+	"io"
 	"net"
 	"net/http"
 	"net/url"
@@ -13,6 +14,26 @@ import (
 	"golang.org/x/net/proxy"
 )
 
+// maxResponseBytes is the maximum size of CONNECT response we will
+// read from the proxy so a malicious proxy can't use all our memory.
+const maxResponseBytes = 1024 * 1024
+
+// bufferedConn is a net.Conn which reads from buffered first then Conn
+type bufferedConn struct {
+	net.Conn
+	buffered []byte // unread bytes received after the CONNECT response
+}
+
+// Read from buffered first then the underlying Conn
+func (c *bufferedConn) Read(p []byte) (n int, err error) {
+	if len(c.buffered) > 0 {
+		n = copy(p, c.buffered)
+		c.buffered = c.buffered[n:]
+		return n, nil
+	}
+	return c.Conn.Read(p)
+}
+
 // HTTPConnectDial connects using HTTP CONNECT via proxyDialer
 //
 // It will read the HTTP proxy address from the environment in the
@@ -67,16 +88,31 @@ func HTTPConnectDial(network, addr string, proxyURL *url.URL, proxyDialer proxy.
 		_ = conn.Close()
 		return nil, fmt.Errorf("HTTP CONNECT proxy failed to send CONNECT: %q", err)
 	}
-	br := bufio.NewReader(conn)
+	limitedConn := &io.LimitedReader{R: conn, N: maxResponseBytes}
+	br := bufio.NewReader(limitedConn)
 	req := &http.Request{URL: &url.URL{Scheme: "http", Host: addr}}
 	resp, err := http.ReadResponse(br, req)
 	if err != nil {
 		_ = conn.Close()
+		if limitedConn.N <= 0 {
+			return nil, fmt.Errorf("HTTP CONNECT proxy response too large (more than %d bytes)", maxResponseBytes)
+		}
 		return nil, fmt.Errorf("HTTP CONNECT proxy failed to read response: %q", err)
 	}
 	if resp.StatusCode != http.StatusOK {
 		_ = conn.Close()
 		return nil, fmt.Errorf("HTTP CONNECT proxy failed: %s", resp.Status)
 	}
+	// The server may have sent bytes for the tunnelled protocol (eg an
+	// SSH banner or FTP greeting) which br has buffered along with the
+	// CONNECT response - make sure they aren't lost.
+	if n := br.Buffered(); n > 0 {
+		buffered := make([]byte, n)
+		if _, err := io.ReadFull(br, buffered); err != nil {
+			_ = conn.Close()
+			return nil, fmt.Errorf("HTTP CONNECT proxy failed to read buffered bytes: %q", err)
+		}
+		conn = &bufferedConn{Conn: conn, buffered: buffered}
+	}
 	return conn, nil
 }

lib/proxy/http_test.go

@@ -0,0 +1,120 @@
+package proxy
+
+import (
+	"bufio"
+	"net"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// startProxy starts a fake HTTP CONNECT proxy which reads the CONNECT
+// request then calls serve with the connection to send the response.
+func startProxy(t *testing.T, serve func(conn net.Conn)) *url.URL {
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = listener.Close()
+	})
+	go func() {
+		conn, err := listener.Accept()
+		if err != nil {
+			return
+		}
+		defer func() {
+			_ = conn.Close()
+		}()
+		// Read the CONNECT request up to the blank line
+		br := bufio.NewReader(conn)
+		for {
+			line, err := br.ReadString('\n')
+			if err != nil || line == "\r\n" || line == "\n" {
+				break
+			}
+		}
+		serve(conn)
+	}()
+	proxyURL, err := url.Parse("http://" + listener.Addr().String())
+	require.NoError(t, err)
+	return proxyURL
+}
+
+func TestHTTPConnectDial(t *testing.T) {
+	proxyURL := startProxy(t, func(conn net.Conn) {
+		if _, err := conn.Write([]byte("HTTP/1.1 200 Connection established\r\n\r\n")); err != nil {
+			return
+		}
+		// echo the tunnelled data back
+		buf := make([]byte, 4)
+		if _, err := conn.Read(buf); err != nil {
+			return
+		}
+		_, _ = conn.Write(buf)
+	})
+	conn, err := HTTPConnectDial("tcp", "example.com:1234", proxyURL, nil)
+	require.NoError(t, err)
+	defer func() {
+		_ = conn.Close()
+	}()
+	_, err = conn.Write([]byte("ping"))
+	require.NoError(t, err)
+	buf := make([]byte, 4)
+	_, err = conn.Read(buf)
+	require.NoError(t, err)
+	assert.Equal(t, "ping", string(buf))
+}
+
+// Check that tunnel bytes the server sends immediately after the
+// CONNECT response (eg an SSH banner) are not lost.
+func TestHTTPConnectDialBuffered(t *testing.T) {
+	proxyURL := startProxy(t, func(conn net.Conn) {
+		// Send the response and the start of the tunnelled protocol in
+		// a single write so they arrive in one read.
+		_, _ = conn.Write([]byte("HTTP/1.1 200 Connection established\r\n\r\nSSH-2.0-banner\r\n"))
+	})
+	conn, err := HTTPConnectDial("tcp", "example.com:1234", proxyURL, nil)
+	require.NoError(t, err)
+	defer func() {
+		_ = conn.Close()
+	}()
+	buf := make([]byte, 16)
+	n, err := conn.Read(buf)
+	require.NoError(t, err)
+	assert.Equal(t, "SSH-2.0-banner\r\n", string(buf[:n]))
+}
+
+// Check that a proxy sending an arbitrarily large response can't use
+// unbounded memory.
+func TestHTTPConnectDialTooLarge(t *testing.T) {
+	proxyURL := startProxy(t, func(conn net.Conn) {
+		_, err := conn.Write([]byte("HTTP/1.1 200 Connection established\r\nX-Fill: "))
+		if err != nil {
+			return
+		}
+		// Stream more header than maxResponseBytes - writes will error
+		// once the client gives up and closes the connection.
+		chunk := []byte(strings.Repeat("x", 64*1024))
+		for written := 0; written <= 2<<20; written += len(chunk) {
+			if _, err := conn.Write(chunk); err != nil {
+				return
+			}
+		}
+	})
+	conn, err := HTTPConnectDial("tcp", "example.com:1234", proxyURL, nil)
+	require.Error(t, err)
+	assert.Nil(t, conn)
+	assert.Contains(t, err.Error(), "too large")
+}
+
+func TestHTTPConnectDialNon200(t *testing.T) {
+	proxyURL := startProxy(t, func(conn net.Conn) {
+		_, _ = conn.Write([]byte("HTTP/1.1 403 Forbidden\r\nContent-Length: 0\r\n\r\n"))
+	})
+	conn, err := HTTPConnectDial("tcp", "example.com:1234", proxyURL, nil)
+	require.Error(t, err)
+	assert.Nil(t, conn)
+	assert.Contains(t, err.Error(), "403")
+}