1*4882a593SmuzhiyunFrom c8bdf59453c95528a444a85e1b206c1c09eb20f6 Mon Sep 17 00:00:00 2001
2*4882a593SmuzhiyunFrom: Damien Neil <dneil@google.com>
3*4882a593SmuzhiyunDate: Thu, 22 Sep 2022 13:32:00 -0700
4*4882a593SmuzhiyunSubject: [PATCH] net/http/httputil: avoid query parameter smuggling
5*4882a593Smuzhiyun
6*4882a593SmuzhiyunQuery parameter smuggling occurs when a proxy's interpretation
7*4882a593Smuzhiyunof query parameters differs from that of a downstream server.
8*4882a593SmuzhiyunChange ReverseProxy to avoid forwarding ignored query parameters.
9*4882a593Smuzhiyun
10*4882a593SmuzhiyunRemove unparsable query parameters from the outbound request
11*4882a593Smuzhiyun
12*4882a593Smuzhiyun   * if req.Form != nil after calling ReverseProxy.Director; and
13*4882a593Smuzhiyun   * before calling ReverseProxy.Rewrite.
14*4882a593Smuzhiyun
15*4882a593SmuzhiyunThis change preserves the existing behavior of forwarding the
16*4882a593Smuzhiyunraw query untouched if a Director hook does not parse the query
17*4882a593Smuzhiyunby calling Request.ParseForm (possibly indirectly).
18*4882a593Smuzhiyun
19*4882a593SmuzhiyunFixes #55842
20*4882a593SmuzhiyunFor #54663
21*4882a593SmuzhiyunFor CVE-2022-2880
22*4882a593Smuzhiyun
23*4882a593SmuzhiyunChange-Id: If1621f6b0e73a49d79059dae9e6b256e0ff18ca9
24*4882a593SmuzhiyunReviewed-on: https://go-review.googlesource.com/c/go/+/432976
25*4882a593SmuzhiyunReviewed-by: Roland Shoemaker <roland@golang.org>
26*4882a593SmuzhiyunReviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
27*4882a593SmuzhiyunTryBot-Result: Gopher Robot <gobot@golang.org>
28*4882a593SmuzhiyunRun-TryBot: Damien Neil <dneil@google.com>
29*4882a593Smuzhiyun(cherry picked from commit 7c84234142149bd24a4096c6cab691d3593f3431)
30*4882a593SmuzhiyunReviewed-on: https://go-review.googlesource.com/c/go/+/433695
31*4882a593SmuzhiyunReviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
32*4882a593SmuzhiyunReviewed-by: Dmitri Shuralyov <dmitshur@google.com>
33*4882a593Smuzhiyun
34*4882a593SmuzhiyunCVE: CVE-2022-2880
35*4882a593SmuzhiyunUpstream-Status: Backport [9d2c73a9fd69e45876509bb3bdb2af99bf77da1e]
36*4882a593Smuzhiyun
37*4882a593SmuzhiyunSigned-off-by: Sakib Sajal <sakib.sajal@windriver.com>
38*4882a593Smuzhiyun---
39*4882a593Smuzhiyun src/net/http/httputil/reverseproxy.go      | 36 +++++++++++
40*4882a593Smuzhiyun src/net/http/httputil/reverseproxy_test.go | 74 ++++++++++++++++++++++
41*4882a593Smuzhiyun 2 files changed, 110 insertions(+)
42*4882a593Smuzhiyun
43*4882a593Smuzhiyundiff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
44*4882a593Smuzhiyunindex 8b63368..c76eec6 100644
45*4882a593Smuzhiyun--- a/src/net/http/httputil/reverseproxy.go
46*4882a593Smuzhiyun+++ b/src/net/http/httputil/reverseproxy.go
47*4882a593Smuzhiyun@@ -249,6 +249,9 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
48*4882a593Smuzhiyun 	}
49*4882a593Smuzhiyun
50*4882a593Smuzhiyun 	p.Director(outreq)
51*4882a593Smuzhiyun+	if outreq.Form != nil {
52*4882a593Smuzhiyun+		outreq.URL.RawQuery = cleanQueryParams(outreq.URL.RawQuery)
53*4882a593Smuzhiyun+	}
54*4882a593Smuzhiyun 	outreq.Close = false
55*4882a593Smuzhiyun
56*4882a593Smuzhiyun 	reqUpType := upgradeType(outreq.Header)
57*4882a593Smuzhiyun@@ -628,3 +631,36 @@ func (c switchProtocolCopier) copyToBackend(errc chan<- error) {
58*4882a593Smuzhiyun 	_, err := io.Copy(c.backend, c.user)
59*4882a593Smuzhiyun 	errc <- err
60*4882a593Smuzhiyun }
61*4882a593Smuzhiyun+
62*4882a593Smuzhiyun+func cleanQueryParams(s string) string {
63*4882a593Smuzhiyun+	reencode := func(s string) string {
64*4882a593Smuzhiyun+		v, _ := url.ParseQuery(s)
65*4882a593Smuzhiyun+		return v.Encode()
66*4882a593Smuzhiyun+	}
67*4882a593Smuzhiyun+	for i := 0; i < len(s); {
68*4882a593Smuzhiyun+		switch s[i] {
69*4882a593Smuzhiyun+		case ';':
70*4882a593Smuzhiyun+			return reencode(s)
71*4882a593Smuzhiyun+		case '%':
72*4882a593Smuzhiyun+			if i+2 >= len(s) || !ishex(s[i+1]) || !ishex(s[i+2]) {
73*4882a593Smuzhiyun+				return reencode(s)
74*4882a593Smuzhiyun+			}
75*4882a593Smuzhiyun+			i += 3
76*4882a593Smuzhiyun+		default:
77*4882a593Smuzhiyun+			i++
78*4882a593Smuzhiyun+		}
79*4882a593Smuzhiyun+	}
80*4882a593Smuzhiyun+	return s
81*4882a593Smuzhiyun+}
82*4882a593Smuzhiyun+
83*4882a593Smuzhiyun+func ishex(c byte) bool {
84*4882a593Smuzhiyun+	switch {
85*4882a593Smuzhiyun+	case '0' <= c && c <= '9':
86*4882a593Smuzhiyun+		return true
87*4882a593Smuzhiyun+	case 'a' <= c && c <= 'f':
88*4882a593Smuzhiyun+		return true
89*4882a593Smuzhiyun+	case 'A' <= c && c <= 'F':
90*4882a593Smuzhiyun+		return true
91*4882a593Smuzhiyun+	}
92*4882a593Smuzhiyun+	return false
93*4882a593Smuzhiyun+}
94*4882a593Smuzhiyundiff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
95*4882a593Smuzhiyunindex 4b6ad77..8c0a4f1 100644
96*4882a593Smuzhiyun--- a/src/net/http/httputil/reverseproxy_test.go
97*4882a593Smuzhiyun+++ b/src/net/http/httputil/reverseproxy_test.go
98*4882a593Smuzhiyun@@ -1517,3 +1517,77 @@ func TestJoinURLPath(t *testing.T) {
99*4882a593Smuzhiyun 		}
100*4882a593Smuzhiyun 	}
101*4882a593Smuzhiyun }
102*4882a593Smuzhiyun+
103*4882a593Smuzhiyun+const (
104*4882a593Smuzhiyun+	testWantsCleanQuery = true
105*4882a593Smuzhiyun+	testWantsRawQuery   = false
106*4882a593Smuzhiyun+)
107*4882a593Smuzhiyun+
108*4882a593Smuzhiyun+func TestReverseProxyQueryParameterSmugglingDirectorDoesNotParseForm(t *testing.T) {
109*4882a593Smuzhiyun+	testReverseProxyQueryParameterSmuggling(t, testWantsRawQuery, func(u *url.URL) *ReverseProxy {
110*4882a593Smuzhiyun+		proxyHandler := NewSingleHostReverseProxy(u)
111*4882a593Smuzhiyun+		oldDirector := proxyHandler.Director
112*4882a593Smuzhiyun+		proxyHandler.Director = func(r *http.Request) {
113*4882a593Smuzhiyun+			oldDirector(r)
114*4882a593Smuzhiyun+		}
115*4882a593Smuzhiyun+		return proxyHandler
116*4882a593Smuzhiyun+	})
117*4882a593Smuzhiyun+}
118*4882a593Smuzhiyun+
119*4882a593Smuzhiyun+func TestReverseProxyQueryParameterSmugglingDirectorParsesForm(t *testing.T) {
120*4882a593Smuzhiyun+	testReverseProxyQueryParameterSmuggling(t, testWantsCleanQuery, func(u *url.URL) *ReverseProxy {
121*4882a593Smuzhiyun+		proxyHandler := NewSingleHostReverseProxy(u)
122*4882a593Smuzhiyun+		oldDirector := proxyHandler.Director
123*4882a593Smuzhiyun+		proxyHandler.Director = func(r *http.Request) {
124*4882a593Smuzhiyun+			// Parsing the form causes ReverseProxy to remove unparsable
125*4882a593Smuzhiyun+			// query parameters before forwarding.
126*4882a593Smuzhiyun+			r.FormValue("a")
127*4882a593Smuzhiyun+			oldDirector(r)
128*4882a593Smuzhiyun+		}
129*4882a593Smuzhiyun+		return proxyHandler
130*4882a593Smuzhiyun+	})
131*4882a593Smuzhiyun+}
132*4882a593Smuzhiyun+
133*4882a593Smuzhiyun+func testReverseProxyQueryParameterSmuggling(t *testing.T, wantCleanQuery bool, newProxy func(*url.URL) *ReverseProxy) {
134*4882a593Smuzhiyun+	const content = "response_content"
135*4882a593Smuzhiyun+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
136*4882a593Smuzhiyun+		w.Write([]byte(r.URL.RawQuery))
137*4882a593Smuzhiyun+	}))
138*4882a593Smuzhiyun+	defer backend.Close()
139*4882a593Smuzhiyun+	backendURL, err := url.Parse(backend.URL)
140*4882a593Smuzhiyun+	if err != nil {
141*4882a593Smuzhiyun+		t.Fatal(err)
142*4882a593Smuzhiyun+	}
143*4882a593Smuzhiyun+	proxyHandler := newProxy(backendURL)
144*4882a593Smuzhiyun+	frontend := httptest.NewServer(proxyHandler)
145*4882a593Smuzhiyun+	defer frontend.Close()
146*4882a593Smuzhiyun+
147*4882a593Smuzhiyun+	// Don't spam output with logs of queries containing semicolons.
148*4882a593Smuzhiyun+	backend.Config.ErrorLog = log.New(io.Discard, "", 0)
149*4882a593Smuzhiyun+	frontend.Config.ErrorLog = log.New(io.Discard, "", 0)
150*4882a593Smuzhiyun+
151*4882a593Smuzhiyun+	for _, test := range []struct {
152*4882a593Smuzhiyun+		rawQuery   string
153*4882a593Smuzhiyun+		cleanQuery string
154*4882a593Smuzhiyun+	}{{
155*4882a593Smuzhiyun+		rawQuery:   "a=1&a=2;b=3",
156*4882a593Smuzhiyun+		cleanQuery: "a=1",
157*4882a593Smuzhiyun+	}, {
158*4882a593Smuzhiyun+		rawQuery:   "a=1&a=%zz&b=3",
159*4882a593Smuzhiyun+		cleanQuery: "a=1&b=3",
160*4882a593Smuzhiyun+	}} {
161*4882a593Smuzhiyun+		res, err := frontend.Client().Get(frontend.URL + "?" + test.rawQuery)
162*4882a593Smuzhiyun+		if err != nil {
163*4882a593Smuzhiyun+			t.Fatalf("Get: %v", err)
164*4882a593Smuzhiyun+		}
165*4882a593Smuzhiyun+		defer res.Body.Close()
166*4882a593Smuzhiyun+		body, _ := io.ReadAll(res.Body)
167*4882a593Smuzhiyun+		wantQuery := test.rawQuery
168*4882a593Smuzhiyun+		if wantCleanQuery {
169*4882a593Smuzhiyun+			wantQuery = test.cleanQuery
170*4882a593Smuzhiyun+		}
171*4882a593Smuzhiyun+		if got, want := string(body), wantQuery; got != want {
172*4882a593Smuzhiyun+			t.Errorf("proxy forwarded raw query %q as %q, want %q", test.rawQuery, got, want)
173*4882a593Smuzhiyun+		}
174*4882a593Smuzhiyun+	}
175*4882a593Smuzhiyun+}
176*4882a593Smuzhiyun--
177*4882a593Smuzhiyun2.32.0
178*4882a593Smuzhiyun
179