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

dep lib change goproxy->martian #238

Merged
merged 12 commits into from
Apr 9, 2023
Merged

dep lib change goproxy->martian #238

merged 12 commits into from
Apr 9, 2023

Conversation

tarunKoyalwar
Copy link
Member

@tarunKoyalwar tarunKoyalwar commented Feb 21, 2023

Proposed changes

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@tarunKoyalwar tarunKoyalwar linked an issue Feb 21, 2023 that may be closed by this pull request
@tarunKoyalwar tarunKoyalwar marked this pull request as draft February 21, 2023 21:28
func (p *Proxy) getRoundTripper() (http.RoundTripper, error) {
roundtrip := &http.Transport{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
if len(p.options.UpstreamHTTPProxies) > 0 {
roundtrip = &http.Transport{Proxy: func(req *http.Request) (*url.URL, error) {
return url.Parse(p.rbhttp.Next())
}, TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
socks5Dialer := socks5Dialers[socks5Proxy]
// use it to perform the request
return socks5Dialer.Dial(network, addr)
}, TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
@Mzack9999 Mzack9999 self-requested a review February 24, 2023 16:14
@tarunKoyalwar tarunKoyalwar removed the request for review from Mzack9999 March 6, 2023 05:36
@tarunKoyalwar
Copy link
Member Author

tarunKoyalwar commented Mar 6, 2023

while proxify is good it feels too invasive i.e errors cause domain/website to not render at all or correctly

Setup

  1. proxify -ha "127.0.0.1:8080" -v
  2. install proxify CA in browser and proxy traffic

With goproxy lib i.e before this PR

Screenshot 2023-03-06 at 11 10 16 AM

with martian lib i.e in this PR

Screenshot 2023-03-06 at 11 12 15 AM

As Shown in above images . there does not seem to be a stable and robust library but in comparison martian has less errors .

What are these errors ?

tls handhshake / other tls failure (only occurs with goproxy) . random io.EOF errors and CONNECT request failures

Anything else

what's wierd is that after these errors we close connection (net.Conn) but browser request is not failed/closed instead is failed after some time due to timeout

@tarunKoyalwar
Copy link
Member Author

Errors obtained while debugging

./proxify -ha "127.0.0.1:8080" -v 

                       _ ___    
   ___  _______ __ __ (_) _/_ __
  / _ \/ __/ _ \\ \ // / _/ // /
 / .__/_/  \___/_\_\/_/_/ \_, / 
/_/                      /___/	v0.0.8

		projectdiscovery.io

[INF] HTTP Proxy Listening on 127.0.0.1:8080
[INF] Socks5 Proxy Listening on 127.0.0.1:10080
[INF] Saving proxify traffic to logs
2023/03/06 11:17:50 ERROR: martian mitm: error peeking message through CONNECT tunnel to determine type dns.projectdiscovery.io:443 : EOF
2023/03/06 11:17:50 fragment values is 0
2023/03/06 11:17:50 got connect request but not a tls handshake
2023/03/06 11:17:50 
-------------
last successful parsed req and resp before error was
`<nil>`
`<nil>`
2023/03/06 11:17:50 got `malformed HTTP request "\x00"` data read from raw conn "\x00"
2023/03/06 11:17:50 ERROR: martian: failed to read request: got `malformed HTTP request "\x00"` data read from raw conn "\x00"
2023/03/06 11:17:50 martian: closing connection: 127.0.0.1:52595
2023/03/06 11:17:50 
-------------
last successful parsed req and resp before error was
`&{CONNECT https://snid.snitcher.com:443 HTTP/1.1 1 1 map[Connection:[keep-alive] Proxy-Connection:[keep-alive] User-Agent:[Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:109.0) Gecko/20100101 Firefox/110.0]] {} <nil> 0 [] false snid.snitcher.com:443 map[] map[] <nil> map[] 127.0.0.1:52582 snid.snitcher.com:443 <nil> <nil> <nil> <nil>}`
`<nil>`
2023/03/06 11:17:50 got `EOF` data read from raw conn ""
2023/03/06 11:17:50 martian: closing connection: 127.0.0.1:52582
2023/03/06 11:17:55 
-------------
last successful parsed req and resp before error was
`<nil>`
`<nil>`
2023/03/06 11:17:55 got `EOF` data read from raw conn ""
2023/03/06 11:17:55 martian: closing connection: 127.0.0.1:52600
2023/03/06 11:17:55 
-------------
last successful parsed req and resp before error was
`<nil>`
`<nil>`
2023/03/06 11:17:55 got `EOF` data read from raw conn ""
2023/03/06 11:17:55 martian: closing connection: 127.0.0.1:52588
2

@tarunKoyalwar
Copy link
Member Author

tarunKoyalwar commented Mar 6, 2023

Verdict

unlike goproxy , martian lib is simple enough with good/proper implementation (all logic we need is in proxy.go file) . If we can fix these errors we can submit a patch upstream . or embed in proxify / fork and remove all bloated logic

Root Cause of these Errors

the root cause of these errors seems to be reading/handling net.Conn . All above shown errors are caused in interaction between browser <-> proxy

@tarunKoyalwar
Copy link
Member Author

@Mzack9999

Debugging and Fixing Errors

  • Fork of martian with some bug fixes and some modifications for verbosity is available at https://github.com/tarunKoyalwar/martian . clone it
  • clone the pr branch and replace directory mentioned in go.mod under replace directive with path of above forked martian

Leads

  • errors originate in martian so we only need to fix/debug martian
  • All required logic is present in proxy.go file in martian .Possible leads / starting out in martian proxy.go file
    • .Serve() -> .handleLoop() -> .handle() -> .readrequest()

@tarunKoyalwar tarunKoyalwar requested a review from Mzack9999 March 6, 2023 06:05
@Mzack9999 Mzack9999 added the Type: Enhancement Most issues will probably ask for additions or changes. label Mar 6, 2023
@Mzack9999 Mzack9999 marked this pull request as ready for review March 29, 2023 15:56
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementation: lgtm
needs a final cross-check if any furher finalization is needed

@tarunKoyalwar
Copy link
Member Author

TODO

Note: Most of changes in PR are breaking changes

@tarunKoyalwar
Copy link
Member Author

@ehsandeep , After some testing it seems like it is almost stable now only malformed chunked encoding is blocker apart from that there weren't issues . (tested with more than 20k live traffic on different sites)

@ehsandeep ehsandeep merged commit 0a542bf into dev Apr 9, 2023
@ehsandeep ehsandeep deleted the issue-174-martian-proxy branch April 9, 2023 07:12
@djallalzoldik
Copy link

I use the lateast version of proxify but steel have the same error, the certificate not work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace unmaintained goproxy with martian
4 participants