From fa8477ce5575e05168f977627a6adf50ff011124 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 22 Sep 2022 13:32:00 -0700 Subject: [PATCH] [release-branch.go1.19] 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 #55843 For #54663 For CVE-2022-2880 Change-Id: If1621f6b0e73a49d79059dae9e6b256e0ff18ca9 Reviewed-on: https://go-review.googlesource.com/c/go/+/432976 Reviewed-by: Roland Shoemaker Reviewed-by: Brad Fitzpatrick TryBot-Result: Gopher Robot Run-TryBot: Damien Neil Reviewed-on: https://go-review.googlesource.com/c/go/+/433735 Reviewed-by: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov --- 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 b5d3ce711097b..cf39222be9637 100644 --- a/src/net/http/httputil/reverseproxy.go +++ b/src/net/http/httputil/reverseproxy.go @@ -261,6 +261,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) @@ -639,3 +642,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 90e8903e9cfd5..33b1adea3aa8a 100644 --- a/src/net/http/httputil/reverseproxy_test.go +++ b/src/net/http/httputil/reverseproxy_test.go @@ -1537,3 +1537,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) + } + } +}