[go] net/http/httputil: always remove hop-by-hop headers

Filippo Valsorda (Gerrit)

unread,

May 27, 2021, 11:01:07 AM5/27/21

to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, Katie Hockman, Roland Shoemaker, golang-co...@googlegroups.com

Filippo Valsorda submitted this change.

View Change

Approvals:
  Katie Hockman: Looks good to me, approved; Trusted
  Filippo Valsorda: Trusted; Run TryBots
  Go Bot: TryBots succeeded

net/http/httputil: always remove hop-by-hop headers

Previously, we'd fail to remove the Connection header from a request
like this:

    Connection:
    Connection: x-header

Fixes #46313
Fixes CVE-2021-33197

Change-Id: Ie3009e926ceecfa86dfa6bcc6fe14ff01086be7d
Reviewed-on: https://go-review.googlesource.com/c/go/+/321929
Run-TryBot: Filippo Valsorda <fil...@golang.org>
Reviewed-by: Katie Hockman <ka...@golang.org>
Trust: Katie Hockman <ka...@golang.org>
Trust: Filippo Valsorda <fil...@golang.org>
TryBot-Result: Go Bot <go...@golang.org>
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 1432ee2..5d39955 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -253,22 +253,18 @@
 	// important is "Connection" because we want a persistent
 	// connection, regardless of what the client sent to us.
 	for _, h := range hopHeaders {
-		hv := outreq.Header.Get(h)
-		if hv == "" {
-			continue
-		}
-		if h == "Te" && hv == "trailers" {
-			// Issue 21096: tell backend applications that
-			// care about trailer support that we support
-			// trailers. (We do, but we don't go out of
-			// our way to advertise that unless the
-			// incoming client request thought it was
-			// worth mentioning)
-			continue
-		}
 		outreq.Header.Del(h)
 	}
 
+	// Issue 21096: tell backend applications that care about trailer support
+	// that we support trailers. (We do, but we don't go out of our way to
+	// advertise that unless the incoming client request thought it was worth
+	// mentioning.) Note that we look at req.Header, not outreq.Header, since
+	// the latter has passed through removeConnectionHeaders.
+	if httpguts.HeaderValuesContainsToken(req.Header["Te"], "trailers") {
+		outreq.Header.Set("Te", "trailers")
+	}
+
 	// After stripping all the hop-by-hop connection headers above, add back any
 	// necessary for protocol upgrades, such as for websockets.
 	if reqUpType != "" {
diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 22720ca..1898ed8 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -91,8 +91,9 @@
 
 	getReq, _ := http.NewRequest("GET", frontend.URL, nil)
 	getReq.Host = "some-name"
-	getReq.Header.Set("Connection", "close")
-	getReq.Header.Set("Te", "trailers")
+	getReq.Header.Set("Connection", "close, TE")
+	getReq.Header.Add("Te", "foo")
+	getReq.Header.Add("Te", "bar, trailers")
 	getReq.Header.Set("Proxy-Connection", "should be deleted")
 	getReq.Header.Set("Upgrade", "foo")
 	getReq.Close = true
@@ -236,6 +237,64 @@
 	}
 }
 
