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

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

Closed

Conversation

stevevls
Copy link

@stevevls stevevls commented Oct 20, 2023

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:[email protected]: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

This fixes a segfault in the case where the caller does not trust the CA
@robbyt
Copy link

robbyt commented Feb 6, 2024

This seems like a very reasonable fix.

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

@ameshkov
Copy link
Member

ameshkov commented Feb 7, 2024

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
Copy link
Member

ameshkov commented Mar 11, 2024

Uh, I made a mistake when merging it in the internal Git repo, didn't keep the original commits :(
51bf962

Sorry for that and thank you for the contribution!

@ameshkov ameshkov closed this Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault if client does not trust the proxy's SSL certificate
3 participants