How to prevent SSRF

[AndrewFarley]

Describe the bug

I'm integrating the got framework as a foundational part of some upcoming services we're launching and there's one potential attack vector that we need to resolve that I believe needs to be resolved in this library. The vector is that there is no way (that I can figure out from the documentation) in the got library to automatically fail/reject if it is requested to resolve a DNS name to a local IP address.

Actual behavior

import got from 'got';
const url = 'http://url-that-resolves.to-an-internal-ip.com/';
const options = {};
const data = await got(url, options).json();

In this simplified example code, lets say I have a system that would allow clients to send webhooks to a customer-specified endpoint any time certain actions occurred. This model is fairly common in SaaS services currently. An attacker would control this DNS name and would set to an internal IP address in order to potentially probe a SaaS service's internal LAN network. The dns name url-that-resolves.to-an-internal-ip.com would resolve to, say 10.1.1.1 and would cause this webhook to attempt to talk to a local service potentially leaking critical data.

Expected behavior

What would be really critical and important, would be to allow an option to be specified to auto-fail without making the request if the DNS resolved to an local IP address range: Local IP Address Ranges: 10.0.0.0-10.255.255.255, 172.16.0.0-172.31. 255.255, 192.168.0.0-192.168.255.255

import got from 'got';
const url = 'http://url-that-resolves.to-an-internal-ip.com/';
const options = {
    allow_local: false
};
const data = await got(url, options).json();

In this above scenario, simply setting the to-be-engineered allow_local flag to false (which is True by default) would cause this library to perform the typical DNS lookup to figure out where to connect to, but when that resolves to a local IP address, it would fail throwing an exception as other type of failures with this library would throw.

Potential workarounds;

I realize there are potential workarounds for this such as before passing to got() I do a DNS lookup myself and verify that it's not an local IP. That avenue seems to not be the right place to fix this problem, one would argue. Because in that scenario I would be looking up a DNS record to connect to 2x as many times as if I let got() handle this logic internally.

Thoughts???

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got, and this problem occurs and no workaround is available

[sindresorhus]

I'm no expert when it comes to this, but I never seen such an option in a HTTP request library. I feel like this should be handled at different level, for example, with a system firewall, network segmentation, network DNS rules, etc.

[szmarczak]

@sindresorhus is right. Either you want to use a custom DNS server, or restrict DNS rules.

[AndrewFarley]

Thanks for the rename, that is an accurate description of the underlying issue, and can't tell you how many times from audits or pen tests have had to fix this issue. It's why I'm raising it here.

@sindresorhus / @szmarczak while it is possible to do at another layer, that other layer would require a level of unnecessary added complexity. As you've stated some ideas include...

• Spin up a custom DNS forwarder which rejects anything which resolves to internal IPs. I've been engineering microservices and at dev shops the last 20 years, I've never seen someone host DNS and I'd think this would be a huge SPOF as well. So I'm writing this off as a bad-idea. While it is possible, it's not feasible or recommended or within' the scope of what I would consider best-practice for most.
• At the firewall level. I would argue this is impossible, as in a typical microservices framework within' a network, any given service typically has access to some or many other internal services at internal IP addresses. How can I firewall something that it needs to be able to use? If I were to, say, allow users to give me a webhook url to call to trigger an action, even if I detected it at object creation time it wasn't an internal IP, they could change the DNS with their registrar afterwards to resolve to an internal and do some tricky probing of an internal infrastructure (hence, SSRF)
• At the application level before making every request I could fetch the IP(s) a DNS resolves to and reject if one of them is an internal IP. Now, while this is possible, this would add unnecessary complexity to an application and would require at least one additional DNS lookup (which arguably, could/should be cached upon the actual request made via this library, but historically when I've done implementations of this different disparate libraries query DNS differently causing it to not cache the request). For now, this is largely what people have to do, here's an example of someone sharing how to do this in Python: https://stackoverflow.com/a/44606903/2334036 which is required because afaict the requests library does not have this feature (as you are stating and arguing for not having).

Because some/many libraries like this one do not have this feature and many folks don't see the merit in such a feature, this falls on engineering teams to implement a DiY application-specific logic in every application out there to prevent such attacks needlessly when it could be in the underlying library. This leads down the path of possibly insecure solutions. If this feature was in more lower level frameworks, like this one, and if it were shared and/or possibly even documented and possibly enabled by default to be more security-first focused I think it would help us as an industry make better, more secure software.

This is not an unheard of request or feature, an example Python package I know and use has this feature: https://validators.readthedocs.io/en/latest/#validators.url.url . And while it's true, this isn't in a request library, I don't see why it couldn't/shouldn't be.

With this idea in mind, would you accept and merge a PR adding an optional flag for this feature? This feature would...

• Be a flag/argument similar to the validators.url example above called public which has 3 valid states
• This variable would be equal to null by default, can be set to true or false as well
• If this argument is set to null it would cause no change in any logic in this library. For an initial merging of this code/feature, this would likely be the default.
• If this argument is set to true then if the IP address it is attempting to connect to is an private IP, it will throw an exception before the connection is attempted, otherwise will act normally. Ideally, in a future version of this library, this would be the new default. This would make this library more serious about security, effectively disallowing SSRF by default.
• If this argument is set to false then if the IP address it resolves to is anything public, it will throw an exception before the connection is attempted, otherwise will act normally. This is probably the rarest edge/use case, however I could see this being set in a high-sec type of environment where they want to guarantee their services and traffic are staying internally and not even attempting to talk to any IPs externally. This gives a contractual guarantee of that fact, without the end-user having to write code (such as the stackoverflow example) in every codebase to facilitate this feature.

[szmarczak]

Hm. I just remembered that a server can return a Location header that would return 127.0.0.1. In that case, changing DNS servers does not help.

CloudFlare solves the issue by running Workers in a container with no network access. All internal communication is done via UNIX sockets.

CloudFlare's approach fixed the possibility of an RCE where the attacked would spawn e.g. curl 127.0.0.1/... (or any other command) to bypass Node.js networking stack.

I think you could run the app in a container with no internal HTTP services (network access can be allowed), they would be exposed via UNIX sockets instead.

[szmarczak]

@AndrewFarley https://replit.com/@szmarczak/UncomfortableAttachedMode#index.mjs

Please take the solution above with a grain of salt. Malicious actors could potentially try to work around this by clearing import cache. Meanwhile it's possible to clear cache for require, I do not know of any way to do so for imports though. That does not mean that it's impossible to bypass this. Pitfall: the solution relies on async contexts, which can disappear when using queueMicrotask or any other deferring function. https://nodejs.org/api/async_context.html#troubleshooting-context-loss

The correct solution here would be to use containers.

If you don't want to use containers, then you're looking for a permission system, which Node.js lacks.

[AndrewFarley]

@szmarczak This has nothing to do with location headers, and 100% to do with the IP that we are trying to connect with. I think your comments are misplaced? What I am proposing above, is the only way to combat SSRF when using end-user-supplied hostnames/URLs. I'm unsure how anything you've shared about Location headers has anything to do with this topic whatsoever. Please re-read, digest and come back at me. If this PR would be accepted, either I or another engineer of mine may pursue this.

[szmarczak]

This has nothing to do with location headers, and 100% to do with the IP that we are trying to connect with. I think your comments are misplaced? I'm unsure how anything you've shared about Location headers has anything to do with this topic whatsoever.

They are not misplaced, as Got by default follows redirects.

If this PR would be accepted, either I or another engineer of mine may pursue this.

Your proposed solution is already possible to do via a beforeRequest hook (for when it's a direct request to an IP address) + dnsLookup option (for when it's a domain name).

this would add unnecessary complexity to an application and would require at least one additional DNS lookup

No, because you would wrap the lookup function that would just perform an additional if/throw before calling the callback.

---

The solution you're proposing is definitely at the wrong layer. Got should not be responsible for who are you trying to connect with. Nor does any other HTTP client in Node.js. Please acknowledge this comment. As I have said, Node.js lacks a permission system. In the replit example I have modified Node.js internals (TCPWrap.connect and TCPWrap.connect6) to disallow connections in the block list of the current context.

[AndrewFarley]

@szmarczak I didn't know there was a beforeRequest hook before now, so thanks. That seems like it would be possible to use this library and to then creatively prevent SSRF.

However, I would disagree on your point that this library could/should support this, as I've said, effectively, anyone using this library for a specific use-case (to handle customer-requested endpoints eg webhooks) would basically need to re-implement the same logic over and over with the same beforeRequest hook. Are there some complexities to discuss/agree upon in regards to how this relates to redirects and what not? Sure. But something being challenging doesn't mean it shouldn't be done. This to me seems like myopic view of software engineering, and is effectively the opposite of the ideals of open-source.

If no plans to accept a PR for this, then close this if you want as "wont-fix" with my disagreement and disapproval

[szmarczak]

would basically need to re-implement the same logic over and over with the same beforeRequest hook.

This applies to every HTTP client in Node.js, not just Got. The more proper solution would be do this in Node.js, however since it lacks permission the only work around is to monkey patch internals like in the replit example.

[szmarczak]

@AndrewFarley There are more transport layers based other than HTTP that are also based on TCP. It does not make sense to duplicate SSRF protection for every TCP-like transport client library. It should be at another layer for a one fits all solution.

[szmarczak]

Feel free to open an issue in the Node.js repo. I'm more than happy to chime in there.