Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: panic due to racy read of persistConn after handler panic #46866

Closed
neild opened this issue Jun 22, 2021 · 13 comments
Closed

net/http: panic due to racy read of persistConn after handler panic #46866

neild opened this issue Jun 22, 2021 · 13 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@neild
Copy link
Contributor

neild commented Jun 22, 2021

Let's start with the reproduction case:

func TestHandlerAbortRacesBodyRead(t *testing.T) {
        setParallel(t)
        defer afterTest(t)

        ts := httptest.NewServer(HandlerFunc(func(rw ResponseWriter, req *Request) {
                go io.Copy(io.Discard, req.Body)
                panic(ErrAbortHandler)
        }))
        defer ts.Close()

        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, _ := NewRequest("POST", ts.URL, &io.LimitedReader{R: neverEnding('x'), N: reqLen})
                                req.ContentLength = reqLen
                                resp, _ := ts.Client().Transport.RoundTrip(req)
                                if resp != nil {
                                        resp.Body.Close()
                                }
                        }
                }()
        }
        wg.Wait()
}

There might be something simpler, but this consistently panics for me on multiple systems. The above is a bit contrived, but the panic consistently occurs in some much less contrived scenarios with httputil.ReverseProxy, which likes to panic(ErrAbortHandler) when it encounters a problem copying a proxied response body.

The root cause is (I think) that we don't close the request body after a handler panic. We do close the *conn, returning its bufio.Reader and bufio.Writer to their sync.Pools. But the request body has an indirect reference to the *conn's reader, and while the handler has exited, some other goroutine may still be reading from it.

@neild neild self-assigned this Jun 22, 2021
@gopherbot
Copy link

Change https://golang.org/cl/329922 mentions this issue: net/http: close request body after recovering from a handler panic

@seankhliao seankhliao added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 23, 2021
@FiloSottile
Copy link
Contributor

I am not convinced this is a net/http issue. The Handler docs say that reading from Body after ServeHTTP has returned is not ok.

Returning signals that the request is finished; it is not valid to use the ResponseWriter or read from the Request.Body after or concurrently with the completion of the ServeHTTP call.

Given that, it sounds like the issue is that ReverseProxy will do that, rather than that net/http won't handle it. Is that correct?

@neild
Copy link
Contributor Author

neild commented Jul 7, 2021

Reading from Body after a ServeHTTP Handler returns is not okay, but it should be "get a read error" not okay, not "panic the process" not okay.

There may be other fixes possible here--this is a complex interaction between the HTTP server and client, with RoundTripper in the middle--but I think the most robust defense is to ensure that use of a Body after a handler has returned is not unsafe.

Also, the Request.Body documentation states:

// The Server will close the request body. The ServeHTTP
// Handler does not need to.

The problem is that the Server does not close the request body if the Handler panics.

@FiloSottile
Copy link
Contributor

Reading from Body after a ServeHTTP Handler returns is not okay, but it should be "get a read error" not okay, not "panic the process" not okay.

I've always interpreted "is not valid" as "might blow up", just like writing to the same memory from two concurrent goroutines is not valid. Why should this one be expected to fail gracefully?

The problem is that the Server does not close the request body if the Handler panics.

I feel like that's a problem, since there might be behavior that replies on the body getting closed, but I don't read the Request.Body documentation as "you can do the thing we said is not valid above because we promise to close the body and you can assume that cleanly interrupts the read".

@neild
Copy link
Contributor Author

neild commented Jul 7, 2021

Why should this one be expected to fail gracefully?

Well, for one thing, it does fail gracefully if you don't panic in a handler.

For another, the way it fails is very bad: You can have two bodies reading from the same TCP connection at the same time. We detect the conflict and panic, but I'd much rather not let that happen. And it doesn't happen...unless you panic in a handler.

And for another, if you look at the RoundTripper implementation, it is very much not obvious to me that it is doing anything wrong: It makes an outbound request using an inbound request body, reads the response to that request, encounters an error, and panics with http.ErrAbortHandler. This all seems fine on the surface, but the outbound request is sneakily still holding on to the request body and may read from it again.

