net/http: panic due to racy read of persistConn after handler panic #46866
Comments
|
Change https://golang.org/cl/329922 mentions this issue: |
seankhliao
added
the
NeedsFix
|
I am not convinced this is a net/http issue. The
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? |
|
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 problem is that the Server does not close the request body if the Handler panics. |
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?
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". |
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 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? |
|
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. |
|
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. |
|
Change https://golang.org/cl/333191 mentions this issue: |
|
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. |
|
@gopherbot, please open backport issues for CL 333191 as a potential security fix. |
|
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. |
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>
|
Change https://golang.org/cl/338550 mentions this issue: |
|
Change https://golang.org/cl/338551 mentions this issue: |
…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>
…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>
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>
Let's start with the reproduction case:
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 topanic(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 itsbufio.Readerandbufio.Writerto theirsync.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.The text was updated successfully, but these errors were encountered: