diff --git a/package-lock.json b/package-lock.json index aaa597f1..8bcf3781 100644 --- a/package-lock.json +++ b/package-lock.json @@ -28,6 +28,7 @@ "file-type": "^21.1.1", "glob": "^10.3.10", "highlight.js": "^11.11.1", + "ipaddr.js": "^1.9.1", "isbinaryfile": "^5.0.4", "markdown-it": "^14.1.0", "md-to-pdf": "^5.2.5", diff --git a/package.json b/package.json index 7e45bea9..4d6968b8 100644 --- a/package.json +++ b/package.json @@ -107,6 +107,7 @@ "file-type": "^21.1.1", "glob": "^10.3.10", "highlight.js": "^11.11.1", + "ipaddr.js": "^1.9.1", "isbinaryfile": "^5.0.4", "markdown-it": "^14.1.0", "md-to-pdf": "^5.2.5", diff --git a/src/tools/filesystem.ts b/src/tools/filesystem.ts index 0476a7ca..e6e6beb9 100644 --- a/src/tools/filesystem.ts +++ b/src/tools/filesystem.ts @@ -11,6 +11,9 @@ import { getFileHandler, TextFileHandler } from '../utils/files/index.js'; import type { ReadOptions, FileResult, PdfPageItem } from '../utils/files/base.js'; import { isPdfFile } from "./mime-types.js"; import { parsePdfToMarkdown, editPdf, PdfOperations, PdfMetadata, parseMarkdownToPdf } from './pdf/index.js'; +import { fetchUrlValidated } from '../utils/urlSafety.js'; + +export { assertUrlIsFetchable } from '../utils/urlSafety.js'; import { isBinaryFile } from 'isbinaryfile'; // CONSTANTS SECTION - Consolidate all timeouts and thresholds @@ -364,9 +367,9 @@ export async function readFileFromUrl(url: string): Promise { const timeoutId = setTimeout(() => controller.abort(), FILE_OPERATION_TIMEOUTS.URL_FETCH); try { - const response = await fetch(url, { - signal: controller.signal - }); + // SSRF guard: validates the URL (and every redirect hop) before any + // request leaves the process. + const { response, finalUrl: currentUrl } = await fetchUrlValidated(url, controller.signal); // Clear the timeout since fetch completed clearTimeout(timeoutId); @@ -378,12 +381,15 @@ export async function readFileFromUrl(url: string): Promise { // Get MIME type from Content-Type header or infer from URL const contentType = response.headers.get('content-type') || 'text/plain'; const isImage = isImageFile(contentType); - const isPdf = isPdfFile(contentType) || url.toLowerCase().endsWith('.pdf'); + const isPdf = isPdfFile(contentType) || currentUrl.toLowerCase().endsWith('.pdf'); // NEW: Add PDF handling before image check if (isPdf) { - // Use URL directly - pdfreader handles URL downloads internally - const pdfResult = await parsePdfToMarkdown(url); + // Parse the bytes from the request we already validated. Handing the URL + // to the parser would download it a second time, resolving DNS again and + // bypassing the checks above. + const pdfBytes = new Uint8Array(await response.arrayBuffer()); + const pdfResult = await parsePdfToMarkdown(pdfBytes); return { content: "", diff --git a/src/tools/pdf/markdown.ts b/src/tools/pdf/markdown.ts index b6c5f0b1..da09adc0 100644 --- a/src/tools/pdf/markdown.ts +++ b/src/tools/pdf/markdown.ts @@ -5,6 +5,7 @@ import { mdToPdf } from 'md-to-pdf'; import type { PageRange } from './lib/pdf2md.js'; import { PdfParseResult, pdf2md } from './lib/pdf2md.js'; import { CONFIG_FILE } from '../../config.js'; +import { fetchUrlValidated } from '../../utils/urlSafety.js'; const isUrl = (source: string): boolean => source.startsWith('http://') || source.startsWith('https://'); @@ -259,9 +260,14 @@ export function ensureChromeAvailable(): void { }); } -async function loadPdfToBuffer(source: string): Promise { +async function loadPdfToBuffer(source: string | Uint8Array): Promise { + if (typeof source !== 'string') { + return source; + } if (isUrl(source)) { - const response = await fetch(source); + // Same SSRF guard as read_file's URL path — this parser must not be a + // way to fetch URLs that the guard would refuse. + const { response } = await fetchUrlValidated(source); return await response.arrayBuffer(); } else { return await fs.readFile(source); @@ -270,8 +276,13 @@ async function loadPdfToBuffer(source: string): Promise { /** * Convert PDF to Markdown using @opendocsg/pdf2md + * + * Callers that already downloaded the PDF should pass the bytes rather than the + * URL — it avoids a second fetch, and the original request's validation stays + * authoritative. URL sources are fetched through the same SSRF guard as + * read_file. */ -export async function parsePdfToMarkdown(source: string, pageNumbers: number[] | PageRange = []): Promise { +export async function parsePdfToMarkdown(source: string | Uint8Array, pageNumbers: number[] | PageRange = []): Promise { try { const data = await loadPdfToBuffer(source); diff --git a/src/utils/urlSafety.ts b/src/utils/urlSafety.ts new file mode 100644 index 00000000..def4a7dd --- /dev/null +++ b/src/utils/urlSafety.ts @@ -0,0 +1,170 @@ +import dns from 'dns/promises'; +import http from 'http'; +import https from 'https'; +import type { LookupFunction } from 'net'; +import ipaddr from 'ipaddr.js'; +import fetch from 'cross-fetch'; + +// Follow at most this many redirects when reading a URL, re-validating each hop. +const MAX_URL_REDIRECTS = 5; + +/** + * Returns true if an IP address is not publicly routable — loopback, private, + * link-local (including the 169.254.169.254 cloud metadata endpoint), unique + * local, CGNAT, multicast, or otherwise reserved. Anything that isn't a valid + * IP is treated as blocked so a malformed value can't slip through. + * + * Uses ipaddr.js for CIDR-correct classification across IPv4 and IPv6: only the + * `unicast` range is publicly routable, everything else is refused. IPv4-mapped + * IPv6 (e.g. `::ffff:127.0.0.1` and its hex form `::ffff:7f00:1`) is unwrapped + * to its IPv4 address first so the underlying range is what gets checked. + */ +function isBlockedAddress(ip: string): boolean { + let addr: ipaddr.IPv4 | ipaddr.IPv6; + try { + addr = ipaddr.parse(ip.split('%')[0]); // drop any IPv6 zone id + } catch { + return true; + } + if (addr.kind() === 'ipv6' && (addr as ipaddr.IPv6).isIPv4MappedAddress()) { + addr = (addr as ipaddr.IPv6).toIPv4Address(); + } + return addr.range() !== 'unicast'; +} + +/** An address that passed the SSRF checks, in `dns.lookup` shape. */ +export interface ValidatedAddress { + address: string; + family: number; +} + +/** + * Guards against SSRF before a URL is fetched. Allows only http(s), and rejects + * hosts that are — or that resolve to — non-public addresses. Every resolved + * address is checked, so a hostname pointing at an internal IP is blocked too. + * + * @param rawUrl The URL about to be fetched + * @returns The addresses the host was validated as — connect to these rather + * than resolving the hostname again, or the second lookup can be + * rebound to an internal address after the check passed + * @throws Error if the URL is not safe to fetch + */ +export async function assertUrlIsFetchable(rawUrl: string): Promise { + let parsed: URL; + try { + parsed = new URL(rawUrl); + } catch { + throw new Error(`Invalid URL: ${rawUrl}`); + } + + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + throw new Error(`URL protocol not allowed: ${parsed.protocol} — only http and https can be read`); + } + + const hostname = parsed.hostname.replace(/^\[|\]$/g, ''); // strip IPv6 brackets + + if (ipaddr.isValid(hostname)) { + if (isBlockedAddress(hostname)) { + throw new Error(`Refusing to fetch a URL that targets a non-public address: ${hostname}`); + } + return [{ address: hostname, family: ipaddr.parse(hostname.split('%')[0]).kind() === 'ipv6' ? 6 : 4 }]; + } + + let resolved: Array<{ address: string; family: number }>; + try { + resolved = await dns.lookup(hostname, { all: true }); + } catch { + throw new Error(`Could not resolve host: ${hostname}`); + } + if (resolved.length === 0) { + throw new Error(`Could not resolve host: ${hostname}`); + } + for (const { address } of resolved) { + if (isBlockedAddress(address)) { + throw new Error( + `Refusing to fetch a URL that resolves to a non-public address: ${hostname} -> ${address}` + ); + } + } + return resolved.map(({ address, family }) => ({ address, family })); +} + +/** + * An http(s) agent that connects only to the given pre-validated addresses + * instead of resolving the hostname again. The socket-level lookup re-resolving + * the name is what makes DNS rebinding work: a host can pass the guard and then + * serve an internal address to the connection's own lookup. TLS SNI and + * certificate checks still run against the original hostname. + */ +function pinnedAgentFor(url: URL, addresses: ValidatedAddress[]): http.Agent { + const lookup: LookupFunction = (_hostname, options, callback) => { + if (options.all) { + callback(null, addresses.map(({ address, family }) => ({ address, family }))); + } else { + callback(null, addresses[0].address, addresses[0].family); + } + }; + return url.protocol === 'https:' ? new https.Agent({ lookup }) : new http.Agent({ lookup }); +} + +/** + * Releases a response we are not going to return (a redirect hop), so its + * unread body does not hold the connection open. Handles both body shapes: + * a WHATWG stream (`cancel`) and node-fetch's Node stream (`destroy`). + */ +function discardResponseBody(response: Awaited>): void { + const body = response.body as { cancel?: () => Promise; destroy?: () => void } | null; + if (body && typeof body.cancel === 'function') { + body.cancel().catch(() => {}); + } else if (body && typeof body.destroy === 'function') { + body.destroy(); + } +} + +/** + * Fetches a URL with the SSRF guard applied to the initial request and to every + * redirect hop — a public URL must not be able to redirect us onto an internal + * address. Each hop's socket connects to the addresses the guard validated + * (never re-resolving the hostname), closing the DNS-rebinding window between + * check and connect. All code that downloads a user-supplied URL must go + * through this (or pass already-downloaded bytes onward) rather than calling + * fetch directly. + * + * @param fetchImpl Overridable for hermetic tests; defaults to cross-fetch + * @returns The final response and the URL it actually came from + */ +export async function fetchUrlValidated( + url: string, + signal?: AbortSignal, + fetchImpl: typeof fetch = fetch +): Promise<{ response: Awaited>; finalUrl: string }> { + let currentUrl = url; + let response!: Awaited>; + for (let hop = 0; ; hop++) { + const addresses = await assertUrlIsFetchable(currentUrl); + const parsed = new URL(currentUrl); + response = await fetchImpl(currentUrl, { + signal, + redirect: 'manual', + // cross-fetch is node-fetch on Node, which connects through this + // agent; runtimes with WHATWG fetch ignore the extra field. + agent: pinnedAgentFor(parsed, addresses) + } as RequestInit); + + if (response.status < 300 || response.status >= 400) { + break; + } + + const location = response.headers.get('location'); + if (!location) { + break; // redirect status without a target — treat as final + } + discardResponseBody(response); // this hop's body is never read + if (hop >= MAX_URL_REDIRECTS) { + throw new Error(`Too many redirects while fetching URL: ${url}`); + } + currentUrl = new URL(location, currentUrl).toString(); + } + + return { response, finalUrl: currentUrl }; +} diff --git a/test/test-read-url-ssrf.js b/test/test-read-url-ssrf.js new file mode 100644 index 00000000..74959c09 --- /dev/null +++ b/test/test-read-url-ssrf.js @@ -0,0 +1,127 @@ +import assert from 'assert'; +import { assertUrlIsFetchable, readFileFromUrl } from '../dist/tools/filesystem.js'; +import { fetchUrlValidated } from '../dist/utils/urlSafety.js'; +import { parsePdfToMarkdown } from '../dist/tools/pdf/index.js'; + +/** + * Regression tests for #587 / #560: read_file with isUrl fetched arbitrary URLs + * with no SSRF protection, so a URL like http://169.254.169.254/… could read + * cloud instance metadata or reach internal services. + * + * assertUrlIsFetchable() now rejects non-http(s) schemes and any host that is + * (or resolves to) a non-public address. These cases short-circuit before any + * network access, so the test is hermetic — no outbound requests are made. + */ + +let passed = 0; +const ok = (msg) => { passed++; console.log(`✓ ${msg}`); }; + +// A minimal single-page PDF, inlined so the PDF check needs no fixture or network. +const MINIMAL_PDF_BASE64 = + 'JVBERi0xLjQKMSAwIG9iago8PCAvVHlwZSAvQ2F0YWxvZyAvUGFnZXMgMiAwIFIgPj4KZW5kb2JqCjIgMCBvYmoK' + + 'PDwgL1R5cGUgL1BhZ2VzIC9LaWRzIFszIDAgUl0gL0NvdW50IDEgPj4KZW5kb2JqCjMgMCBvYmoKPDwgL1R5cGUg' + + 'L1BhZ2UgL1BhcmVudCAyIDAgUiAvTWVkaWFCb3ggWzAgMCAyMDAgMjAwXSAvUmVzb3VyY2VzIDw8IC9Gb250IDw8' + + 'IC9GMSA0IDAgUiA+PiA+PiAvQ29udGVudHMgNSAwIFIgPj4KZW5kb2JqCjQgMCBvYmoKPDwgL1R5cGUgL0ZvbnQg' + + 'L1N1YnR5cGUgL1R5cGUxIC9CYXNlRm9udCAvSGVsdmV0aWNhID4+CmVuZG9iago1IDAgb2JqCjw8IC9MZW5ndGgg' + + 'MzMgPj4Kc3RyZWFtCkJUIC9GMSAyNCBUZiAyMCAxMDAgVGQgKEhpKSBUaiBFVAplbmRzdHJlYW0KZW5kb2JqCnhy' + + 'ZWYKMCA2CjAwMDAwMDAwMDAgNjU1MzUgZiAKMDAwMDAwMDAwOSAwMDAwMCBuIAowMDAwMDAwMDU4IDAwMDAwIG4g' + + 'CjAwMDAwMDAxMTUgMDAwMDAgbiAKMDAwMDAwMDI0MSAwMDAwMCBuIAowMDAwMDAwMzExIDAwMDAwIG4gCnRyYWls' + + 'ZXIKPDwgL1NpemUgNiAvUm9vdCAxIDAgUiA+PgpzdGFydHhyZWYKMzk0CiUlRU9GCg=='; + +const BLOCKED_URLS = [ + 'http://169.254.169.254/latest/meta-data/iam/security-credentials/', // AWS metadata + 'http://[fd00::1]/', // IPv6 unique-local + 'http://127.0.0.1:8080/admin', // loopback + 'http://localhost/internal', // resolves to loopback + 'http://10.0.0.1/', // private + 'http://192.168.1.1/', // private + 'http://172.16.5.4/', // private + 'http://[::1]/', // IPv6 loopback + 'http://[fe90::1]/', // IPv6 link-local (fe80::/10, not just fe80::) + 'http://[::ffff:7f00:1]/', // IPv4-mapped loopback in hex form (127.0.0.1) + 'file:///etc/passwd', // non-http scheme + 'ftp://example.com/secret', // non-http scheme +]; + +async function run() { + for (const url of BLOCKED_URLS) { + await assert.rejects( + () => assertUrlIsFetchable(url), + (err) => err instanceof Error && /not allowed|non-public|Invalid URL|resolve/i.test(err.message), + `assertUrlIsFetchable should reject ${url}` + ); + } + ok(`assertUrlIsFetchable rejects all ${BLOCKED_URLS.length} SSRF / non-http URLs`); + + // readFile(isUrl) routes through the same guard, so the metadata PoC is + // blocked before any request leaves the process. + await assert.rejects( + () => readFileFromUrl('http://169.254.169.254/latest/meta-data/'), + /non-public/i, + 'readFileFromUrl should refuse the cloud metadata endpoint' + ); + ok('readFileFromUrl refuses the cloud metadata endpoint'); + + // A public address with a valid scheme passes validation. An IP literal is + // used so the check needs no outbound DNS and stays hermetic; no request is + // made — we only assert the guard itself does not reject it. + await assert.doesNotReject( + () => assertUrlIsFetchable('https://8.8.8.8/file.txt'), + 'a public https URL must pass the guard' + ); + ok('a public https address passes the guard'); + + // readFileFromUrl hands the PDF parser the bytes it already fetched through the + // guard; passing the URL made the parser download it a second time, re-resolving + // DNS outside these checks. That requires the parser to accept bytes — it used + // to treat every source as a path or URL. + const parsed = await parsePdfToMarkdown(Buffer.from(MINIMAL_PDF_BASE64, 'base64')); + assert.strictEqual(parsed.metadata.totalPages, 1, 'byte input should parse as a PDF'); + ok('parsePdfToMarkdown accepts already-fetched bytes'); + + // A public URL must not be able to redirect us onto an internal address. + // The fetch is stubbed (fetchUrlValidated takes the implementation as its + // third parameter precisely so this stays hermetic): the stub answers the + // public entry URL with a 302 pointing at cloud metadata, and records every + // request it is asked to make. An IP-literal entry URL keeps validation off + // the real DNS resolver. + const requested = []; + let capturedInit; + const redirectingFetch = async (url, init) => { + requested.push(String(url)); + capturedInit = init; + return { + status: 302, + headers: { get: (name) => (name.toLowerCase() === 'location' ? 'http://169.254.169.254/latest/meta-data/' : null) }, + body: null, + }; + }; + await assert.rejects( + () => fetchUrlValidated('https://8.8.8.8/entry', undefined, redirectingFetch), + /non-public/i, + 'a redirect onto the metadata endpoint must be rejected' + ); + assert.deepStrictEqual( + requested, + ['https://8.8.8.8/entry'], + 'the internal redirect target must never be requested' + ); + ok('redirect from a public URL to the metadata endpoint is blocked before any request to it'); + + // The connection must go to the address that passed validation, not wherever + // the hostname resolves at connect time (DNS rebinding). The fetch options + // carry an agent whose lookup answers from the validated set; ask it directly + // and confirm it returns the pinned address without touching the resolver. + assert.ok(capturedInit && capturedInit.agent, 'fetch should be given a pinning agent'); + const pinned = await new Promise((resolve, reject) => { + capturedInit.agent.options.lookup('rebind.example', {}, (err, address, family) => { + if (err) reject(err); else resolve({ address, family }); + }); + }); + assert.strictEqual(pinned.address, '8.8.8.8', 'socket lookup must return the validated address'); + ok('socket-level lookup is pinned to the validated address'); +} + +run() + .then(() => { console.log(`\nPASS (${passed}/6)`); process.exit(0); }) + .catch((e) => { console.error(`\nFAIL: ${e.message}`); process.exit(1); });