SSRF protection in undici / native-node-fetch

ronjouch commented 1 year ago

This would solve...

undici and native-node>=18-fetch expose (as far as I know, and see documented) no way to protect against SSRF attacks when making server-side calls to arbitrary / user-controllable URLs (that are normally WWW, but could be abused to try to exfiltrate info from an internal network).

In the nodejs ecosystem,

Non-fetch libs like request or axios can use middleware like ssrf-req-filter. It provides an agent / httpAgent / httpsAgent that you can pass in these libs second options argument:

const ssrfFilter = require('ssrf-req-filter');
const url = 'http://169.254.169.254/latest/user-data/iam/security-credentials/' // your choice of sensitive "internal" URL here
axios.get(url, { httpAgent: ssrfFilter(url), httpsAgent: ssrfFilter(url) }) // <-- SSRF "PLUGIN" ADDED HERE
    .then((response) => { console.log(`Success', response) });
    .catch((error) => { console.log('Error', error); })

3rd-party fetch lib node-fetch also supports ssrf-req-filter, though a similar agent API:

const ssrfFilter = require('ssrf-req-filter');
const nodefetch = require("node-fetch");
const url = 'http://169.254.169.254/latest/user-data/iam/security-credentials/' // your choice of sensitive "internal" URL here
nodefetch(url, { agent: ssrfFilter(url) }) // <-- SSRF "PLUGIN" ADDED HERE
    .then((response) => { console.log(`Success', response) });
    .catch((error) => { console.log('Error', error); })

The implementation of such SSRF protection is out of topic, but:

ssrf-req-filter's "TL;DR" is this line ...
... which depends on ipaddr.js categorizing addresses according to various RFCs

Given the above, my question:

In undici / built-in-node>=18 native fetch, I see no trace of supporting such agents with a ({host: string}) => boolean signature letting me decide at runtime to cancel a request.

Is it actually unsupported, with no plan to support it? If so, what do you (undici/native-node-fetch maintainers) recommend to do to protect against SSRF? To make the SSRF check before calling fetch? (At the cost of an extra DNS resolution before calling fetch)
Is it unsupported in browser fetch, but potentially coming to server-side node fetch at some point?
Or is it supported but lacks documentation (or I missed it 😄?)

The implementation should look like...

undici (fetch?) should support setting kind of option letting users plug anti-SSRF logic.

Typically, an agent function of signature {host: string} => boolean (agent is passed an IP, and expected to return a boolean).

I have also considered...

I searched undici's open & closed issues for ssrf, nothing. Grepping for agent yields many things but it looks like there's a naming collision around what is an agent in your context vs. in an axios/request context. They don't seem to have similar responsibilities.

I searched undici's discussions for ssrf, nothing.

I read the options supported by standard fetch in mdn / fetch, nothing in the documented options I could find related to agent / ssrf stuff.

I grepped the fetch WHATWG spec for "SSRF", without success.

Additional context

Thanks for fetch!

mcollina commented 1 year ago

This module does not claim to have this protection in any way.

I think it should be possible to implement this with an interceptor: https://github.com/nodejs/undici/blob/main/docs/api/DispatchInterceptor.md.

We might want to develop some mitigation via a custom agent. wdyt @RafaelGSS @ronag?

ronjouch commented 1 year ago

@mcollina thx for the fast reply!

This module does not claim to have this protection in any way.

Okay. Not surprised, given that fetch comes foremost from a browser world.

I think it should be possible to implement this with an interceptor: https://github.com/nodejs/undici/blob/main/docs/api/DispatchInterceptor.md . We might want to develop some mitigation via a custom agent.

👍, super interested in this discussion. But in an interceptor world, if I read correctly your types at https://github.com/nodejs/undici/blob/7276126945c52cf7ec61460b36d19f882e1bab82/types/dispatcher.d.ts#L96-L98 , I see that a DispatchInterceptor is being fed a DispatchOptions with an origin URL and path, whereas I need a host/IP. So, from my understanding, I'd still have to do my own dns lookup here (which undici will repeat afterwards), which means this API isn't exactly set up at the right point I need in the request lifecycle (after { DNS resolution, redirects followup }, before firing the request) 😕. Am I correct?

In the meantime, and putting aside interceptors, my plan was to do the SSRF check manually before calling fetch, repeating what ssrf-req-filter does with help from ipaddr.js:

const ip = await require('dns').promises.lookup(maybeMaliciousUrl).address;
const isSuspicious = require('ipaddr.js').parse(ip).range() !== 'unicast';
if (isSuspicious) { throw 'why u ssrf'; }

I actually like this thing, it's straightforward and explicit, more than the plugins.

However, I dislike that it forces an extra dns lookup, whereas a "plugin" agent solution integrated to fetch (similar to what request & axios do) does the lookup, and feeds it the agent/interceptor the IP, eliminating the extra dns resolution.

Any opinion/comment about it? Or should I just not care because the dns lookup fast/cached?

mcollina commented 1 year ago

I think we might have to provide a checkup option in Client for you to plug into.

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

ronjouch commented 1 year ago

I think we might have to provide a checkup option in Client for you to plug into.

@mcollina not sure I understand you: what do you mean by a checkup option in Client ?

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina considering it. As mentioned in https://github.com/nodejs/undici/issues/2019#issuecomment-1478534997 , I except for my DNS resolve question I also somehow like a dumb pre-call check (wrappable in a helper function). But I don't fully understand yet how e.g. request does its agent thing, and if it's anymore DNS-lookup-efficient than a dumb pre-call check.

I'll be researching what request does, and if still convinced by the agent approach, will give a PR a try. No commitment as long as I have no code to show, and passersby very welcome to jump on it and do it, of course.

Thanks for your halp.

mcollina commented 1 year ago

By looking at how ssrf-req-filter is implemented, we need to add an option to Client so that we can install the same lookup event handler inside our connect function.

andsens commented 1 year ago

@ronjouch

I actually like this thing, it's straightforward and explicit, more than the plugins.

However, I dislike that it forces an extra dns lookup, whereas a "plugin" agent solution integrated to fetch (similar to what request & axios do) does the lookup, and feeds it the agent/interceptor the IP, eliminating the extra dns resolution.

Any opinion/comment about it? Or should I just not care because the dns lookup fast/cached?

This is actually insecure because of a TOCTOU (time of check, time of use) vulnerability (owasp link). You can create e.g. a round robin A record where only one of the addresses is for an internal IP while the others are fine. So when you check you get one IP and when you run it you get another.

nunofgs commented 1 year ago

FWIW I was able to pass a custom lookup function to get this functionality:

import dns from 'node:dns';

const lookup = function (hostname, options, callback) {
  dns.lookup(hostname, options, (err, address, family) => {
    if (err) {
      return callback(err, address, family);
    }

    if (address === '127.0.0.1') {
      return callback(new Error('not allowed'), address, family);
    }

    return callback(err, address, family);
  })
}

const agent = new Agent({
  connect: {
    lookup
  }
});

setGlobalDispatcher(agent);

matthieusieben commented 7 months ago

@nunofgs

FWIW I was able to pass a custom lookup function to get this functionality:

Using a global dispatcher might be problematic as, in some cases, you do might want to be able to contact other micro services. Also, using only the lookup function as protection would allow an attacker to circumvent the protection by using an IP in the URI (e.g. http://[::1]/foo).

matthieusieben commented 7 months ago

I personally use a wrapper around fetch that can be used as follows:

const safeFetch = ssrfFetchWrap({ fetch: globalThis.fetch })
safeFetch(new Request('http://[::1]/foo')) // Will throw

fetch() can still be used for trusted hosts (e.g. micro services) and safeFetch() for user provided endpoints.

Here is what the implementation looks like:

```ts import dns, { promises as dnsPromises } from 'node:dns' import ipaddr from 'ipaddr.js' const { IPv4, IPv6 } = ipaddr type Fetch = ( this: void | null | typeof globalThis, input: Request, ) => Promise class FetchError extends Error { constructor( public readonly status: number, message: string, public readonly context: Record, ) { super(message) } } export type SsrfSafeFetchWrapOptions = NonNullable< Parameters[0] > /** * @see {@link https://owasp.org/Top10/A10_2021-Server-Side_Request_Forgery_%28SSRF%29/} */ export const ssrfFetchWrap = ({ fetch = globalThis.fetch as Fetch, } = {}): Fetch => { const ssrfSafeFetch: Fetch = async (request) => { const { protocol, hostname } = new URL(request.url) if (protocol === 'http:' || protocol === 'https:') { if (request.redirect === 'follow') { // Owasp recommends to "Disable HTTP redirections" throw new FetchError( 500, 'Request redirect must be "error" or "manual" when SSRF is enabled', { request }, ) } // Make sure the hostname is a unicast IP address await verifyHostnameIps(hostname) } else if (protocol === 'data:') { // No SSRF issue } else { // blob: about: file: all should be rejected throw new FetchError(400, `Forbidden protocol ${protocol}`, { request }) } return fetch(request) } return ssrfSafeFetch } async function verifyHostnameIps(hostname: string) { for await (const ip of hostnameLookup(hostname)) { if (ip.range() !== 'unicast') { throw new FetchError(400, `Invalid hostname IP address ${ip}`, {}) } } } async function* hostnameLookup( hostname: string, ): AsyncGenerator { if (IPv4.isIPv4(hostname)) { return yield IPv4.parse(hostname) } if (hostname.startsWith('[') && hostname.endsWith(']')) { return yield IPv6.parse(hostname.slice(1, -1)) } return yield* domainLookup(hostname) } async function* domainLookup( domain: string, ): AsyncGenerator { const results = await dnsPromises .lookup(domain, { hints: dns.ADDRCONFIG | dns.V4MAPPED, all: true, }) .catch((cause) => { throw cause?.code === 'ENOTFOUND' ? new FetchError(400, `Invalid hostname ${domain}`, { cause, }) : new FetchError(500, `Unable resolve DNS for ${domain}`, { cause, }) }) for (const addr of results) { const ip = addr.family === 4 ? IPv4.parse(addr.address) : IPv6.parse(addr.address) if (ip instanceof IPv6 && ip.isIPv4MappedAddress()) { yield ip.toIPv4Address() } else { yield ip } } } ```

andsens commented 7 months ago

@matthieusieben see my previous comment https://github.com/nodejs/undici/issues/2019#issuecomment-1706572083 as to why this is unsafe.

nodejs / undici