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