Skip to content

Commit 950fa11

Browse files
FiloSottileFiloSottile
committed
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 <[email protected]> Reviewed-by: Katie Hockman <[email protected]> Trust: Katie Hockman <[email protected]> Trust: Filippo Valsorda <[email protected]> TryBot-Result: Go Bot <[email protected]>
1 parent 9bc5268 commit 950fa11

2 files changed

Lines changed: 70 additions & 15 deletions

File tree

‎src/net/http/httputil/reverseproxy.go‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -253,22 +253,18 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
253253
// important is "Connection" because we want a persistent
254254
// connection, regardless of what the client sent to us.
255255
for _, h := range hopHeaders {
256-
hv := outreq.Header.Get(h)
257-
if hv == "" {
258-
continue
259-
}
260-
if h == "Te" && hv == "trailers" {
261-
// Issue 21096: tell backend applications that
262-
// care about trailer support that we support
263-
// trailers. (We do, but we don't go out of
264-
// our way to advertise that unless the
265-
// incoming client request thought it was
266-
// worth mentioning)
267-
continue
268-
}
269256
outreq.Header.Del(h)
270257
}
271258

259+
// Issue 21096: tell backend applications that care about trailer support
260+
// that we support trailers. (We do, but we don't go out of our way to
261+
// advertise that unless the incoming client request thought it was worth
262+
// mentioning.) Note that we look at req.Header, not outreq.Header, since
263+
// the latter has passed through removeConnectionHeaders.
264+
if httpguts.HeaderValuesContainsToken(req.Header["Te"], "trailers") {
265+
outreq.Header.Set("Te", "trailers")
266+
}
267+
272268
// After stripping all the hop-by-hop connection headers above, add back any
273269
// necessary for protocol upgrades, such as for websockets.
274270
if reqUpType != "" {

‎src/net/http/httputil/reverseproxy_test.go‎

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,9 @@ func TestReverseProxy(t *testing.T) {
9191

9292
getReq, _ := http.NewRequest("GET", frontend.URL, nil)
9393
getReq.Host = "some-name"
94-
getReq.Header.Set("Connection", "close")
95-
getReq.Header.Set("Te", "trailers")
94+
getReq.Header.Set("Connection", "close, TE")
95+
getReq.Header.Add("Te", "foo")
96+
getReq.Header.Add("Te", "bar, trailers")
9697
getReq.Header.Set("Proxy-Connection", "should be deleted")
9798
getReq.Header.Set("Upgrade", "foo")
9899
getReq.Close = true
@@ -236,6 +237,64 @@ func TestReverseProxyStripHeadersPresentInConnection(t *testing.T) {
236237
}
237238
}
238239

240+
func TestReverseProxyStripEmptyConnection(t *testing.T) {
241+
// See Issue 46313.
242+
const backendResponse = "I am the backend"
243+
244+
// someConnHeader is some arbitrary header to be declared as a hop-by-hop header
245+
// in the Request's Connection header.
246+
const someConnHeader = "X-Some-Conn-Header"
247+
248+
backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
249+
if c := r.Header.Values("Connection"); len(c) != 0 {
250+
t.Errorf("handler got header %q = %v; want empty", "Connection", c)
251+
}
252+
if c := r.Header.Get(someConnHeader); c != "" {
253+
t.Errorf("handler got header %q = %q; want empty", someConnHeader, c)
254+
}
255+
w.Header().Add("Connection", "")
256+
w.Header().Add("Connection", someConnHeader)
257+
w.Header().Set(someConnHeader, "should be deleted")
258+
io.WriteString(w, backendResponse)
259+
}))
260+
defer backend.Close()
261+
backendURL, err := url.Parse(backend.URL)
262+
if err != nil {
263+
t.Fatal(err)
264+
}
265+
proxyHandler := NewSingleHostReverseProxy(backendURL)
266+
frontend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
267+
proxyHandler.ServeHTTP(w, r)
268+
if c := r.Header.Get(someConnHeader); c != "should be deleted" {
269+
t.Errorf("handler modified header %q = %q; want %q", someConnHeader, c, "should be deleted")
270+
}
271+
}))
272+
defer frontend.Close()
273+
274+
getReq, _ := http.NewRequest("GET", frontend.URL, nil)
275+
getReq.Header.Add("Connection", "")
276+
getReq.Header.Add("Connection", someConnHeader)
277+
getReq.Header.Set(someConnHeader, "should be deleted")
278+
res, err := frontend.Client().Do(getReq)
279+
if err != nil {
280+
t.Fatalf("Get: %v", err)
281+
}
282+
defer res.Body.Close()
283+
bodyBytes, err := io.ReadAll(res.Body)
284+
if err != nil {
285+
t.Fatalf("reading body: %v", err)
286+
}
287+
if got, want := string(bodyBytes), backendResponse; got != want {
288+
t.Errorf("got body %q; want %q", got, want)
289+
}
290+
if c := res.Header.Get("Connection"); c != "" {
291+
t.Errorf("handler got header %q = %q; want empty", "Connection", c)
292+
}
293+
if c := res.Header.Get(someConnHeader); c != "" {
294+
t.Errorf("handler got header %q = %q; want empty", someConnHeader, c)
295+
}
296+
}
297+
239298
func TestXForwardedFor(t *testing.T) {
240299
const prevForwardedFor = "client ip"
241300
const backendResponse = "I am the backend"

0 commit comments

Comments
 (0)