diff --git a/lib/proxy/http.go b/lib/proxy/http.go index 647a3c5a1..f4c165967 100644 --- a/lib/proxy/http.go +++ b/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 } diff --git a/lib/proxy/http_test.go b/lib/proxy/http_test.go new file mode 100644 index 000000000..495a86af3 --- /dev/null +++ b/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") +}