From d34a97374126e6833dfa5c055925f4a27058cf80 Mon Sep 17 00:00:00 2001 From: Sebastion Date: Tue, 7 Apr 2026 02:11:15 +0100 Subject: [PATCH] fix: eliminate DNS rebinding TOCTOU by pinning resolved IP for fetch Resolve DNS once via resolveAndPin(), validate the IP is not private, then rewrite the URL to use the validated IP directly with appropriate Host header and TLS SNI. This prevents DNS rebinding attacks where a second DNS resolution (inside fetch) could return a different IP. Also switches to manual redirect handling so every redirect hop is validated (protocol, hostname, and resolved IP) before the request is made, preventing SSRF via open redirects. CWE-918 --- src/Fetcher.ssrf.test.ts | 188 +++++++++++++++++++++++++++++++++++++++ src/Fetcher.test.ts | 97 ++++++++++++++------ src/Fetcher.ts | 141 +++++++++++++++++++++-------- 3 files changed, 362 insertions(+), 64 deletions(-) create mode 100644 src/Fetcher.ssrf.test.ts diff --git a/src/Fetcher.ssrf.test.ts b/src/Fetcher.ssrf.test.ts new file mode 100644 index 0000000..0b2e154 --- /dev/null +++ b/src/Fetcher.ssrf.test.ts @@ -0,0 +1,188 @@ +/** + * PoC test for CWE-918: DNS rebinding TOCTOU race in Fetcher.ts + * + * The vulnerability: validateResolvedIp() resolves DNS and checks the IP, + * then fetch() resolves DNS *again* internally. An attacker controlling DNS + * can return a public IP for validation and a private IP for the actual fetch. + * + * Additionally, redirects are followed automatically before validation, + * meaning requests to private IPs from redirect targets have already completed + * by the time response.url is checked. + * + * The fix: resolve DNS once, validate the IP, and pin the resolved IP by + * rewriting the fetch URL to use the IP directly (with Host header and TLS SNI). + * Redirects are followed manually with validation at each hop. + */ +import { describe, it, expect, beforeEach, afterEach, jest } from "bun:test"; +import dns from "node:dns"; +import { Fetcher } from "./Fetcher"; + +describe("CWE-918: DNS rebinding TOCTOU", () => { + const originalLookup = dns.promises.lookup; + const originalFetch = globalThis.fetch; + + beforeEach(() => { + globalThis.fetch = originalFetch; + dns.promises.lookup = originalLookup; + Fetcher.hasYtDlp = false; + }); + + afterEach(() => { + dns.promises.lookup = originalLookup; + globalThis.fetch = originalFetch; + }); + + it("should pin resolved IP: fetch URL uses validated IP, not hostname", async () => { + const attackUrl = "http://rebind.test:8080/path"; + + // Mock DNS to return public IP (validation passes) + dns.promises.lookup = (async () => { + return { address: "93.184.216.34", family: 4 }; + }) as any; + + // Spy on real fetch to see what URL it receives + const fetchSpy = jest.fn().mockResolvedValue( + new Response("ok", { status: 200 }) + ); + globalThis.fetch = fetchSpy as any; + + await Fetcher.html({ url: attackUrl }); + + // After fix: fetch should be called with the resolved IP, not the hostname. + // This eliminates the TOCTOU because the runtime can't re-resolve DNS. + expect(fetchSpy).toHaveBeenCalled(); + const fetchedUrl = fetchSpy.mock.calls[0][0] as string; + expect(fetchedUrl).toContain("93.184.216.34"); + expect(fetchedUrl).not.toContain("rebind.test"); + }); + + it("should validate redirect targets BEFORE following them", async () => { + const startUrl = "http://public.example.com/start"; + + dns.promises.lookup = (async (hostname: string) => { + if (hostname === "public.example.com") { + return { address: "93.184.216.34", family: 4 }; + } + return { address: "127.0.0.1", family: 4 }; + }) as any; + + // First fetch returns a redirect to a private IP + const fetchSpy = jest.fn().mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { Location: "http://127.0.0.1/secret" }, + }) + ); + globalThis.fetch = fetchSpy as any; + + const result = await Fetcher.html({ url: startUrl }); + + // The redirect to 127.0.0.1 must be blocked + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("private"); + + // fetch should only have been called once — the redirect must NOT have been followed + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("should include Host header when pinning IP", async () => { + const url = "http://example.com/page"; + + dns.promises.lookup = (async () => { + return { address: "93.184.216.34", family: 4 }; + }) as any; + + const fetchSpy = jest.fn().mockResolvedValue( + new Response("ok", { status: 200 }) + ); + globalThis.fetch = fetchSpy as any; + + await Fetcher.html({ url }); + + expect(fetchSpy).toHaveBeenCalled(); + const fetchOptions = fetchSpy.mock.calls[0][1] as RequestInit; + const headers = fetchOptions.headers as Record; + expect(headers["Host"]).toBe("example.com"); + }); + + it("should properly follow valid redirects with IP pinning", async () => { + const startUrl = "http://example.com/start"; + + dns.promises.lookup = (async (hostname: string) => { + if (hostname === "example.com") { + return { address: "93.184.216.34", family: 4 }; + } + if (hostname === "cdn.example.com") { + return { address: "104.18.26.120", family: 4 }; + } + return originalLookup(hostname); + }) as any; + + const fetchSpy = jest.fn() + .mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { Location: "http://cdn.example.com/page" }, + }) + ) + .mockResolvedValueOnce( + new Response("final page", { status: 200 }) + ); + globalThis.fetch = fetchSpy as any; + + const result = await Fetcher.html({ url: startUrl }); + expect(result.isError).toBe(false); + expect(result.content[0].text).toContain("final page"); + + // Both fetch calls should use IP addresses, not hostnames + const firstUrl = fetchSpy.mock.calls[0][0] as string; + const secondUrl = fetchSpy.mock.calls[1][0] as string; + expect(firstUrl).toContain("93.184.216.34"); + expect(secondUrl).toContain("104.18.26.120"); + }); + + it("should handle relative redirect URLs", async () => { + const startUrl = "http://example.com/start"; + + dns.promises.lookup = (async () => { + return { address: "93.184.216.34", family: 4 }; + }) as any; + + const fetchSpy = jest.fn() + .mockResolvedValueOnce( + new Response(null, { + status: 301, + headers: { Location: "/final" }, + }) + ) + .mockResolvedValueOnce( + new Response("final", { status: 200 }) + ); + globalThis.fetch = fetchSpy as any; + + const result = await Fetcher.html({ url: startUrl }); + expect(result.isError).toBe(false); + expect(result.content[0].text).toContain("final"); + }); + + it("should reject too many redirects", async () => { + const startUrl = "http://example.com/loop"; + + dns.promises.lookup = (async () => { + return { address: "93.184.216.34", family: 4 }; + }) as any; + + // Create an infinite redirect loop + const fetchSpy = jest.fn().mockResolvedValue( + new Response(null, { + status: 302, + headers: { Location: "http://example.com/loop" }, + }) + ); + globalThis.fetch = fetchSpy as any; + + const result = await Fetcher.html({ url: startUrl }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("redirect"); + }); +}); diff --git a/src/Fetcher.test.ts b/src/Fetcher.test.ts index fd82bdd..a43432e 100644 --- a/src/Fetcher.test.ts +++ b/src/Fetcher.test.ts @@ -43,6 +43,8 @@ describe("Fetcher", () => { it("should return the raw HTML content", async () => { mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(mockHtml), }); @@ -74,6 +76,8 @@ describe("Fetcher", () => { const mockJson = { key: "value" }; mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(JSON.stringify(mockJson)), }); @@ -135,6 +139,8 @@ describe("Fetcher", () => { it("should return readable content as markdown", async () => { mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(articleHtml), }); @@ -146,6 +152,8 @@ describe("Fetcher", () => { it("should return error when Readability cannot parse", async () => { mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(""), }); @@ -206,29 +214,43 @@ describe("Fetcher", () => { }); it("should block redirects to private IPs", async () => { - mockFetch.mockResolvedValueOnce({ - ok: true, - url: "http://127.0.0.1/internal", - text: jest.fn().mockResolvedValueOnce("secret"), - }); + // With manual redirect handling, the first fetch returns a 302 to a private IP + mockFetch.mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { Location: "http://127.0.0.1/internal" }, + }) + ); const result = await Fetcher.html({ url: "https://example.com" }); expect(result.isError).toBe(true); expect(result.content[0].text).toContain("private address"); - // Should NOT be double-wrapped with "Failed to fetch" prefix - expect(result.content[0].text).not.toContain("Failed to fetch"); }); it("should allow redirects to public URLs", async () => { - mockFetch.mockResolvedValueOnce({ - ok: true, - url: "https://cdn.example.com/page", - text: jest.fn().mockResolvedValueOnce("ok"), - }); + const lookupSpy = spyOn(dns.promises, "lookup") + .mockResolvedValueOnce({ address: "93.184.216.34", family: 4 } as any) // initial URL + .mockResolvedValueOnce({ address: "104.18.26.120", family: 4 } as any); // redirect target + + // First fetch returns redirect, second returns content + mockFetch + .mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { Location: "https://cdn.example.com/page" }, + }) + ) + .mockResolvedValueOnce({ + ok: true, + status: 200, + headers: new Headers(), + text: jest.fn().mockResolvedValueOnce("ok"), + }); const result = await Fetcher.html({ url: "https://example.com" }); expect(result.isError).toBe(false); expect(result.content[0].text).toBe("ok"); + lookupSpy.mockRestore(); }); }); @@ -236,6 +258,8 @@ describe("Fetcher", () => { it("should pass proxy option to fetch when provided", async () => { mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce("ok"), }); @@ -248,6 +272,8 @@ describe("Fetcher", () => { it("should not include proxy option when not provided", async () => { mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce("ok"), }); @@ -280,10 +306,14 @@ describe("Fetcher", () => { mockFetch .mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(pageHtml), }) .mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(captionXml), }); @@ -317,10 +347,14 @@ describe("Fetcher", () => { mockFetch .mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(pageHtml), }) .mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(captionXml), }); @@ -340,6 +374,8 @@ describe("Fetcher", () => { mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce(pageHtml), }); @@ -368,14 +404,16 @@ describe("Fetcher", () => { it("should block post-redirect hostnames that resolve to private IPs", async () => { const lookupSpy = spyOn(dns.promises, "lookup") - .mockResolvedValueOnce({ address: "93.184.216.34", family: 4 } as any) // pre-fetch: public - .mockResolvedValueOnce({ address: "10.0.0.1", family: 4 } as any); // post-redirect: private - - mockFetch.mockResolvedValueOnce({ - ok: true, - url: "https://internal.evil.com/secret", - text: jest.fn().mockResolvedValueOnce("secret data"), - }); + .mockResolvedValueOnce({ address: "93.184.216.34", family: 4 } as any) // initial URL: public + .mockResolvedValueOnce({ address: "10.0.0.1", family: 4 } as any); // redirect target: private + + // First fetch returns a redirect to a hostname that resolves to private IP + mockFetch.mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { Location: "https://internal.evil.com/secret" }, + }) + ); const result = await Fetcher.html({ url: "https://example.com" }); expect(result.isError).toBe(true); @@ -391,6 +429,8 @@ describe("Fetcher", () => { mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockResolvedValueOnce("ok"), }); @@ -443,8 +483,8 @@ describe("Fetcher", () => { const captionXml = `Olá`; mockFetch - .mockResolvedValueOnce({ ok: true, text: jest.fn().mockResolvedValueOnce(pageHtml) }) - .mockResolvedValueOnce({ ok: true, text: jest.fn().mockResolvedValueOnce(captionXml) }); + .mockResolvedValueOnce({ ok: true, status: 200, headers: new Headers(), text: jest.fn().mockResolvedValueOnce(pageHtml) }) + .mockResolvedValueOnce({ ok: true, status: 200, headers: new Headers(), text: jest.fn().mockResolvedValueOnce(captionXml) }); const result = await Fetcher.youtubeTranscript({ url: "https://www.youtube.com/watch?v=test", @@ -472,8 +512,8 @@ describe("Fetcher", () => { const captionXml = `Hola`; mockFetch - .mockResolvedValueOnce({ ok: true, text: jest.fn().mockResolvedValueOnce(pageHtml) }) - .mockResolvedValueOnce({ ok: true, text: jest.fn().mockResolvedValueOnce(captionXml) }); + .mockResolvedValueOnce({ ok: true, status: 200, headers: new Headers(), text: jest.fn().mockResolvedValueOnce(pageHtml) }) + .mockResolvedValueOnce({ ok: true, status: 200, headers: new Headers(), text: jest.fn().mockResolvedValueOnce(captionXml) }); const result = await Fetcher.youtubeTranscript({ url: "https://www.youtube.com/watch?v=test", @@ -502,7 +542,8 @@ describe("Fetcher", () => { it("should reject responses with Content-Length exceeding limit", async () => { mockFetch.mockResolvedValueOnce({ ok: true, - headers: { get: (h: string) => h === "content-length" ? "999999999999" : null }, + status: 200, + headers: new Headers({ "content-length": "999999999999" }), text: jest.fn().mockResolvedValueOnce("data"), }); @@ -514,7 +555,8 @@ describe("Fetcher", () => { it("should allow responses within size limit", async () => { mockFetch.mockResolvedValueOnce({ ok: true, - headers: { get: (h: string) => h === "content-length" ? "100" : null }, + status: 200, + headers: new Headers({ "content-length": "100" }), text: jest.fn().mockResolvedValueOnce("ok"), }); @@ -528,6 +570,7 @@ describe("Fetcher", () => { mockFetch.mockResolvedValueOnce({ ok: false, status: 404, + headers: new Headers(), }); const result = await Fetcher.html(mockRequest); @@ -560,6 +603,8 @@ describe("Fetcher", () => { it("should produce a string text field when response processing throws a non-Error", async () => { mockFetch.mockResolvedValueOnce({ ok: true, + status: 200, + headers: new Headers(), text: jest.fn().mockRejectedValueOnce("string error"), }); diff --git a/src/Fetcher.ts b/src/Fetcher.ts index 256bdfd..f58edf4 100644 --- a/src/Fetcher.ts +++ b/src/Fetcher.ts @@ -6,6 +6,8 @@ import dns from "node:dns"; import { RequestPayload, YouTubeTranscriptPayload, downloadLimit, maxResponseBytes } from "./types.js"; import { YouTubeTranscript } from "./YouTubeTranscript.js"; +const MAX_REDIRECTS = 20; + export class Fetcher { private static applyLengthLimits(text: string, maxLength: number, startIndex: number): string { if (startIndex >= text.length) { @@ -34,22 +36,50 @@ export class Fetcher { } } - private static async validateResolvedIp(url: string): Promise { - const hostname = new URL(url).hostname; + /** + * Resolve DNS for the given URL's hostname, validate that the resolved IP + * is not private, and return a rewritten URL that uses the resolved IP + * directly. This eliminates DNS rebinding TOCTOU attacks by ensuring the + * validated IP is the one actually connected to. + * + * Returns { pinnedUrl, hostname } where pinnedUrl has the IP in place of + * the hostname, and hostname is the original hostname (for Host header / SNI). + */ + private static async resolveAndPin(url: string): Promise<{ pinnedUrl: string; hostname: string }> { + const parsedUrl = new URL(url); + const hostname = parsedUrl.hostname; const bareHostname = hostname.startsWith('[') && hostname.endsWith(']') ? hostname.slice(1, -1) : hostname; + + let resolvedAddress: string; + let family: number; try { - const { address } = await dns.promises.lookup(bareHostname); - if (is_ip_private(address)) { - throw new Error( - `Fetcher blocked request: hostname "${bareHostname}" resolved to private IP "${address}". This prevents DNS rebinding SSRF attacks.`, - ); - } + const result = await dns.promises.lookup(bareHostname); + resolvedAddress = result.address; + family = result.family; } catch (e) { if (e instanceof Error && e.message.includes('Fetcher blocked')) throw e; - // DNS lookup failures (e.g. non-resolvable hostnames) are not SSRF — let fetch handle them + // DNS lookup failures (e.g. non-resolvable hostnames) are not SSRF — let fetch handle them. + // Return the original URL unpinned so fetch produces a normal DNS error. + return { pinnedUrl: url, hostname: bareHostname }; } + + if (is_ip_private(resolvedAddress)) { + throw new Error( + `Fetcher blocked request: hostname "${bareHostname}" resolved to private IP "${resolvedAddress}". This prevents DNS rebinding SSRF attacks.`, + ); + } + + // Rewrite the URL to use the resolved IP directly, preventing re-resolution + const pinnedUrl = new URL(url); + if (family === 6) { + pinnedUrl.hostname = `[${resolvedAddress}]`; + } else { + pinnedUrl.hostname = resolvedAddress; + } + + return { pinnedUrl: pinnedUrl.toString(), hostname: bareHostname }; } private static async _fetch({ @@ -58,41 +88,76 @@ export class Fetcher { proxy, }: RequestPayload): Promise { this.validateUrl(url); - await this.validateResolvedIp(url); - let response: Response; - try { - response = await fetch(url, { - headers: { - "User-Agent": - "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36", - ...headers, - }, - // Note: proxy is a Bun-specific fetch option. On Node.js, this option is silently ignored. - // To use a proxy on Node.js, you would need an HTTP agent library like http-proxy-agent. - ...(proxy ? { proxy } : {}), - } as RequestInit); - } catch (e: unknown) { - if (e instanceof Error) { - throw new Error(`Failed to fetch ${url}: ${e.message}`); + const { pinnedUrl, hostname } = await this.resolveAndPin(url); + + const parsedUrl = new URL(url); + const originalHost = parsedUrl.host; // includes port if non-default + + let currentUrl = pinnedUrl; + let currentOriginalHost = originalHost; + let currentHostname = hostname; + + for (let redirectCount = 0; redirectCount <= MAX_REDIRECTS; redirectCount++) { + let response: Response; + try { + response = await fetch(currentUrl, { + headers: { + "User-Agent": + "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36", + ...headers, + "Host": currentOriginalHost, + }, + redirect: "manual", + // Note: proxy is a Bun-specific fetch option. On Node.js, this option is silently ignored. + // To use a proxy on Node.js, you would need an HTTP agent library like http-proxy-agent. + ...(proxy ? { proxy } : {}), + // Set TLS SNI to the original hostname for HTTPS + ...(new URL(currentUrl).protocol === 'https:' ? { tls: { serverName: currentHostname } } : {}), + } as RequestInit); + } catch (e: unknown) { + if (e instanceof Error) { + throw new Error(`Failed to fetch ${url}: ${e.message}`); + } + throw new Error(`Failed to fetch ${url}: Unknown error`); } - throw new Error(`Failed to fetch ${url}: Unknown error`); - } - if (response.url && response.url !== url) { - this.validateUrl(response.url); - await this.validateResolvedIp(response.url); - } + // Handle redirects manually to validate each hop + if (response.status >= 300 && response.status < 400) { + const location = response.headers.get("location"); + if (!location) { + throw new Error(`Failed to fetch ${url}: Redirect with no Location header`); + } - if (!response.ok) { - throw new Error(`Failed to fetch ${url}: HTTP error: ${response.status}`); - } + // Resolve relative redirects against the original (non-pinned) URL + const originalUrl = new URL(currentUrl); + originalUrl.hostname = currentHostname.includes(':') + ? `[${currentHostname}]` + : currentHostname; + const redirectTarget = new URL(location, originalUrl.toString()).toString(); + + // Validate and pin the redirect target before following + this.validateUrl(redirectTarget); + const redirectParsed = new URL(redirectTarget); + const redirectResult = await this.resolveAndPin(redirectTarget); + currentUrl = redirectResult.pinnedUrl; + currentOriginalHost = redirectParsed.host; + currentHostname = redirectResult.hostname; + continue; + } + + if (!response.ok) { + throw new Error(`Failed to fetch ${url}: HTTP error: ${response.status}`); + } + + const contentLength = response.headers?.get?.("content-length"); + if (contentLength && parseInt(contentLength, 10) > maxResponseBytes) { + throw new Error(`Response too large: ${contentLength} bytes exceeds ${maxResponseBytes} byte limit`); + } - const contentLength = response.headers?.get?.("content-length"); - if (contentLength && parseInt(contentLength, 10) > maxResponseBytes) { - throw new Error(`Response too large: ${contentLength} bytes exceeds ${maxResponseBytes} byte limit`); + return response; } - return response; + throw new Error(`Failed to fetch ${url}: Too many redirects (max ${MAX_REDIRECTS})`); } private static async readResponseText(response: Response): Promise {