elazarl / goproxy

An HTTP proxy library for Go
BSD 3-Clause "New" or "Revised" License
5.89k stars 1.07k forks source link

Fix data race in MITM'ed HTTPS request handling #509

Closed WGH- closed 10 months ago

WGH- commented 11 months ago

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 (4ec79339bd51dec356e283d8c8de29748b42b1fe).

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 https://github.com/golang/go/issues/61596#issuecomment-1652345131

elazarl commented 11 months ago

@WGH- can you please rebase?

WGH- commented 10 months ago

Done!

BTW, I tried to write a regression test for it, but it turned out to be quite complicated (given Connection: close and all). So I gave up.