-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[SECURITY] Improve SSRF checks, strict path check for well_known_path #2510
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
Conversation
👋 @ajinabraham |
return False | ||
|
||
prefixs = ('http://', 'https://') | ||
if not host.startswith(prefixs): |
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.
Why not. Use host.scheme here?
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.
host is not parsed yet, we can have hosts with user controlled scheme or no scheme at all, So forcing HTTP scheme on it.
|
||
# Resolve dns to get ipv4 or ipv6 address | ||
socket.setdefaulttimeout(5) # 5 second timeout | ||
ip_addresses = socket.getaddrinfo(hostname, None) |
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.
Each time this is called a DNS rebinding is possible
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.
Internal port scanning is still possible with the next urls, the ip_addresses = socket.getaddrinfo(hostname, None)
does issue a DNS query thus permitting the host to switch from 1.1.1.1:80
to 127.0.0.1:80
- "prefix1-make-1.1.1.1-rebindfor30safter1times-127.0.0.1-rr.1u.ms:80"
- "prefix1-make-1.1.1.1-rebindfor30safter1times-127.0.0.1-rr.1u.ms:81"
- ....
- "prefix1-make-1.1.1.1-rebindfor30safter1times-127.0.0.1-rr.1u.ms:443"
measuring the response time would yield if the port is open or not.
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 PR doesn't do anything against DNS rebind. We cannot removesocket.getaddrinfo
as it's required to translate IPs from hostnames dynamically to validate against a known private/internal list.
I did some overall refactoring to SSRF checks and enforced strict hardcoded path check for the 2 places we invoke user controlled requests.
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.
Since firebase and assetlink looks at https://, or http://, I can also do a port check.
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.
how about using the initially retrieved value of socket.getaddreinfo
to avoid the DNS rebind ?
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.
We cannot rely on the IP alone to resolve a resource. It depends on the virtual hosting or domain based routing configurations of the server.
a single IP address can host different resources at the same path across multiple domain names.
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.
indeed, but after the resolving of socket.getaddrinfo()
the next call to .get()
would point to the second DNS rebinded host, thus still vulnerable to a certain extent with a limited impact.
Regards
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.
Yes, I acknowledge that this PR does not address the DNS rebind issue. A potential fix could involve monkey patching requests/urllib3 or implementing a custom HTTP request library with fixed domain name and IP. However, I am hesitant to add this unless there is a medium/high impact real-world exploit case. The current checks in place offer a balanced approach to mitigating the SSRF issue and its primary exploit use cases.
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.
I see now, anyway, keep me posted about when you would release the advisory. Thank you
Describe the Pull Request
Checklist for PR
tox -e lint,test
StaticAnalyzer/tests.py
)Additional Comments (if any)