1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
|
From c8bdf59453c95528a444a85e1b206c1c09eb20f6 Mon Sep 17 00:00:00 2001
From: Damien Neil <dneil@google.com>
Date: Thu, 22 Sep 2022 13:32:00 -0700
Subject: [PATCH] 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 #55842
For #54663
For 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>
(cherry picked from commit 7c84234142149bd24a4096c6cab691d3593f3431)
Reviewed-on: https://go-review.googlesource.com/c/go/+/433695
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
CVE: CVE-2022-2880
Upstream-Status: Backport [9d2c73a9fd69e45876509bb3bdb2af99bf77da1e]
Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>
---
src/net/http/httputil/reverseproxy.go | 36 +++++++++++
src/net/http/httputil/reverseproxy_test.go | 74 ++++++++++++++++++++++
2 files changed, 110 insertions(+)
diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 8b63368..c76eec6 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -249,6 +249,9 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
}
p.Director(outreq)
+ if outreq.Form != nil {
+ outreq.URL.RawQuery = cleanQueryParams(outreq.URL.RawQuery)
+ }
outreq.Close = false
reqUpType := upgradeType(outreq.Header)
@@ -628,3 +631,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
+}
diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 4b6ad77..8c0a4f1 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -1517,3 +1517,77 @@ func TestJoinURLPath(t *testing.T) {
}
}
}
+
+const (
+ testWantsCleanQuery = true
+ testWantsRawQuery = false
+)
+
+func TestReverseProxyQueryParameterSmugglingDirectorDoesNotParseForm(t *testing.T) {
+ testReverseProxyQueryParameterSmuggling(t, testWantsRawQuery, func(u *url.URL) *ReverseProxy {
+ proxyHandler := NewSingleHostReverseProxy(u)
+ oldDirector := proxyHandler.Director
+ proxyHandler.Director = func(r *http.Request) {
+ oldDirector(r)
+ }
+ return proxyHandler
+ })
+}
+
+func TestReverseProxyQueryParameterSmugglingDirectorParsesForm(t *testing.T) {
+ testReverseProxyQueryParameterSmuggling(t, testWantsCleanQuery, func(u *url.URL) *ReverseProxy {
+ proxyHandler := NewSingleHostReverseProxy(u)
+ oldDirector := proxyHandler.Director
+ proxyHandler.Director = func(r *http.Request) {
+ // Parsing the form causes ReverseProxy to remove unparsable
+ // query parameters before forwarding.
+ r.FormValue("a")
+ oldDirector(r)
+ }
+ return proxyHandler
+ })
+}
+
+func testReverseProxyQueryParameterSmuggling(t *testing.T, wantCleanQuery bool, newProxy func(*url.URL) *ReverseProxy) {
+ const content = "response_content"
+ backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Write([]byte(r.URL.RawQuery))
+ }))
+ defer backend.Close()
+ backendURL, err := url.Parse(backend.URL)
+ if err != nil {
+ t.Fatal(err)
+ }
+ proxyHandler := newProxy(backendURL)
+ frontend := httptest.NewServer(proxyHandler)
+ defer frontend.Close()
+
+ // Don't spam output with logs of queries containing semicolons.
+ backend.Config.ErrorLog = log.New(io.Discard, "", 0)
+ frontend.Config.ErrorLog = log.New(io.Discard, "", 0)
+
+ for _, test := range []struct {
+ rawQuery string
+ cleanQuery string
+ }{{
+ rawQuery: "a=1&a=2;b=3",
+ cleanQuery: "a=1",
+ }, {
+ rawQuery: "a=1&a=%zz&b=3",
+ cleanQuery: "a=1&b=3",
+ }} {
+ res, err := frontend.Client().Get(frontend.URL + "?" + test.rawQuery)
+ if err != nil {
+ t.Fatalf("Get: %v", err)
+ }
+ defer res.Body.Close()
+ body, _ := io.ReadAll(res.Body)
+ wantQuery := test.rawQuery
+ if wantCleanQuery {
+ wantQuery = test.cleanQuery
+ }
+ if got, want := string(body), wantQuery; got != want {
+ t.Errorf("proxy forwarded raw query %q as %q, want %q", test.rawQuery, got, want)
+ }
+ }
+}
--
2.32.0
|