I suppose RoundTripper could close the request body itself (although the Request.Body documentation explicitly says "The ServeHTTP handler does not need to [close the request body]), but why not entirely eliminate this subtle class of bugs by fixing the one corner case where the request body doesn't get closed?

@neild
Copy link
Contributor Author

neild commented Jul 7, 2021

I think the summary of my position here is that if we want to panic when something reads from a Request.Body after the handler has returned, we should do so with an intentional error that says exactly what you did, and we should do it regardless of whether the handler returned normally or panicked. (I think that swallowing HTTP handler panics is unfortunate, but the net/http package explicitly supports a panic as an acceptable way of exiting a handler.)

Currently, we do not panic with an intentional error (we mostly give a really confusing nil dereference after improperly recycling a cached object that still has a live reference via the request body, or occasionally an exciting error resulting from the recycled object now being in use on another connection), and we only do it if the handler panics.

Upgrading the existing error case (where you read from a Request.Body after a normal handler return) to a panic seems like a non-starter, so we should instead make the panic case an error.

@FiloSottile
Copy link
Contributor

Fair enough, if we can consistently make a failure case more predictable/understandable/graceful, I am all for it. We can even change the docs to say something less scary than "is not valid", if you think we can be always graceful with it.

However, that's not a critical nor a security fix, so it's not something we should backport. Especially in code so delicate and complex, shipping a fix without the testing period of the beta and the rc is a real risk.

ReverseProxy is doing something that's not valid, and that leads to a panic in regular operations. That's a critical fix, maybe even a security fix. Unless you think fixing this in ReverseProxy is just too complicated, keeping the backported fix to net/http/httputil feels much safer to me, and then we can change net/http in Go 1.18.

@gopherbot
Copy link

Change https://golang.org/cl/333191 mentions this issue: net/http/httputil: close incoming ReverseProxy request body

@neild
Copy link
Contributor Author

neild commented Jul 7, 2021

CL 333191 fixes the crash in ReverseProxy, although not in an entirely principled fashion--it ensures that the incoming Request.Body is always closed when ReverseProxy.ServeHTTP returns, but it is still possible that an in-flight request will read from the (safely closed) body after the handler returns.

I suppose an actual principled fix would be to wrap the incoming request body with something that lets us ensure the outgoing request RoundTripper will not read from the incoming request body after we're done, but I just counted nine(!!!) levels of wrapping on this request body already and am loath to add another one.

@FiloSottile
Copy link
Contributor

@gopherbot, please open backport issues for CL 333191 as a potential security fix.

@gopherbot
Copy link

Backport issue(s) opened: #47473 (for 1.15), #47474 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

gopherbot pushed a commit that referenced this issue Jul 30, 2021
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.

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>
@gopherbot
Copy link

Change https://golang.org/cl/338550 mentions this issue: [release-branch.go1.15] net/http/httputil: close incoming ReverseProxy request body

@gopherbot
Copy link

Change https://golang.org/cl/338551 mentions this issue: [release-branch.go1.16] net/http/httputil: close incoming ReverseProxy request body

gopherbot pushed a commit that referenced this issue Aug 2, 2021
…y 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 b7a85e0)
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>
gopherbot pushed a commit that referenced this issue Aug 2, 2021
…y 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 #47473
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 b7a85e0)
Reviewed-on: https://go-review.googlesource.com/c/go/+/338550
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>
@toothrot toothrot closed this as completed Aug 5, 2021
@dmitshur dmitshur added this to the Go1.17 milestone Aug 5, 2021
gopherbot pushed a commit that referenced this issue Sep 2, 2021
When recovering from a panic in a HTTP handler, close the request body
before closing the *conn, ensuring that the *conn's bufio.Reader is safe
to recycle.

Fixes #46866.

Change-Id: I3fe304592e3b423a0970727d68bc1229c3752939
Reviewed-on: https://go-review.googlesource.com/c/go/+/329922
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@rsc rsc unassigned neild Jun 23, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants