fix(vhost): avoid request body access after CONNECT hijack (#5562)

pkg/util/vhost/http.go

@@ -206,12 +206,28 @@ func checkRouteAuthByRequest(req *http.Request, rc *RouteConfig) bool {
 }
 
 func (rp *HTTPReverseProxy) connectHandler(rw http.ResponseWriter, req *http.Request) {
+	if req.ContentLength > 0 || len(req.TransferEncoding) > 0 {
+		http.Error(rw, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
+		return
+	}
+
 	hj, ok := rw.(http.Hijacker)
 	if !ok {
 		rw.WriteHeader(http.StatusInternalServerError)
 		return
 	}
 
+	// Forward only CONNECT headers; the original Body must not be used after Hijack.
+	outreq := req.Clone(req.Context())
+	outreq.Body = nil
+	outreq.GetBody = nil
+	outreq.ContentLength = 0
+	outreq.TransferEncoding = nil
+	outreq.Trailer = nil
+	outreq.Header.Del("Content-Length")
+	outreq.Header.Del("Transfer-Encoding")
+	outreq.Header.Del("Trailer")
+
 	client, buffered, err := hj.Hijack()
 	if err != nil {
 		rw.WriteHeader(http.StatusInternalServerError)
@@ -224,7 +240,11 @@ func (rp *HTTPReverseProxy) connectHandler(rw http.ResponseWriter, req *http.Req
 		client.Close()
 		return
 	}
-	_ = req.Write(remote)
+	if err := outreq.Write(remote); err != nil {
+		_ = remote.Close()
+		_ = client.Close()
+		return
+	}
 	go libio.Join(remote, libio.WrapReadWriteCloser(buffered.Reader, client, client.Close))
 }
 

pkg/util/vhost/http_test.go

@@ -7,6 +7,8 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -17,8 +19,17 @@ import (
 )
 
 func TestHTTPConnectTunnel(t *testing.T) {
-	for _, pipelined := range []bool{false, true} {
+	for _, tc := range []struct {
-		t.Run(fmt.Sprintf("pipelined=%v", pipelined), func(t *testing.T) {
+		name      string
+		framing   string
+		pipelined bool
+	}{
+		{name: "plain"},
+		{name: "plain-early", pipelined: true},
+		{name: "cl0", framing: "Content-Length: 0\r\n"},
+		{name: "cl0-early", framing: "Content-Length: 0\r\n", pipelined: true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
 			remote, backend := net.Pipe()
 			defer remote.Close()
 			defer backend.Close()
@@ -27,6 +38,8 @@ func TestHTTPConnectTunnel(t *testing.T) {
 			rp := NewHTTPReverseProxy(HTTPReverseProxyOptions{}, NewRouters())
 			require.NoError(t, rp.Register(RouteConfig{
 				Domain:   "target.example.com",
+				Username: "alice",
+				Password: "secret",
 				CreateConnFn: func(string) (net.Conn, error) {
 					return remote, nil
 				},
@@ -48,8 +61,10 @@ func TestHTTPConnectTunnel(t *testing.T) {
 			require.NoError(t, client.SetDeadline(time.Now().Add(5*time.Second)))
 			require.NoError(t, listener.PutConn(incoming))
 
-			request := "CONNECT target.example.com:443 HTTP/1.1\r\nHost: target.example.com:443\r\n\r\n"
+			proxyAuth := httppkg.BasicAuth("alice", "secret")
-			if pipelined {
+			request := "CONNECT target.example.com:443 HTTP/1.1\r\nHost: target.example.com:443\r\n" +
+				"X-Connect-Test: tunnel\r\nProxy-Authorization: " + proxyAuth + "\r\n" + tc.framing + "\r\n"
+			if tc.pipelined {
 				// One pipe write puts the tunnel data in net/http's request buffer.
 				request += "early data"
 			}
@@ -61,7 +76,12 @@ func TestHTTPConnectTunnel(t *testing.T) {
 			defer req.Body.Close()
 			require.Equal(t, http.MethodConnect, req.Method)
 			require.Equal(t, "target.example.com:443", req.Host)
-			if pipelined {
+			require.Equal(t, "target.example.com:443", req.URL.Host)
+			require.Equal(t, "tunnel", req.Header.Get("X-Connect-Test"))
+			require.Equal(t, proxyAuth, req.Header.Get("Proxy-Authorization"))
+			require.Zero(t, req.ContentLength)
+			require.Empty(t, req.TransferEncoding)
+			if tc.pipelined {
 				data := make([]byte, len("early data"))
 				_, err = io.ReadFull(backendReader, data)
 				require.NoError(t, err)
@@ -94,6 +114,155 @@ func TestHTTPConnectTunnel(t *testing.T) {
 	}
 }
 
+func TestHTTPConnectRejectsContent(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		framing string
+		body    string
+	}{
+		{name: "content-length", framing: "Content-Length: 1\r\n", body: "X"},
+		{name: "chunked", framing: "Transfer-Encoding: chunked\r\n", body: "1\r\nX\r\n0\r\n\r\n"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			backendCalls := 0
+			rp := NewHTTPReverseProxy(HTTPReverseProxyOptions{}, NewRouters())
+			require.NoError(t, rp.Register(RouteConfig{
+				Domain: "target.example.com",
+				CreateConnFn: func(string) (net.Conn, error) {
+					backendCalls++
+					return nil, io.ErrClosedPipe
+				},
+			}))
+			req, err := http.ReadRequest(bufio.NewReader(strings.NewReader(
+				"CONNECT target.example.com:443 HTTP/1.1\r\nHost: target.example.com:443\r\n" + tc.framing + "\r\n" + tc.body)))
+			require.NoError(t, err)
+			defer req.Body.Close()
+			rw := &connectTestResponseWriter{ResponseRecorder: httptest.NewRecorder()}
+
+			rp.ServeHTTP(rw, req)
+
+			require.Equal(t, http.StatusBadRequest, rw.Code)
+			require.Zero(t, rw.hijackCalls, "reject content before Hijack")
+			require.Zero(t, backendCalls, "reject content before creating a backend")
+		})
+	}
+}
+
+func TestHTTPConnectHeaderWriteFailure(t *testing.T) {
+	for _, originalBody := range []bool{false, true} {
+		t.Run(fmt.Sprintf("originalBody=%v", originalBody), func(t *testing.T) {
+			client, incoming := net.Pipe()
+			defer client.Close()
+			defer incoming.Close()
+			remote, backend := net.Pipe()
+			defer remote.Close()
+			defer backend.Close()
+			clientConn := &connectTestConn{Conn: incoming}
+			remoteConn := &connectTestConn{Conn: remote, failWrites: true}
+			rp := NewHTTPReverseProxy(HTTPReverseProxyOptions{}, NewRouters())
+			require.NoError(t, rp.Register(RouteConfig{
+				Domain: "target.example.com",
+				CreateConnFn: func(string) (net.Conn, error) {
+					return remoteConn, nil
+				},
+			}))
+			req := rp.injectRequestInfoToCtx(httptest.NewRequest(http.MethodConnect, "target.example.com:443", nil))
+			require.Equal(t, "target.example.com:443", req.Host)
+			body := &connectTestBody{}
+			if originalBody {
+				req.Body = body
+			}
+			requestBody := req.Body
+			getBodyCalls := 0
+			req.GetBody = func() (io.ReadCloser, error) {
+				getBodyCalls++
+				return body, nil
+			}
+			req.Header.Set("Content-Length", "0")
+			req.Header.Set("Trailer", "X-Connect-Trailer")
+			req.Trailer = http.Header{"X-Connect-Trailer": {"value"}}
+			originalHeader, originalTrailer := req.Header.Clone(), req.Trailer.Clone()
+			rw := &connectTestResponseWriter{
+				ResponseRecorder: httptest.NewRecorder(),
+				conn:             clientConn,
+				buffered:         bufio.NewReadWriter(bufio.NewReader(clientConn), bufio.NewWriter(clientConn)),
+			}
+
+			rp.connectHandler(rw, req)
+
+			require.Equal(t, 1, rw.hijackCalls)
+			require.Zero(t, body.reads, "do not read the original Body after Hijack")
+			require.Zero(t, body.closes, "do not close the original Body after Hijack")
+			require.Positive(t, remoteConn.writes.Load(), "exercise CONNECT header write failure")
+			require.True(t, remoteConn.closed.Load(), "close backend on CONNECT header write failure")
+			require.True(t, clientConn.closed.Load(), "close client on CONNECT header write failure")
+			require.Zero(t, remoteConn.reads.Load(), "do not start Join after header write failure")
+			require.Zero(t, clientConn.reads.Load(), "do not start Join after header write failure")
+			require.Zero(t, getBodyCalls)
+			require.Equal(t, requestBody, req.Body)
+			require.NotNil(t, req.GetBody)
+			require.Equal(t, originalHeader, req.Header)
+			require.Equal(t, originalTrailer, req.Trailer)
+		})
+	}
+}
+
+type connectTestResponseWriter struct {
+	*httptest.ResponseRecorder
+	conn        net.Conn
+	buffered    *bufio.ReadWriter
+	hijackCalls int
+}
+
+func (rw *connectTestResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
+	rw.hijackCalls++
+	if rw.conn == nil {
+		return nil, nil, io.ErrClosedPipe
+	}
+	return rw.conn, rw.buffered, nil
+}
+
+type connectTestConn struct {
+	net.Conn
+	failWrites bool
+	closed     atomic.Bool
+	reads      atomic.Int32
+	writes     atomic.Int32
+}
+
+func (c *connectTestConn) Read(p []byte) (int, error) {
+	c.reads.Add(1)
+	return c.Conn.Read(p)
+}
+
+func (c *connectTestConn) Write(p []byte) (int, error) {
+	c.writes.Add(1)
+	if c.failWrites {
+		return 0, io.ErrClosedPipe
+	}
+	return c.Conn.Write(p)
+}
+
+func (c *connectTestConn) Close() error {
+	c.closed.Store(true)
+	return c.Conn.Close()
+}
+
+type connectTestBody struct {
+	reads  int
+	closes int
+}
+
+func (b *connectTestBody) Read([]byte) (int, error) {
+	b.reads++
+	return 0, io.ErrUnexpectedEOF
+}
+
+func (b *connectTestBody) Close() error {
+	b.closes++
+	return nil
+}
+
 func TestHTTPServerProtocols(t *testing.T) {
 	rp := NewHTTPReverseProxy(HTTPReverseProxyOptions{}, NewRouters())
 	protocols := new(http.Protocols)