net/http/httputil: avoid query parameter smuggling

Query parameter smuggling occurs when a proxy's interpretation
of query parameters differs from that of a downstream server.
Change ReverseProxy to avoid forwarding ignored query parameters.

Remove unparsable query parameters from the outbound request

   * if req.Form != nil after calling ReverseProxy.Director; and
   * before calling ReverseProxy.Rewrite.

This change preserves the existing behavior of forwarding the
raw query untouched if a Director hook does not parse the query
by calling Request.ParseForm (possibly indirectly).

Fixes #54663
Fixes CVE-2022-2880

Change-Id: If1621f6b0e73a49d79059dae9e6b256e0ff18ca9
Reviewed-on: https://go-review.googlesource.com/c/go/+/432976
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>

1 files changed, 50 insertions, 0 deletions

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index fb1aa0f3e4..190279ca00 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -113,6 +113,14 @@ type ReverseProxy struct {
 	// outbound request before Rewrite is called. See also
 	// the ProxyRequest.SetXForwarded method.
 	//
+	// Unparsable query parameters are removed from the
+	// outbound request before Rewrite is called.
+	// The Rewrite function may copy the inbound URL's
+	// RawQuery to the outbound URL to preserve the original
+	// parameter string. Note that this can lead to security
+	// issues if the proxy's interpretation of query parameters
+	// does not match that of the downstream server.
+	//
 	// At most one of Rewrite or Director may be set.
 	Rewrite func(*ProxyRequest)
 
@@ -140,6 +148,9 @@ type ReverseProxy struct {
 	// Director. Use a Rewrite function instead to ensure
 	// modifications to the request are preserved.
 	//
+	// Unparsable query parameters are removed from the outbound
+	// request if Request.Form is set after Director returns.
+	//
 	// At most one of Rewrite or Director may be set.
 	Director func(*http.Request)
 
@@ -374,6 +385,9 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 
 	if p.Director != nil {
 		p.Director(outreq)
+		if outreq.Form != nil {
+			outreq.URL.RawQuery = cleanQueryParams(outreq.URL.RawQuery)
+		}
 	}
 	outreq.Close = false
 
@@ -409,6 +423,9 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		outreq.Header.Del("X-Forwarded-Host")
 		outreq.Header.Del("X-Forwarded-Proto")
 
+		// Remove unparsable query parameters from the outbound request.
+		outreq.URL.RawQuery = cleanQueryParams(outreq.URL.RawQuery)
+
 		pr := &ProxyRequest{
 			In:  req,
 			Out: outreq,
@@ -797,3 +814,36 @@ func (c switchProtocolCopier) copyToBackend(errc chan<- error) {
 	_, err := io.Copy(c.backend, c.user)
 	errc <- err
 }
+
+func cleanQueryParams(s string) string {
+	reencode := func(s string) string {
+		v, _ := url.ParseQuery(s)
+		return v.Encode()
+	}
+	for i := 0; i < len(s); {
+		switch s[i] {
+		case ';':
+			return reencode(s)
+		case '%':
+			if i+2 >= len(s) || !ishex(s[i+1]) || !ishex(s[i+2]) {
+				return reencode(s)
+			}
+			i += 3
+		default:
+			i++
+		}
+	}
+	return s
+}
+
+func ishex(c byte) bool {
+	switch {
+	case '0' <= c && c <= '9':
+		return true
+	case 'a' <= c && c <= 'f':
+		return true
+	case 'A' <= c && c <= 'F':
+		return true
+	}
+	return false
+}