[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 <filippo@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Katie Hockman <katie@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/323091
Run-TryBot: Katie Hockman <katie@golang.org>

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 3f48fab544e912506e94f78b4eed8b23b5f31019..f49cefbb4f17120d31f82a3eeec103f81d45426a 100644 (file)
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -248,22 +248,18 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
        // 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 764939fb0f0c26a6f2511e603d9cc9237808e4a4..1f2dfb9867f68ea2db61d00abc92dfba5fc53680 100644 (file)
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -91,8 +91,9 @@ func TestReverseProxy(t *testing.T) {
 
        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 TestReverseProxyStripHeadersPresentInConnection(t *testing.T) {
        }
 }
 
+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 := 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"