diff options
author | Damien Neil <dneil@google.com> | 2021-07-07 16:34:34 -0700 |
---|---|---|
committer | Filippo Valsorda <filippo@golang.org> | 2021-08-02 16:58:12 +0000 |
commit | accf363d5da864521c90b152fb734f3f15e00521 (patch) | |
tree | 2e77972f5aeb0cf0f3f8931e0f31777c323c7509 | |
parent | ae7943e11b41b5ee98279bf0574942e09f15ce20 (diff) | |
download | go-accf363d5da864521c90b152fb734f3f15e00521.tar.gz go-accf363d5da864521c90b152fb734f3f15e00521.zip |
[release-branch.go1.16] net/http/httputil: close incoming ReverseProxy request body
Reading from an incoming request body after the request handler aborts
with a panic can cause a panic, becuse http.Server does not (contrary
to its documentation) close the request body in this case.
Always close the incoming request body in ReverseProxy.ServeHTTP to
ensure that any in-flight outgoing requests using the body do not
read from it.
Fixes #47474
Updates #46866
Fixes CVE-2021-36221
Change-Id: I310df269200ad8732c5d9f1a2b00de68725831df
Reviewed-on: https://go-review.googlesource.com/c/go/+/333191
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
(cherry picked from commit b7a85e0003cedb1b48a1fd3ae5b746ec6330102e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/338551
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
-rw-r--r-- | src/net/http/httputil/reverseproxy.go | 9 | ||||
-rw-r--r-- | src/net/http/httputil/reverseproxy_test.go | 39 |
2 files changed, 48 insertions, 0 deletions
diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go index ccb84562e0..17e6a1dc7a 100644 --- a/src/net/http/httputil/reverseproxy.go +++ b/src/net/http/httputil/reverseproxy.go @@ -234,6 +234,15 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { if req.ContentLength == 0 { outreq.Body = nil // Issue 16036: nil Body for http.Transport retries } + if outreq.Body != nil { + // Reading from the request body after returning from a handler is not + // allowed, and the RoundTrip goroutine that reads the Body can outlive + // this handler. This can lead to a crash if the handler panics (see + // Issue 46866). Although calling Close doesn't guarantee there isn't + // any Read in flight after the handle returns, in practice it's safe to + // read after closing it. + defer outreq.Body.Close() + } if outreq.Header == nil { outreq.Header = make(http.Header) // Issue 33142: historical behavior was to always allocate } diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go index 3211463bcb..46da4022c7 100644 --- a/src/net/http/httputil/reverseproxy_test.go +++ b/src/net/http/httputil/reverseproxy_test.go @@ -1121,6 +1121,45 @@ func TestReverseProxy_PanicBodyError(t *testing.T) { rproxy.ServeHTTP(httptest.NewRecorder(), req) } +// Issue #46866: panic without closing incoming request body causes a panic +func TestReverseProxy_PanicClosesIncomingBody(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + out := "this call was relayed by the reverse proxy" + // Coerce a wrong content length to induce io.ErrUnexpectedEOF + w.Header().Set("Content-Length", fmt.Sprintf("%d", len(out)*2)) + fmt.Fprintln(w, out) + })) + defer backend.Close() + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + proxyHandler := NewSingleHostReverseProxy(backendURL) + proxyHandler.ErrorLog = log.New(io.Discard, "", 0) // quiet for tests + frontend := httptest.NewServer(proxyHandler) + defer frontend.Close() + frontendClient := frontend.Client() + + var wg sync.WaitGroup + for i := 0; i < 2; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 10; j++ { + const reqLen = 6 * 1024 * 1024 + req, _ := http.NewRequest("POST", frontend.URL, &io.LimitedReader{R: neverEnding('x'), N: reqLen}) + req.ContentLength = reqLen + resp, _ := frontendClient.Transport.RoundTrip(req) + if resp != nil { + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + } + } + }() + } + wg.Wait() +} + func TestSelectFlushInterval(t *testing.T) { tests := []struct { name string |