+func TestReverseProxyStripEmptyConnection(t *testing.T) {
+	// See Issue 46313.
+	const backendResponse = "I am the backend"
+
+	// someConnHeader is some arbitrary header to be declared as a hop-by-hop header
+	// in the Request's Connection header.
+	const someConnHeader = "X-Some-Conn-Header"
+
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if c := r.Header.Values("Connection"); len(c) != 0 {
+			t.Errorf("handler got header %q = %v; want empty", "Connection", c)
+		}
+		if c := r.Header.Get(someConnHeader); c != "" {
+			t.Errorf("handler got header %q = %q; want empty", someConnHeader, c)
+		}
+		w.Header().Add("Connection", "")
+		w.Header().Add("Connection", someConnHeader)
+		w.Header().Set(someConnHeader, "should be deleted")
+		io.WriteString(w, backendResponse)
+	}))
+	defer backend.Close()
+	backendURL, err := url.Parse(backend.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	proxyHandler := NewSingleHostReverseProxy(backendURL)
+	frontend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		proxyHandler.ServeHTTP(w, r)
+		if c := r.Header.Get(someConnHeader); c != "should be deleted" {
+			t.Errorf("handler modified header %q = %q; want %q", someConnHeader, c, "should be deleted")
+		}
+	}))
+	defer frontend.Close()
+
+	getReq, _ := http.NewRequest("GET", frontend.URL, nil)
+	getReq.Header.Add("Connection", "")
+	getReq.Header.Add("Connection", someConnHeader)
+	getReq.Header.Set(someConnHeader, "should be deleted")
+	res, err := frontend.Client().Do(getReq)
+	if err != nil {
+		t.Fatalf("Get: %v", err)
+	}
+	defer res.Body.Close()
+	bodyBytes, err := io.ReadAll(res.Body)
+	if err != nil {
+		t.Fatalf("reading body: %v", err)
+	}
+	if got, want := string(bodyBytes), backendResponse; got != want {
+		t.Errorf("got body %q; want %q", got, want)
+	}
+	if c := res.Header.Get("Connection"); c != "" {
+		t.Errorf("handler got header %q = %q; want empty", "Connection", c)
+	}
+	if c := res.Header.Get(someConnHeader); c != "" {
+		t.Errorf("handler got header %q = %q; want empty", someConnHeader, c)
+	}
+}
+
 func TestXForwardedFor(t *testing.T) {
 	const prevForwardedFor = "client ip"
 	const backendResponse = "I am the backend"

To view, visit change 321929. To unsubscribe, or for help writing mail filters, visit settings.

Katie Hockman (Gerrit)

unread,

May 28, 2021, 9:53:40 AM5/28/21

to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, golang-co...@googlegroups.com

Katie Hockman submitted this change.

View Change

Approvals:
  Katie Hockman: Looks good to me, approved
  Filippo Valsorda: Trusted; Run TryBots
  Go Bot: TryBots succeeded

[release-branch.go1.16] net/http/httputil: always remove hop-by-hop headers



Previously, we'd fail to remove the Connection header from a request
like this:

    Connection:
    Connection: x-header

Updates #46313
Fixes #46315


Fixes CVE-2021-33197

Change-Id: Ie3009e926ceecfa86dfa6bcc6fe14ff01086be7d
Reviewed-on: https://go-review.googlesource.com/c/go/+/321929
Run-TryBot: Filippo Valsorda <fil...@golang.org>
Reviewed-by: Katie Hockman <ka...@golang.org>
Trust: Katie Hockman <ka...@golang.org>
Trust: Filippo Valsorda <fil...@golang.org>
TryBot-Result: Go Bot <go...@golang.org>

(cherry picked from commit 950fa11c4cb01a145bb07eeb167d90a1846061b3)
Reviewed-on: https://go-review.googlesource.com/c/go/+/323090


---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 4e36958..ccb8456 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -248,22 +248,18 @@

index 3acbd94..3211463 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -90,8 +90,9 @@


 
 	getReq, _ := http.NewRequest("GET", frontend.URL, nil)
 	getReq.Host = "some-name"
-	getReq.Header.Set("Connection", "close")
-	getReq.Header.Set("Te", "trailers")
+	getReq.Header.Set("Connection", "close, TE")
+	getReq.Header.Add("Te", "foo")
+	getReq.Header.Add("Te", "bar, trailers")
 	getReq.Header.Set("Proxy-Connection", "should be deleted")
 	getReq.Header.Set("Upgrade", "foo")
 	getReq.Close = true

@@ -235,6 +236,64 @@

To view, visit change 323090. To unsubscribe, or for help writing mail filters, visit settings.

Katie Hockman (Gerrit)

unread,

May 28, 2021, 10:38:24 AM5/28/21

to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, golang-co...@googlegroups.com

Katie Hockman submitted this change.

View Change

Approvals:
  Katie Hockman: Looks good to me, approved; Trusted; Run TryBots
  Filippo Valsorda: Trusted
  Go Bot: TryBots succeeded

[release-branch.go1.15] net/http/httputil: always remove hop-by-hop headers



Previously, we'd fail to remove the Connection header from a request
like this:

    Connection:
    Connection: x-header

Updates #46313

Fixes #46314


Fixes CVE-2021-33197

Change-Id: Ie3009e926ceecfa86dfa6bcc6fe14ff01086be7d
Reviewed-on: https://go-review.googlesource.com/c/go/+/321929
Run-TryBot: Filippo Valsorda <fil...@golang.org>
Reviewed-by: Katie Hockman <ka...@golang.org>
Trust: Katie Hockman <ka...@golang.org>
Trust: Filippo Valsorda <fil...@golang.org>
TryBot-Result: Go Bot <go...@golang.org>

Reviewed-on: https://go-review.googlesource.com/c/go/+/323091
Run-TryBot: Katie Hockman <ka...@golang.org>


---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 3f48fab..f49cefb 100644

index 764939f..1f2dfb9 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -91,8 +91,9 @@


 
 	getReq, _ := http.NewRequest("GET", frontend.URL, nil)
 	getReq.Host = "some-name"
-	getReq.Header.Set("Connection", "close")
-	getReq.Header.Set("Te", "trailers")
+	getReq.Header.Set("Connection", "close, TE")
+	getReq.Header.Add("Te", "foo")
+	getReq.Header.Add("Te", "bar, trailers")
 	getReq.Header.Set("Proxy-Connection", "should be deleted")
 	getReq.Header.Set("Upgrade", "foo")
 	getReq.Close = true

@@ -236,6 +237,64 @@

+	bodyBytes, err := ioutil.ReadAll(res.Body)


+	if err != nil {
+		t.Fatalf("reading body: %v", err)
+	}
+	if got, want := string(bodyBytes), backendResponse; got != want {
+		t.Errorf("got body %q; want %q", got, want)
+	}
+	if c := res.Header.Get("Connection"); c != "" {
+		t.Errorf("handler got header %q = %q; want empty", "Connection", c)
+	}
+	if c := res.Header.Get(someConnHeader); c != "" {
+		t.Errorf("handler got header %q = %q; want empty", someConnHeader, c)
+	}
+}
+
 func TestXForwardedFor(t *testing.T) {
 	const prevForwardedFor = "client ip"
 	const backendResponse = "I am the backend"

1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

The name of the file: src/net/http/httputil/reverseproxy_test.go
Insertions: 1, Deletions: 1.

```
@@ -282:283, +282:283 @@
-  	bodyBytes, err := io.ReadAll(res.Body)
+  	bodyBytes, err := ioutil.ReadAll(res.Body)

```

To view, visit change 323091. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward