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

Make it easier to prevent Server side request forgery (SSRF) attacks #141

Open
stanhu opened this issue Jun 1, 2023 · 4 comments
Open

Comments

@stanhu
Copy link

stanhu commented Jun 1, 2023

Over the years, we've received many bug bounty reports relating to Server side request forgery (SSRF) attacks. In a nutshell, these attacks use short-lived DNS entries to direct Web hooks and other URLs to internal IP addresses, such as AWS's instance metadata endpoint.

To a large extent, the problem is mitigated by using HTTPS, since a SSL certificate Common Name (CN) must match the hostname. However, there are a number of edge cases where HTTPS doesn't solve the issue. For example:

  • DNS rebinding might still enable reconnaissance on the local network, since errors show the difference between "10.1.2.3:4567 unreachable" and "10.1.2.3:4567 reachable but TLS error".
  • Some clients or Web hooks may disable SSL certificate verification.

In the past, we've mitigated the problem by:

  1. Performing a DNS lookup first for the IP address.
  2. If the IP address maps to internal or local networks, reject the request.
  3. If the IP address is allowed, make the HTTPS request with the IP address instead of the hostname. To ensure SNI works, we patched net-http to use the original hostname by overriding the hostname= method.

A similar approach is taken by ssrf_filter.

However, with #36, our net-http patch no longer works because hostname= isn't called when an IP address is used. To handle that, arkadiyt/ssrf_filter#54 introduced an even uglier patch that overrides the Resolv equality methods.

Both hostname= and Resolv patches are a bit ugly, but short of patching the #connect method there's no alternative at the moment.

A better approach might be to invoke some callback in #connect that will allow the caller to resolve the hostname and decide whether the connection should still proceed.

I realize that others might argue that a proxying all external calls via a proxy server is ultimately the right approach, but that's another moving part that requires more setup.

@jeremyevans What do you think about this?

@jeremyevans
Copy link
Contributor

I would be open to possible refactoring/method extraction so that you could override methods to get the behavior you want. However, understand that I am not the net-http maintainer, so you should get @nurse's opinion.

@nurse
Copy link
Member

nurse commented Oct 5, 2023

I once discussed about a hook for DNS resolution, but failed to get a consensus. I'll discussed about this topic again in a next developer meeting.

stanhu added a commit to stanhu/net-http that referenced this issue Jan 10, 2024
The method was getting large, and in preparation for ruby#141, I thought
it would be easier to break up the method into several other methods.
stanhu added a commit to stanhu/net-http that referenced this issue Jan 10, 2024
The method was getting large, and in preparation for ruby#141, I thought
it would be easier to break up the method into several other methods.
stanhu added a commit to stanhu/net-http that referenced this issue Jan 10, 2024
The method was getting large, and in preparation for ruby#141, I thought
it would be easier to break up the method into several other methods.
stanhu added a commit to stanhu/net-http that referenced this issue Jan 10, 2024
The method was getting large, and in preparation for ruby#141, I thought
it would be easier to break up the method into several other methods.
@stanhu
Copy link
Author

stanhu commented Aug 8, 2024

I should note that Golang supports something similar: https://www.agwa.name/blog/post/preventing_server_side_request_forgery_in_golang

Reference: golang/go#55301.

@collimarco
Copy link

I totally agree with @stanhu

An easier solution to protect from SSRF is really necessary.

@nurse Did you get any update on this issue in the developer meeting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants