Skip to content

Commit

Permalink
Fix data race in MITM'ed HTTPS request handling
Browse files Browse the repository at this point in the history
This data race requires quite of few conditions to appear.

1) The transport has to have HTTP/2 enabled (in goproxy, it's not by default).
2) The target server must be an HTTP/2 one.
3) The request must have a body.
4) The server must return certain erroneus HTTP status code.
5) The client must ignore Connection: close header from the proxy
   (4ec7933).

Somehow, I managed to hit all these conditions, and got this error:

    panic: runtime error: slice bounds out of range [96:48]

    goroutine 341699 [running]:
    bufio.(*Reader).ReadSlice(0xc000b0c1e0, 0x80?)
        /usr/local/go/src/bufio/bufio.go:347 +0x225
    bufio.(*Reader).ReadLine(0xc000b0c1e0)
        /usr/local/go/src/bufio/bufio.go:401 +0x27
    net/textproto.(*Reader).readLineSlice(0xc0013400f0)
        /usr/local/go/src/net/textproto/reader.go:56 +0x99
    net/textproto.(*Reader).ReadLine(...)
        /usr/local/go/src/net/textproto/reader.go:39
    net/http.readRequest(0xc000847d40?)
        /usr/local/go/src/net/http/request.go:1042 +0xba
    net/http.ReadRequest(0xc000b0c1e0?)
        /usr/local/go/src/net/http/request.go:1025 +0x19
    github.com/elazarl/goproxy.(*ProxyHttpServer).handleHttps.func2()
        /root/go/pkg/mod/github.com/elazarl/goproxy@v0.0.0-20220901064549-fbd10ff4f5a1/https.go:221 +0x3db
    created by github.com/elazarl/goproxy.(*ProxyHttpServer).handleHttps
        /root/go/pkg/mod/github.com/elazarl/goproxy@v0.0.0-20220901064549-fbd10ff4f5a1/https.go:211 +0x611

Despite 5), goproxy should not break from non-compliant HTTP clients,
and "Connection: close" is more like of an accidental fix.

See also golang/go#61596 (comment)
  • Loading branch information
WGH- committed Jul 26, 2023
1 parent a0805db commit a83dba5
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion https.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,12 @@ func (proxy *ProxyHttpServer) handleHttps(w http.ResponseWriter, r *http.Request
return
}
removeProxyHeaders(ctx, req)
resp, err = ctx.RoundTrip(req)
resp, err = func() (*http.Response, error) {
// explicitly discard request body to avoid data races in certain RoundTripper implementations
// see https://github.com/golang/go/issues/61596#issuecomment-1652345131
defer req.Body.Close()
return ctx.RoundTrip(req)
}()
if err != nil {
ctx.Warnf("Cannot read TLS response from mitm'd server %v", err)
return
Expand Down

0 comments on commit a83dba5

Please sign in to comment.