From 31239f368737d695fc8fa7f4125cf62e48a198e0 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 20 Sep 2026 13:54:31 +0200 Subject: [PATCH] fix(ssrf): connect over every address a name resolves to, IPv4 first The guard resolved a name to one address and pinned the socket to it. On a dual-stack name whose AAAA came first that was an IPv6 many hosts cannot reach on 443, and the request sat until the caller's timeout while the A record would have answered at once. OIDC discovery, Dawarich, AirTrail, the photo providers, a self-hosted router or Nominatim and the model endpoints all ride this path. The name is now resolved to every address it has, each one is checked, and one blocked address blocks the name. The whole checked list goes to the socket, IPv4 ahead of IPv6, where Node walks it itself and moves on after its attempt timeout. Redirect hops do the same per hop. A caller that still names one address gets exactly the old behaviour. --- server/src/utils/ssrfGuard.ts | 94 +++++++++++---- .../tests/unit/utils/ssrf-dual-stack.test.ts | 99 ++++++++++++++++ server/tests/unit/utils/ssrfGuard.test.ts | 109 ++++++++++++++++++ 3 files changed, 280 insertions(+), 22 deletions(-) create mode 100644 server/tests/unit/utils/ssrf-dual-stack.test.ts diff --git a/server/src/utils/ssrfGuard.ts b/server/src/utils/ssrfGuard.ts index eb6ea717e..2e11bd06c 100644 --- a/server/src/utils/ssrfGuard.ts +++ b/server/src/utils/ssrfGuard.ts @@ -12,11 +12,42 @@ const ALLOWED_LINK_LOCAL = new Set(readEnv().net.allowLinkLocalIps); export interface SsrfResult { allowed: boolean; + /** The first of `resolvedIps`, kept for the callers that name one address. */ resolvedIp?: string; + /** Every address the name resolves to, each one checked, IPv4 first. */ + resolvedIps?: string[]; isPrivate: boolean; error?: string; } +/** + * Every address a name resolves to, IPv4 ahead of IPv6. + * + * One address was never enough. `dns.lookup(name)` hands back whichever record + * the resolver lists first, on Node 22 in the resolver's own order, and a + * dual-stack name whose AAAA comes first then pinned the connection to an IPv6 + * that many hosts cannot reach on port 443 while the A record would have + * answered at once. The whole list goes to the socket, where Node tries the + * addresses in turn (autoSelectFamily, the default since Node 20), so an + * unreachable family costs a quarter of a second instead of the caller's whole + * timeout. IPv4 leads because that is the family that is reachable from the + * containers and LXCs TREK usually runs in. + */ +async function resolveAll(hostname: string): Promise<{ address: string; family: number }[]> { + const result = await dns.lookup(hostname, { all: true }); + // The promise API answers an array for `all: true`; a resolver stub in a test + // may still answer one record, which is the same thing said shorter. + const list = (Array.isArray(result) ? result : [result]).filter((entry) => typeof entry?.address === 'string'); + if (list.length === 0) { + const error = new Error(`getaddrinfo ENOTFOUND ${hostname}`) as NodeJS.ErrnoException; + error.code = 'ENOTFOUND'; + throw error; + } + const family = (entry: { address: string; family: number }) => entry.family || (entry.address.includes(':') ? 6 : 4); + return [...list.filter((entry) => family(entry) === 4), ...list.filter((entry) => family(entry) !== 4)] + .map((entry) => ({ address: entry.address, family: family(entry) })); +} + /** * The IPv4 an IPv4-mapped address (`::ffff:a.b.c.d`) stands for, or null. * @@ -112,39 +143,46 @@ export async function checkSsrf(rawUrl: string, bypassInternalIpAllowed: boolean const hostname = url.hostname.toLowerCase(); - // Resolve hostname to IP - let resolvedIp: string; + // Resolve the name to every address it has. Each one is judged on its own, + // and a single blocked address blocks the name: the socket may end up on any + // of them, so a list that mixes a public and a private address is a private + // target with a public alibi. + let resolvedIps: string[]; try { - const result = await dns.lookup(hostname); - resolvedIp = result.address; + resolvedIps = (await resolveAll(hostname)).map((entry) => entry.address); } catch (error_) { const code = error_ instanceof Error && 'code' in error_ ? String(error_.code) : 'unknown'; return { allowed: false, isPrivate: false, error: `Could not resolve hostname (${code})` }; } + const resolvedIp = resolvedIps[0]; - if (isAlwaysBlocked(resolvedIp)) { + const blocked = resolvedIps.find((ip) => isAlwaysBlocked(ip)); + if (blocked) { return { allowed: false, isPrivate: true, - resolvedIp, + resolvedIp: blocked, + resolvedIps, error: 'Requests to loopback and link-local addresses are not allowed', }; } - if (isPrivateNetwork(resolvedIp) || isInternalHostname(hostname)) { + const privateIp = resolvedIps.find((ip) => isPrivateNetwork(ip)); + if (privateIp || isInternalHostname(hostname)) { if (!ALLOW_INTERNAL_NETWORK || bypassInternalIpAllowed) { return { allowed: false, isPrivate: true, - resolvedIp, + resolvedIp: privateIp ?? resolvedIp, + resolvedIps, error: 'Requests to private/internal network addresses are not allowed. Set ALLOW_INTERNAL_NETWORK=true to permit this for self-hosted setups.', }; } - return { allowed: true, isPrivate: true, resolvedIp }; + return { allowed: true, isPrivate: true, resolvedIp: privateIp ?? resolvedIp, resolvedIps }; } - return { allowed: true, isPrivate: false, resolvedIp }; + return { allowed: true, isPrivate: false, resolvedIp, resolvedIps }; } /** Link-local / cloud-metadata addresses — never a legitimate model host. */ @@ -212,18 +250,18 @@ export async function safeFetchAdminConfigured( if (!['http:', 'https:'].includes(parsed.protocol)) { throw new SsrfBlockedError('Only HTTP and HTTPS URLs are allowed'); } - let resolvedIp: string; + let resolvedIps: string[]; try { - resolvedIp = (await dns.lookup(parsed.hostname)).address; + resolvedIps = (await resolveAll(parsed.hostname)).map((entry) => entry.address); } catch (error_) { const code = error_ instanceof Error && 'code' in error_ ? String(error_.code) : 'unknown'; throw new SsrfBlockedError(`Could not resolve hostname (${code})`); } - if (isLinkLocal(resolvedIp)) { + if (resolvedIps.some((ip) => isLinkLocal(ip))) { throw new SsrfBlockedError('Requests to link-local / cloud-metadata addresses are not allowed'); } - const dispatcher = createPinnedDispatcher(resolvedIp, true, responseTimeoutMs); + const dispatcher = createPinnedDispatcher(resolvedIps, true, responseTimeoutMs); const response = await fetch(currentUrl, { ...hopInit, redirect: 'manual', dispatcher } as any); // Only a 3xx WITH a Location header is a redirect we follow; anything else @@ -417,7 +455,7 @@ export async function safeFetchFollow( throw new SsrfBlockedError(ssrf.error ?? 'Request blocked by SSRF guard'); } - const dispatcher = createPinnedDispatcher(ssrf.resolvedIp!, rejectUnauthorized); + const dispatcher = createPinnedDispatcher(ssrf.resolvedIps ?? [ssrf.resolvedIp!], rejectUnauthorized); const response = await fetch(currentUrl, { ...hopInit, redirect: 'manual', @@ -454,10 +492,23 @@ export async function safeFetchFollow( /** * Returns an undici Agent whose connect.lookup is pinned to the already-validated - * IP. This prevents DNS rebinding (TOCTOU) by ensuring the outbound connection - * goes to the IP we checked, not a re-resolved one. + * addresses. This prevents DNS rebinding (TOCTOU) by ensuring the outbound + * connection goes to an address we checked, not a re-resolved one. + * + * Given the whole checked list, in the order `resolveAll` put it, the socket + * gets every address at once: Node asks for `all: true` and walks the list + * itself, moving on after its attempt timeout when one family does not answer. + * A caller that still names one address gets exactly the old behaviour. */ -export function createPinnedDispatcher(resolvedIp: string, rejectUnauthorized = true, responseTimeoutMs?: number): Agent { +export function createPinnedDispatcher( + resolved: string | readonly string[], + rejectUnauthorized = true, + responseTimeoutMs?: number, +): Agent { + const addresses = (typeof resolved === 'string' ? [resolved] : [...resolved]).map((address) => ({ + address, + family: address.includes(':') ? 6 : 4, + })); return new Agent({ // undici caps the wait for response headers at 5 minutes by default, and // that cap is invisible from the call site: an AbortController set to @@ -468,12 +519,11 @@ export function createPinnedDispatcher(resolvedIp: string, rejectUnauthorized = connect: { rejectUnauthorized, lookup: (_hostname: string, opts: Record, callback: Function) => { - const family = resolvedIp.includes(':') ? 6 : 4; - // Node.js 18+ may call lookup with `all: true`, expecting an array of address objects + // Node asks with `all: true` when it may choose between families itself. if (opts?.all) { - callback(null, [{ address: resolvedIp, family }]); + callback(null, addresses); } else { - callback(null, resolvedIp, family); + callback(null, addresses[0].address, addresses[0].family); } }, }, diff --git a/server/tests/unit/utils/ssrf-dual-stack.test.ts b/server/tests/unit/utils/ssrf-dual-stack.test.ts new file mode 100644 index 000000000..36122c1b2 --- /dev/null +++ b/server/tests/unit/utils/ssrf-dual-stack.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, vi, afterEach, beforeAll, afterAll } from 'vitest'; +import http from 'node:http'; +import net from 'node:net'; +import type { AddressInfo } from 'node:net'; + +// Only the resolver is stubbed. undici and net are the real thing, because the +// point of these cases is what the socket does with the list it is handed. +vi.mock('dns/promises', () => ({ + default: { lookup: vi.fn() }, + lookup: vi.fn(), +})); + +import dns from 'dns/promises'; +import { safeFetchAdminConfigured } from '../../../src/utils/ssrfGuard'; + +const mockLookup = vi.mocked(dns.lookup); + +// RFC 6666 discard prefix and TEST-NET-1: routed nowhere on any sane machine, +// so a connect attempt either fails at once or sits until the family timeout. +const DEAD_V6 = '100::1'; +const DEAD_V4 = '192.0.2.1'; + +let v4: http.Server; +let v4Port: number; +let v6: http.Server | null = null; +let v6Port = 0; + +const listen = (server: http.Server, host: string) => + new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, host, () => resolve((server.address() as AddressInfo).port)); + }); + +beforeAll(async () => { + v4 = http.createServer((_req, res) => res.end('v4')); + v4Port = await listen(v4, '127.0.0.1'); + const six = http.createServer((_req, res) => res.end('v6')); + try { + v6Port = await listen(six, '::1'); + v6 = six; + } catch { + six.close(); + } +}); + +afterAll(async () => { + await new Promise((r) => v4.close(r)); + if (v6) await new Promise((r) => v6!.close(r)); +}); + +afterEach(() => { + mockLookup.mockReset(); +}); + +describe('a dual-stack name where one family does not answer', () => { + it('SEC-DUAL-101: the AAAA first in the answer no longer pins the connection to an unreachable IPv6', async () => { + mockLookup.mockResolvedValue([ + { address: DEAD_V6, family: 6 }, + { address: '127.0.0.1', family: 4 }, + ] as never); + + const started = Date.now(); + const res = await safeFetchAdminConfigured(`http://idp.example:${v4Port}/`, { signal: AbortSignal.timeout(8000) }); + + expect(res.status).toBe(200); + expect(await res.text()).toBe('v4'); + // Well inside what a single dead attempt would have cost. + expect(Date.now() - started).toBeLessThan(3000); + }); + + it('SEC-DUAL-102: a dead IPv4 ahead of a live IPv6 still connects, the socket moves on by itself', async (ctx) => { + if (!v6) return ctx.skip(); + mockLookup.mockResolvedValue([ + { address: DEAD_V4, family: 4 }, + { address: '::1', family: 6 }, + ] as never); + + const started = Date.now(); + const res = await safeFetchAdminConfigured(`http://idp.example:${v6Port}/`, { signal: AbortSignal.timeout(8000) }); + + expect(res.status).toBe(200); + expect(await res.text()).toBe('v6'); + // One attempt timeout (250 ms by default) plus the real connect. + expect(Date.now() - started).toBeLessThan(net.getDefaultAutoSelectFamilyAttemptTimeout() * 8 + 1000); + }); + + it('SEC-DUAL-103: the socket never gets an address the guard did not check', async () => { + // The resolver answers the checked list once; if the socket re-resolved the + // name it would be handed this second, unchecked answer and connect there. + mockLookup + .mockResolvedValueOnce([{ address: '127.0.0.1', family: 4 }] as never) + .mockResolvedValue([{ address: DEAD_V4, family: 4 }] as never); + + const res = await safeFetchAdminConfigured(`http://idp.example:${v4Port}/`, { signal: AbortSignal.timeout(8000) }); + + expect(res.status).toBe(200); + expect(mockLookup).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/tests/unit/utils/ssrfGuard.test.ts b/server/tests/unit/utils/ssrfGuard.test.ts index e2bb283aa..95a1ed15b 100644 --- a/server/tests/unit/utils/ssrfGuard.test.ts +++ b/server/tests/unit/utils/ssrfGuard.test.ts @@ -799,3 +799,112 @@ describe('ALLOW_LINK_LOCAL_IPS (#2400)', () => { expect(await open.guard.checkSsrf('https://immich.example/')).toMatchObject({ allowed: true, isPrivate: true }); }); }); + +describe('dual-stack names: every address is checked and every address is pinned', () => { + afterEach(() => { + vi.unstubAllEnvs(); + vi.unstubAllGlobals(); + mockLookup.mockReset(); + }); + + const resolveAll = (entries: { address: string; family: number }[]) => + mockLookup.mockResolvedValue(entries as never); + + it('SEC-DUAL-001: asks the resolver for all addresses and lists IPv4 ahead of IPv6', async () => { + resolveAll([{ address: '2001:db8::10', family: 6 }, { address: '203.0.113.10', family: 4 }]); + + const result = await checkSsrf('https://idp.example'); + + expect(mockLookup).toHaveBeenCalledWith('idp.example', { all: true }); + expect(result.allowed).toBe(true); + expect(result.resolvedIps).toEqual(['203.0.113.10', '2001:db8::10']); + expect(result.resolvedIp).toBe('203.0.113.10'); + }); + + it('SEC-DUAL-002: a name with one private address among public ones stays blocked', async () => { + resolveAll([{ address: '203.0.113.10', family: 4 }, { address: '10.0.0.5', family: 4 }]); + + const result = await checkSsrf('https://split.example'); + + expect(result.allowed).toBe(false); + expect(result.isPrivate).toBe(true); + expect(result.resolvedIp).toBe('10.0.0.5'); + }); + + it('SEC-DUAL-003: a loopback address hidden behind a public one is still loopback', async () => { + resolveAll([{ address: '203.0.113.10', family: 4 }, { address: '::1', family: 6 }]); + + const result = await checkSsrf('https://rebind.example'); + + expect(result.allowed).toBe(false); + expect(result.error).toContain('loopback'); + }); + + it('SEC-DUAL-004: the pinned dispatcher hands the socket the whole checked list, IPv4 first', async () => { + resolveAll([{ address: '2001:db8::10', family: 6 }, { address: '203.0.113.10', family: 4 }]); + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ status: 200, ok: true, headers: { get: () => null } })); + + await safeFetchFollow('https://idp.example/'); + + const lookup = agentCapture.options.connect.lookup as (h: string, o: object, cb: (...a: unknown[]) => void) => void; + const seen: unknown[] = []; + lookup('somebody-else.example', { all: true }, (...args: unknown[]) => seen.push(...args)); + expect(seen).toEqual([null, [ + { address: '203.0.113.10', family: 4 }, + { address: '2001:db8::10', family: 6 }, + ]]); + // Asked for one address, the socket gets the first of the same list; the + // name it asks for never matters, that is the whole point of the pin. + const single: unknown[] = []; + lookup('somebody-else.example', {}, (...args: unknown[]) => single.push(...args)); + expect(single).toEqual([null, '203.0.113.10', 4]); + }); + + it('SEC-DUAL-005: the admin lane blocks a name with a metadata address anywhere in its answer', async () => { + resolveAll([{ address: '203.0.113.10', family: 4 }, { address: '169.254.169.254', family: 4 }]); + const fetchSpy = vi.fn(); + vi.stubGlobal('fetch', fetchSpy); + + await expect(safeFetchLlm('https://models.example/v1')).rejects.toThrow(SsrfBlockedError); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('SEC-DUAL-006: a redirect hop resolves and pins its own full list again', async () => { + mockLookup + .mockResolvedValueOnce([{ address: '2001:db8::1', family: 6 }, { address: '203.0.113.1', family: 4 }] as never) + .mockResolvedValueOnce([{ address: '2001:db8::2', family: 6 }, { address: '203.0.113.2', family: 4 }] as never); + const pins: unknown[][] = []; + vi.stubGlobal('fetch', vi.fn().mockImplementation(async () => { + const lookup = agentCapture.options.connect.lookup as (h: string, o: object, cb: (...a: unknown[]) => void) => void; + const seen: unknown[] = []; + lookup('x', { all: true }, (...args: unknown[]) => seen.push(...args)); + pins.push(seen); + return pins.length === 1 + ? { status: 302, ok: false, headers: { get: (h: string) => (h.toLowerCase() === 'location' ? 'https://second.example/' : null) } } + : { status: 200, ok: true, headers: { get: () => null } }; + })); + + const response = await safeFetchLlm('https://first.example/'); + + expect(response.status).toBe(200); + expect(pins[0][1]).toEqual([{ address: '203.0.113.1', family: 4 }, { address: '2001:db8::1', family: 6 }]); + expect(pins[1][1]).toEqual([{ address: '203.0.113.2', family: 4 }, { address: '2001:db8::2', family: 6 }]); + }); + + it('SEC-DUAL-007: a single record answered the old way is still one pinned address', async () => { + mockLookup.mockResolvedValue({ address: '203.0.113.10', family: 4 }); + + const result = await checkSsrf('https://one.example'); + + expect(result).toMatchObject({ allowed: true, resolvedIp: '203.0.113.10', resolvedIps: ['203.0.113.10'] }); + }); + + it('SEC-DUAL-008: an empty answer is a name that does not resolve', async () => { + mockLookup.mockResolvedValue([] as never); + + const result = await checkSsrf('https://empty.example'); + + expect(result.allowed).toBe(false); + expect(result.error).toBe('Could not resolve hostname (ENOTFOUND)'); + }); +});