Skip to content

SSRF fix in fetch.ts is bypassable via DNS rebinding — validated IP is never pinned to the actual connection #116

Description

@dmchaledev

Problem

#91 added private/internal-IP protection to fetchHeadersWithMeta (src/fetch.ts), but the fix has a time-of-check-to-time-of-use (TOCTOU) gap: it validates the hostname's DNS resolution, then makes an entirely separate DNS resolution for the actual request — so the validated IP is never the IP that's actually connected to.

// src/fetch.ts:88-96
for (let hop = 0; ; hop++) {
  await assertPublicUrl(current, options?.allowPrivateNetworks); // resolves via dns.lookup(), checks the result
  ...
  const res = await fetch(current, { method: 'GET', redirect: 'manual', signal: controller.signal });
  // ^ passes the hostname (not the validated IP) — Node's fetch (undici) re-resolves
  //   DNS itself here, independently of the lookup() call above.

assertPublicUrl calls lookup(hostname, { all: true }) and rejects the request if that resolution returns a private/loopback/link-local address. But the subsequent fetch(current, ...) call is given the hostname again, not the resolved IP — so it triggers its own, independent DNS resolution inside undici. Nothing pins the connection to the address that was actually checked.

This is the classic DNS rebinding bypass for exactly this style of SSRF guard:

  1. Attacker controls DNS for evil.example.com, TTL=0.
  2. First query (assertPublicUrl's lookup()) returns a public IP → passes validation.
  3. Second query (undici's internal resolution inside fetch()) returns 127.0.0.1 / 169.254.169.254 / 10.x.x.x → the actual HTTP request goes to the internal target.

Since the two resolutions happen milliseconds apart against an attacker-controlled authoritative server, this is practical, not theoretical — it's a well-documented bypass class for "resolve-then-fetch-by-hostname" SSRF guards (the reason tools like ssrf-req-filter/ssrfvpn-style guards pin the resolved IP into the actual connection instead of re-resolving).

Confirming this isn't just theoretical: every existing test in test/fetch.test.ts mocks the global fetch directly, so none of them exercise real DNS resolution at the connection layer — there's no test proving the IP that was validated is the IP that's actually connected to, because today it isn't.

Why this matters here specifically

This is the exact vulnerability class #91 was opened to close, in a project whose whole value proposition is header/security hardening, and whose README explicitly advertises server-side scanning of arbitrary/customer-supplied targets (the ASM integration use case). A partial SSRF fix that's bypassable by anyone who controls a domain and sets TTL=0 is worse for credibility than having no fix and saying so, if it's ever pointed out publicly.

Proposed fix

Pin the validated IP address to the actual connection instead of re-resolving:

  1. Use fetch's dispatcher option with an undici Agent configured with a custom connect that dials the already-validated IP directly, while still sending the original Host header and TLS SNI (servername) for the hostname — so cert validation and virtual-hosting still work correctly. Node bundles undici; Agent/Client's connect option accepts { hostname, port } overrides per-request via a connect function, e.g.:
    import { Agent } from 'undici'; // or Node's internal undici export, depending on the Node version floor
    const dispatcher = new Agent({
      connect: (opts, cb) => {
        // opts.hostname is the original hostname; substitute the validated IP here
        // while keeping servername: opts.hostname for TLS SNI/cert checks.
      },
    });
    await fetch(current, { dispatcher, ... });
  2. Re-run assertPublicUrl against the same resolved address that's about to be pinned (don't resolve twice), so there's exactly one DNS query per hop, and it's the query whose result is actually connected to.
  3. Add a regression test in test/fetch.test.ts that mocks lookup() to return a public IP on the first call and would return a private IP on any second call — proving the implementation issues only one resolution per hop and pins it, rather than trusting that the mock happens to be consistent.

This is a real fix, not a small tweak — it touches how the request is dispatched — so I'm filing it as an issue for design review rather than pushing a PR straight to main, given fetch.ts handles security-sensitive request routing and deserves scrutiny on the pinning approach (especially TLS SNI/hostname-verification correctness) before landing.

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions