AdguardTeam / gomitmproxy

Simple golang mitm proxy implementation
https://adguard.com/
GNU General Public License v3.0
273 stars 53 forks source link

In Proxy.handleRequest, check error before dereferencing request #21

Closed stevevls closed 3 months ago

stevevls commented 8 months ago

This fixes a segfault in the case where the caller does not trust the CA.

Fixes #20

Can be reproduced with curl:

$ curl -v -x https://user:pass@127.0.0.1:3333/ https://www.google.com/
*   Trying 127.0.0.1:3333...
* Connected to 127.0.0.1 (127.0.0.1) port 3333 (#0)
* ALPN: offers http/1.1
* (304) (OUT), TLS handshake, Client hello (1):
*  CAfile: /etc/ssl/cert.pem
*  CApath: none
* (304) (IN), TLS handshake, Server hello (2):
* (304) (IN), TLS handshake, Unknown (8):
* (304) (IN), TLS handshake, Certificate (11):
* SSL certificate problem: self signed certificate in certificate chain
* Closing connection 0
curl: (60) SSL certificate problem: self signed certificate in certificate chain
More details here: https://curl.se/docs/sslcerts.html

curl failed to verify the legitimacy of the server and therefore could not
establish a secure connection to it. To learn more about this situation and
how to fix it, please visit the web page mentioned above.

Before:

$ go run examples/mitm/main.go 
2023/10/20 11:56:57 55262#1 [debug] mitm: cache miss for 127.0.0.1
2023/10/20 11:56:57 55262#6 [info] start listening to [::]:3333
2023/10/20 11:56:59 55262#6 [debug] id=100001: accepted connection from 127.0.0.1:58335
2023/10/20 11:56:59 55262#9 [debug] id=100001: waiting for request
2023/10/20 11:56:59 55262#10 [debug] id=100001: failed to read request: remote error: tls: unknown certificate authority
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x40 pc=0x10468f04c]

goroutine 9 [running]:
github.com/AdguardTeam/gomitmproxy.(*Proxy).handleRequest(0x140001b2200, 0x1400006a940)
    /Users/steve/go/github.com/gomitmproxy/proxy.go:232 +0x3c
github.com/AdguardTeam/gomitmproxy.(*Proxy).handleLoop(0x140001b2200, 0x0?)
    /Users/steve/go/github.com/gomitmproxy/proxy.go:220 +0x70
github.com/AdguardTeam/gomitmproxy.(*Proxy).handleConnection(0x140001b2200, 0x1400006a940)
    /Users/steve/go/github.com/gomitmproxy/proxy.go:208 +0x10c
created by github.com/AdguardTeam/gomitmproxy.(*Proxy).serve
    /Users/steve/go/github.com/gomitmproxy/proxy.go:191 +0x34
exit status 2

After:

$ go run examples/mitm/main.go
2023/10/20 12:00:32 72781#1 [debug] mitm: cache miss for 127.0.0.1
2023/10/20 12:00:32 72781#7 [info] start listening to [::]:3333
2023/10/20 12:00:34 72781#7 [debug] id=100001: accepted connection from 127.0.0.1:58585
2023/10/20 12:00:34 72781#9 [debug] id=100001: waiting for request
2023/10/20 12:00:35 72781#10 [debug] id=100001: failed to read request: remote error: tls: unknown certificate authority
2023/10/20 12:00:35 72781#9 [debug] id=100001: closing connection due to: remote error: tls: unknown certificate authority
robbyt commented 4 months ago

This seems like a very reasonable fix.

@ameshkov is this proxy code still being maintained? or are you looking for a new maintainer?

ameshkov commented 4 months ago

Sorry, the fix is indeed reasonable, I'll see it to be merged.

@robbyt the project needs a bit more care, but it'll soon receive it no worries:)

ameshkov commented 3 months ago

Uh, I made a mistake when merging it in the internal Git repo, didn't keep the original commits :( https://github.com/AdguardTeam/gomitmproxy/commit/51bf9628e86ce0d3cfd39ffbc1283f55f8c7173c

Sorry for that and thank you for the contribution!