From 76744a476b2d1d81e62e6ef26dde93eb279eba19 Mon Sep 17 00:00:00 2001 From: Matthew-Selvam Date: Sat, 18 Jul 2026 10:30:35 +0530 Subject: [PATCH] fix: resolve DNS before SSRF checks to close rebinding bypass (CWE-918) isInternalNetwork() only checked the literal hostname string, not the resolved IP, so an attacker-controlled domain resolving to a private address bypassed the check entirely. isAllowedUri() is now async and resolves the hostname via dns.lookup(), checking every returned address against the private-range blocklist before allowing a fetch. Fails closed if resolution errors. Also unwraps IPv6-mapped IPv4 addresses (::ffff:127.0.0.1 and the compressed hex form) before the range check, since neither was previously matched by the blocklist regexes. Updates the one caller (fetchAgentCard) and the isAllowedUri call sites in tests to await the now-async function, with DNS mocked for determinism, plus new regression tests for the rebinding bypass, multi-A-record responses, resolution failures (fail-closed), and the IPv6-mapped-IPv4 forms. Fixes #183 --- src/__tests__/data-layer.test.ts | 94 +++++++++++++++++++----- src/__tests__/discovery-data-uri.test.ts | 10 ++- src/__tests__/social.test.ts | 12 ++- src/registry/discovery.ts | 62 ++++++++++++++-- 4 files changed, 146 insertions(+), 32 deletions(-) diff --git a/src/__tests__/data-layer.test.ts b/src/__tests__/data-layer.test.ts index d69de04b..53f760f0 100644 --- a/src/__tests__/data-layer.test.ts +++ b/src/__tests__/data-layer.test.ts @@ -27,8 +27,23 @@ vi.mock("../agent/injection-defense.js", () => ({ sanitizeInput: vi.fn((s: string) => ({ content: s, blocked: false })), })); +// Mock DNS resolution so isAllowedUri's rebinding check is deterministic +// and doesn't require network access in tests. Hostnames not listed here +// resolve to a public IP by default. +const dnsMockAddresses: Record = { + "example.com": ["93.184.216.34"], + "api.conway.tech": ["203.0.113.10"], +}; +vi.mock("node:dns/promises", () => ({ + lookup: vi.fn(async (hostname: string, _opts?: unknown) => { + const addresses = dnsMockAddresses[hostname] ?? ["203.0.113.99"]; + return addresses.map((address) => ({ address, family: 4 })); + }), +})); + // Import after mocks are set up const { isAllowedUri, isInternalNetwork, validateAgentCard } = await import("../registry/discovery.js"); +const dns = await import("node:dns/promises"); const { loadInstalledTools } = await import("../agent/tools.js"); function makeTmpDbPath(): string { @@ -109,40 +124,79 @@ describe("SSRF Protection", () => { }); describe("isAllowedUri", () => { - it("allows https URIs", () => { - expect(isAllowedUri("https://example.com/agent-card.json")).toBe(true); + it("allows https URIs", async () => { + expect(await isAllowedUri("https://example.com/agent-card.json")).toBe(true); + }); + + it("allows ipfs URIs", async () => { + expect(await isAllowedUri("ipfs://QmTest123")).toBe(true); + }); + + it("blocks http URIs", async () => { + expect(await isAllowedUri("http://example.com/agent-card.json")).toBe(false); + }); + + it("blocks file URIs", async () => { + expect(await isAllowedUri("file:///etc/passwd")).toBe(false); + }); + + it("blocks ftp URIs", async () => { + expect(await isAllowedUri("ftp://evil.com/data")).toBe(false); + }); + + it("blocks javascript URIs", async () => { + expect(await isAllowedUri("javascript:alert(1)")).toBe(false); }); - it("allows ipfs URIs", () => { - expect(isAllowedUri("ipfs://QmTest123")).toBe(true); + it("blocks https URIs to internal networks", async () => { + expect(await isAllowedUri("https://127.0.0.1/card.json")).toBe(false); + expect(await isAllowedUri("https://10.0.0.1/card.json")).toBe(false); + expect(await isAllowedUri("https://192.168.1.1/card.json")).toBe(false); + expect(await isAllowedUri("https://localhost/card.json")).toBe(false); }); - it("blocks http URIs", () => { - expect(isAllowedUri("http://example.com/agent-card.json")).toBe(false); + it("blocks invalid URIs", async () => { + expect(await isAllowedUri("not-a-url")).toBe(false); + expect(await isAllowedUri("")).toBe(false); }); - it("blocks file URIs", () => { - expect(isAllowedUri("file:///etc/passwd")).toBe(false); + it("blocks DNS-rebinding: a hostname that resolves to a private IP (CWE-918)", async () => { + const dnsModule = dns as unknown as { + lookup: (hostname: string, opts?: unknown) => Promise>; + }; + vi.mocked(dnsModule.lookup).mockImplementationOnce(async () => [ + { address: "127.0.0.1", family: 4 }, + ]); + expect(await isAllowedUri("https://evil.attacker.com/card.json")).toBe(false); }); - it("blocks ftp URIs", () => { - expect(isAllowedUri("ftp://evil.com/data")).toBe(false); + it("blocks DNS-rebinding: one of several resolved addresses is private", async () => { + const dnsModule = dns as unknown as { + lookup: (hostname: string, opts?: unknown) => Promise>; + }; + vi.mocked(dnsModule.lookup).mockImplementationOnce(async () => [ + { address: "203.0.113.5", family: 4 }, + { address: "10.0.0.1", family: 4 }, + ]); + expect(await isAllowedUri("https://multi-a-record.example/card.json")).toBe(false); }); - it("blocks javascript URIs", () => { - expect(isAllowedUri("javascript:alert(1)")).toBe(false); + it("fails closed when DNS resolution errors", async () => { + const dnsModule = dns as unknown as { + lookup: (hostname: string, opts?: unknown) => Promise>; + }; + vi.mocked(dnsModule.lookup).mockImplementationOnce(async () => { + throw new Error("ENOTFOUND"); + }); + expect(await isAllowedUri("https://does-not-resolve.example/card.json")).toBe(false); }); - it("blocks https URIs to internal networks", () => { - expect(isAllowedUri("https://127.0.0.1/card.json")).toBe(false); - expect(isAllowedUri("https://10.0.0.1/card.json")).toBe(false); - expect(isAllowedUri("https://192.168.1.1/card.json")).toBe(false); - expect(isAllowedUri("https://localhost/card.json")).toBe(false); + it("blocks IPv6-mapped IPv4 loopback (::ffff:127.0.0.1)", async () => { + expect(await isAllowedUri("https://[::ffff:127.0.0.1]/card.json")).toBe(false); }); - it("blocks invalid URIs", () => { - expect(isAllowedUri("not-a-url")).toBe(false); - expect(isAllowedUri("")).toBe(false); + it("blocks IPv6-mapped IPv4 loopback (compressed hex form)", async () => { + expect(await isAllowedUri("https://[::ffff:7f00:1]/card.json")).toBe(false); }); }); }); diff --git a/src/__tests__/discovery-data-uri.test.ts b/src/__tests__/discovery-data-uri.test.ts index 3eb5f896..d1edc2e7 100644 --- a/src/__tests__/discovery-data-uri.test.ts +++ b/src/__tests__/discovery-data-uri.test.ts @@ -23,6 +23,12 @@ vi.mock("../agent/injection-defense.js", () => ({ sanitizeInput: vi.fn((s: string) => ({ content: s, blocked: false })), })); +// Mock DNS resolution so isAllowedUri's rebinding check doesn't require +// network access in tests; example.com resolves to a public IP. +vi.mock("node:dns/promises", () => ({ + lookup: vi.fn(async () => [{ address: "93.184.216.34", family: 4 }]), +})); + // Import after mocks are set up const { fetchAgentCard, isAllowedUri } = await import("../registry/discovery.js"); @@ -131,7 +137,7 @@ describe("fetchAgentCard — data: URI handling", () => { expect(result).toBeNull(); // Confirm isAllowedUri rejects data:text/html - expect(isAllowedUri(uri)).toBe(false); + expect(await isAllowedUri(uri)).toBe(false); }); it("returns null when validateAgentCard rejects schema", async () => { @@ -150,6 +156,6 @@ describe("fetchAgentCard — data: URI handling", () => { // it doesn't go through the data: path. const httpsUri = "https://example.com/agent-card.json"; expect(httpsUri.startsWith("data:application/json")).toBe(false); - expect(isAllowedUri(httpsUri)).toBe(true); + expect(await isAllowedUri(httpsUri)).toBe(true); }); }); diff --git a/src/__tests__/social.test.ts b/src/__tests__/social.test.ts index 4f6d9528..4b09aaf2 100644 --- a/src/__tests__/social.test.ts +++ b/src/__tests__/social.test.ts @@ -9,6 +9,12 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import Database from "better-sqlite3"; import { MIGRATION_V7 } from "../state/schema.js"; +// Mock DNS resolution so isAllowedUri's rebinding check doesn't require +// network access in tests; example.com resolves to a public IP. +vi.mock("node:dns/promises", () => ({ + lookup: vi.fn(async () => [{ address: "93.184.216.34", family: 4 }]), +})); + // ─── Test helpers ─────────────────────────────────────────────── function createTestDb(): import("better-sqlite3").Database { @@ -608,17 +614,17 @@ describe("Discovery", () => { it("isAllowedUri blocks HTTP", async () => { const { isAllowedUri } = await import("../registry/discovery.js"); - expect(isAllowedUri("http://example.com/card.json")).toBe(false); + expect(await isAllowedUri("http://example.com/card.json")).toBe(false); }); it("isAllowedUri allows HTTPS", async () => { const { isAllowedUri } = await import("../registry/discovery.js"); - expect(isAllowedUri("https://example.com/card.json")).toBe(true); + expect(await isAllowedUri("https://example.com/card.json")).toBe(true); }); it("isAllowedUri blocks localhost", async () => { const { isAllowedUri } = await import("../registry/discovery.js"); - expect(isAllowedUri("https://localhost/card.json")).toBe(false); + expect(await isAllowedUri("https://localhost/card.json")).toBe(false); }); }); diff --git a/src/registry/discovery.ts b/src/registry/discovery.ts index 7ce04eda..9145c7c0 100644 --- a/src/registry/discovery.ts +++ b/src/registry/discovery.ts @@ -17,6 +17,7 @@ import { DEFAULT_DISCOVERY_CONFIG } from "../types.js"; import { queryAgent, getTotalAgents, getRegisteredAgentsByEvents } from "./erc8004.js"; import { keccak256, toBytes } from "viem"; import { createLogger } from "../observability/logger.js"; +import { lookup as dnsLookup } from "node:dns/promises"; const logger = createLogger("registry.discovery"); type Network = "mainnet" | "testnet"; @@ -27,11 +28,36 @@ const DISCOVERY_TIMEOUT_MS = 60_000; // ─── SSRF Protection ──────────────────────────────────────────── /** - * Check if a hostname resolves to an internal/private network. + * Unwrap an IPv6-mapped IPv4 address (e.g. "::ffff:127.0.0.1" or the + * compressed hex form "::ffff:7f00:1") to its dotted-quad IPv4 form. + * Returns the input unchanged if it isn't an IPv4-mapped address. + */ +function unwrapIPv4MappedAddress(address: string): string { + const stripped = address.replace(/^\[|\]$/g, ""); + const dotted = stripped.match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/i); + if (dotted) return dotted[1]; + const hex = stripped.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/i); + if (hex) { + const hi = parseInt(hex[1], 16); + const lo = parseInt(hex[2], 16); + return `${(hi >> 8) & 0xff}.${hi & 0xff}.${(lo >> 8) & 0xff}.${lo & 0xff}`; + } + return stripped; +} + +/** + * Check if a hostname or IP literal is an internal/private network address. * Blocks: 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, - * 192.168.0.0/16, 169.254.0.0/16, ::1, localhost, 0.0.0.0/8 + * 192.168.0.0/16, 169.254.0.0/16, ::1, localhost, 0.0.0.0/8, + * and IPv6-mapped forms of the above. + * + * This is a literal string check only — it does NOT resolve DNS, so a + * hostname that *resolves* to a private IP will not be caught here. + * Use `isAllowedUri` (which does resolve DNS) to fully validate a URI + * before fetching it. */ export function isInternalNetwork(hostname: string): boolean { + const normalized = unwrapIPv4MappedAddress(hostname); const blocked = [ /^127\./, /^10\./, @@ -42,19 +68,41 @@ export function isInternalNetwork(hostname: string): boolean { /^localhost$/i, /^0\./, ]; - return blocked.some(pattern => pattern.test(hostname)); + return blocked.some(pattern => pattern.test(normalized)); +} + +/** + * Resolve `hostname` via DNS and check whether ANY resolved address is + * an internal/private network address. Closes the DNS-rebinding gap where + * an attacker-controlled domain resolves to a private IP (CWE-918): + * checking the hostname string alone is not sufficient. + * + * Fails closed: if resolution errors (NXDOMAIN, no network, etc.), the + * hostname is treated as blocked, since we can't prove it's safe to fetch. + */ +async function resolvesToInternalNetwork(hostname: string): Promise { + if (isInternalNetwork(hostname)) return true; + try { + const results = await dnsLookup(hostname, { all: true }); + return results.some((r) => isInternalNetwork(r.address)); + } catch { + return true; + } } /** * Check if a URI is allowed for fetching. * Only https: and ipfs: schemes are permitted. - * Internal network addresses are blocked (SSRF protection). + * Internal network addresses are blocked (SSRF protection), including + * hostnames that resolve to a private IP via DNS rebinding. */ -export function isAllowedUri(uri: string): boolean { +export async function isAllowedUri(uri: string): Promise { try { const url = new URL(uri); if (!['https:', 'ipfs:'].includes(url.protocol)) return false; - if (url.protocol === 'https:' && isInternalNetwork(url.hostname)) return false; + if (url.protocol === 'https:' && (await resolvesToInternalNetwork(url.hostname))) { + return false; + } return true; } catch { return false; @@ -311,7 +359,7 @@ export async function fetchAgentCard( } // SSRF protection: validate URI before fetching - if (!isAllowedUri(uri)) { + if (!(await isAllowedUri(uri))) { logger.error(`Blocked URI (SSRF protection): ${uri}`); return null; }