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