diff --git a/package.json b/package.json index a80f82f..25b187a 100644 --- a/package.json +++ b/package.json @@ -31,6 +31,7 @@ }, "dependencies": { "sharp": "^0.35.3", + "undici": "^7.0.0", "srvx": "^0.12.4" }, "devDependencies": { @@ -63,7 +64,7 @@ } }, "engines": { - "node": "^20.16.0 || >=22.3.0" + "node": "^20.18.1 || >=22.3.0" }, "packageManager": "pnpm@11.17.0" } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a71a541..3e93f6d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -14,6 +14,9 @@ importers: srvx: specifier: ^0.12.4 version: 0.12.4 + undici: + specifier: ^7.0.0 + version: 7.29.1 devDependencies: '@fastify/accept-negotiator': specifier: ^2.0.1 @@ -2390,6 +2393,10 @@ packages: undici-types@8.3.0: resolution: {integrity: sha512-j375ScV60dom+YkPFIfTLcOiPxkN/buHz5GobjLhixFuANaNs3C9l4GmrWqejgXWJ7BbJcFYpTEUkS1Ge8bpZQ==} + undici@7.29.1: + resolution: {integrity: sha512-RYONW2MeafgYlkVOKYKkA/Ag7BmXqgIWCa8t1m0JcxrQg9pI9lEqRhAOruOBCbAohOa/gkCF+iPi9hrgvTzu6Q==} + engines: {node: '>=20.18.1'} + unist-util-is@6.0.1: resolution: {integrity: sha512-LsiILbtBETkDz8I9p1dQ0uyRUWuaQzd/cuEeS1hoRSyW5E5XGmTzlwY1OrNzzakGowI9Dr/I8HVaw4hTtnxy8g==} @@ -4932,6 +4939,8 @@ snapshots: undici-types@8.3.0: {} + undici@7.29.1: {} + unist-util-is@6.0.1: dependencies: '@types/unist': 3.0.3 diff --git a/src/storage/http.ts b/src/storage/http.ts index 8be0f65..fcc13dc 100644 --- a/src/storage/http.ts +++ b/src/storage/http.ts @@ -1,5 +1,5 @@ import { HTTPError } from "h3"; -import { getBuiltinModule, getEnv } from "../utils.ts"; +import { getBuiltinModule, getEnv, requireModule } from "../utils.ts"; import type { IPXStorage } from "../types.ts"; export type HTTPStorageOptions = { @@ -340,11 +340,12 @@ export function ipxHttpStorage(_options: HTTPStorageOptions = {}): IPXStorage { * DNS record pointing at `127.0.0.1` or `169.254.169.254`. All resolved addresses have * to be public, since which one the socket ends up using is not ours to decide. * - * Limitation: this cannot close the TOCTOU / DNS-rebinding window. The name is resolved - * again when `fetch` opens the socket, so a record with a short TTL can answer with a - * public address here and a private one there. Closing that gap needs connection-level - * pinning (a custom agent/dispatcher that validates the peer address of the socket it - * just connected), which is out of reach of the plain `fetch` used here. + * This check alone cannot close the TOCTOU / DNS-rebinding window: the name is + * resolved again when `fetch` opens the socket, so a record with a short TTL could + * in principle answer with a public address here and a private one there. That gap + * is closed separately, at connect time, by {@link getPinnedFetch}, which every + * caller of this module already goes through when `blockPrivateIPs` is enabled -- + * see its own doc comment for how. */ async function validatePublicIP(url: URL, id: string) { // Strip the brackets of an IPv6 literal (`[::1]`) @@ -408,6 +409,98 @@ export function ipxHttpStorage(_options: HTTPStorageOptions = {}): IPXStorage { } } + let pinnedFetchPromise: Promise | undefined; + + /** + * Returns a fetch function whose underlying socket connection is pinned to + * addresses validated by {@link isPublicIP}, closing the TOCTOU window that + * {@link validatePublicIP} alone cannot: resolution and validation happen + * inside the connector's own lookup hook, so the same addresses that were + * checked are the only ones the socket can ever connect to. A record with a + * short TTL cannot answer differently between the check and the connect. + * + * Throws `IPX_IP_CHECK_UNAVAILABLE` if `node:dns` or `undici`'s `Agent` + * cannot be loaded (e.g. a non-Node runtime, or `undici` not installed). + * The per-call `validatePublicIP` check alone cannot close the TOCTOU + * window, so when pinning is unavailable the request fails closed instead + * of silently serving with a weaker guarantee. + */ + function getPinnedFetch(): Promise { + return (pinnedFetchPromise ??= (async () => { + const dns = getBuiltinModule("node:dns"); + if (!dns?.lookup) { + if (blockPrivateIPs) { + throw new HTTPError({ + statusCode: 500, + statusText: `IPX_IP_CHECK_UNAVAILABLE`, + message: `Cannot pin the connection to a validated address: \`blockPrivateIPs\` requires \`node:dns\`.`, + }); + } + return fetch; + } + + let Agent: any; + try { + ({ Agent } = requireModule<{ Agent: any }>("undici")); + } catch { + if (blockPrivateIPs) { + throw new HTTPError({ + statusCode: 500, + statusText: `IPX_IP_CHECK_UNAVAILABLE`, + message: `Cannot pin the connection to a validated address: \`blockPrivateIPs\` requires \`undici\`.`, + }); + } + return fetch; + } + + // A dns.lookup-compatible function: undici's Agent connector calls this + // instead of resolving the hostname itself, so whatever address is + // handed back here is the only one the connector ever sees. + + const pinnedLookup = ( + hostname: string, + options: any, + callback: any, + ): void => { + dns.lookup(hostname, { ...options, all: true }, (error, addresses) => { + if (error) { + callback(error); + return; + } + const list = addresses as unknown as { + address: string; + family: number; + }[]; + for (const { address, family } of list) { + if (!isPublicIP(address, family)) { + callback( + new Error( + `Hostname ${hostname} resolved to a disallowed address: ${address}`, + ), + ); + return; + } + } + if (options?.all) { + callback(null, list); + } else { + const [first] = list; + callback(null, first!.address, first!.family); + } + }); + }; + + const dispatcher = new Agent({ connect: { lookup: pinnedLookup } }); + + return ((input: RequestInfo | URL, init?: RequestInit) => + fetch(input, { + ...init, + // @ts-expect-error -- `dispatcher` is a Node-specific fetch() extension, not in lib.dom.d.ts + dispatcher, + })) as typeof fetch; + })()); + } + /** * Fetches `url`, following redirects manually so that every hop is re-validated * against the allowlist and, when enabled, the private IP check (see {@link validateURL}). @@ -420,14 +513,16 @@ export function ipxHttpStorage(_options: HTTPStorageOptions = {}): IPXStorage { const _init: RequestInit = { ...fetchOptions, ...init }; if ((allowAllDomains && !blockPrivateIPs) || _init.redirect) { - return fetch(url, _init); + const fetchFn = blockPrivateIPs ? await getPinnedFetch() : fetch; + return fetchFn(url, _init); } let currentURL = url; let method = _init.method || "GET"; + const fetchFn = blockPrivateIPs ? await getPinnedFetch() : fetch; for (let i = 0; i <= MAX_REDIRECTS; i++) { - const response = await fetch(currentURL, { + const response = await fetchFn(currentURL, { ..._init, method, redirect: "manual", diff --git a/test/storage/http.test.ts b/test/storage/http.test.ts index 305342d..1cc58c7 100644 --- a/test/storage/http.test.ts +++ b/test/storage/http.test.ts @@ -351,7 +351,9 @@ describe("http", () => { ).rejects.toMatchObject({ statusCode: 403, statusText: "IPX_FORBIDDEN_IP", - message: expect.stringContaining("127.0.0.1"), + cause: expect.objectContaining({ + message: expect.stringContaining("127.0.0.1"), + }), }); expect(fetch).not.toHaveBeenCalled(); }); @@ -412,6 +414,33 @@ describe("http", () => { expect(fetch).not.toHaveBeenCalled(); }); + it("closes the TOCTOU/DNS-rebinding gap: the connection is pinned to the validated address, not re-resolved", async () => { + // The pre-check (dns.promises.lookup) sees a PUBLIC address and passes. + stubLookup({ address: "93.184.216.34", family: 4 }); + + // The raw dns.lookup -- used only by the pinned connector's own lookup + // hook, not by the pre-check -- answers with a PRIVATE address instead, + // simulating a DNS record that changed between the check and the + // connection (or a resolver that genuinely answers differently to two + // separate queries for the same name, which is exactly what DNS + // rebinding relies on). + vi.spyOn(dns, "lookup").mockImplementation((( + _hostname: string, + _options: unknown, + callback: (error: null, addresses: unknown) => void, + ) => { + callback(null, [{ address: "127.0.0.1", family: 4 }]); + }) as unknown as typeof dns.lookup); + + await expect( + storage.getData("https://example.com/image.png"), + ).rejects.toMatchObject({ + cause: expect.objectContaining({ + message: expect.stringContaining("127.0.0.1"), + }), + }); + }); + it("blocks a redirect hop to a private address", async () => { const fetch = vi .fn()