179 lines
5.3 KiB
Diff
179 lines
5.3 KiB
Diff
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
|
|
|