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
Do local URI verification, while attempting to defuse SSRF #879
Conversation
087f8dc
to
75ab1f1
Compare
pkg/detectors/mongodb/mongodb.go
Outdated
@@ -21,7 +21,7 @@ var _ detectors.Detector = (*Scanner)(nil) | |||
|
|||
var ( | |||
// Make sure that your group is surrounded in boundary characters such as below to reduce false positives. | |||
keyPat = regexp.MustCompile(`\b(mongodb(\+srv)?://(?:[^:]+:(?:[^@]+)?@)?(?:[^/]+|/.+.sock?,?)+(?:/([^/."*<>:|?]*))?\?(?:(.+=?=\S+)&?)+)\b`) | |||
keyPat = regexp.MustCompile(`\b(mongodb(\+srv)?://[-.%\w{}]{1,50}:([-.%\S]{3,50})@[-.%\w\/:]+)\b`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous regex was a bit too inclusive and exclusive, sometimes grabbing html nearby and sometimes missing things that were picked up by this pattern, which is a modified version of the URI pattern but only for the mongodb scheme.
4b25b51
to
e6de528
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to add the two new detectors to the default detectors?
return results, nil | ||
} | ||
|
||
func verifyFTP(ctx context.Context, u *url.URL) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We can remove ctx as an arg since it doesn't look to be used.
return results, nil | ||
} | ||
|
||
func verifyRedis(ctx context.Context, u *url.URL) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can remove ctx as an arg here if unused.
keyPat = regexp.MustCompile(`\b[a-zA-Z]{1,10}:?\/\/[-.%\w{}]{1,50}:([-.%\S]{3,50})@[-.%\w\/:]+\b`) | ||
keyPat = regexp.MustCompile(`\b(?:http(?:s)?:)?\/\/[\S]{3,50}:([\S]{3,50})@[-.%\w\/:]+\b`) | ||
|
||
client = common.SaneHttpClient() | ||
) | ||
|
||
type proxyRes struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know you didn't touch this, but looks like we no longer use this struct. We can probably remove?
return false | ||
} | ||
|
||
time.Sleep(time.Millisecond * 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤣
…curity#879) * simplify monogo pattern * do URI verification locally, while attempting to defuse SSRF * test SSRF defuse * simplify err check logic per linter recommendation * split up detectors * address comments * remove unused var
Previously we used the SSRF proxy to mitigate SSRF concerns of potential malicious payloads. However this meant that URI credentials were sent up to the proxy, and verification only worked if the resource was externally accessible.
This change performs verification locally to the scanner, after attempting to defuse http and https payloads. It does so with the following 3 modifications to the detected URI:
Additionally it verifies using a GET request.
So if there is a blind SSRF payload on a state-changing GET endpoint with no filepath, it could still fire, but otherwise it should be defused.
The SSRF proxy also previously verified redis and ftp, and these have been split up into new detectors.