diff --git a/server/src/__tests__/remote-http-rebinding.test.ts b/server/src/__tests__/remote-http-rebinding.test.ts new file mode 100644 index 00000000000..7910e622919 --- /dev/null +++ b/server/src/__tests__/remote-http-rebinding.test.ts @@ -0,0 +1,347 @@ +import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; +import { connect as netConnect, type AddressInfo, type Socket } from "node:net"; +import { gzipSync } from "node:zlib"; +import { afterEach, describe, expect, it } from "vitest"; + +import { guardedRemoteHttpFetch, type RemoteHttpSocketFactory } from "../services/remote-http-fetch.js"; + +/** + * PAP-17098 — DNS-rebinding regression coverage for outbound MCP/OAuth calls. + * + * The vulnerability was a TOCTOU: the guard resolved a hostname, approved it, + * and then handed the *hostname* to global `fetch`, which resolved it a second + * time. A name server the attacker controls can answer with a public address for + * the first lookup and a loopback/private/link-local address for the second, + * pointing the connection at an internal service or the cloud metadata endpoint. + * + * These tests model that name server deterministically: `lookup` returns a public + * address on its first call and `127.0.0.1` on every call after it. Nothing here + * touches real DNS or the real network — `socketFactory` stands in for the + * network so a "public" address is a loopback listener that reports itself as + * public, exactly as it would look to the kernel. + */ + +const PUBLIC_ADDRESS = "93.184.216.34"; +const REBIND_HOST = "mcp.rebind.test"; + +type TestServer = { + server: Server; + port: number; + requests: Array<{ url: string; host: string | undefined }>; + connections: number; +}; + +const openServers: Server[] = []; +const openSockets: Socket[] = []; + +afterEach(async () => { + for (const socket of openSockets.splice(0)) socket.destroy(); + await Promise.all(openServers.splice(0).map((server) => new Promise((resolve) => server.close(() => resolve())))); +}); + +async function startServer(handler?: (req: IncomingMessage, res: ServerResponse) => void): Promise { + const requests: TestServer["requests"] = []; + const state = { connections: 0 }; + const server = createServer((req, res) => { + requests.push({ url: req.url ?? "", host: req.headers.host }); + if (handler) { + handler(req, res); + return; + } + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ ok: true })); + }); + server.on("connection", () => { + state.connections += 1; + }); + openServers.push(server); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const port = (server.address() as AddressInfo).port; + return { + server, + port, + requests, + get connections() { + return state.connections; + }, + }; +} + +function guardError(message: string, code: string) { + return Object.assign(new Error(message), { code }); +} + +/** + * A stand-in for the network: routes whatever address the transport dialled to a + * loopback listener, and reports `remoteAddress` as the dialled address so the + * peer check sees what the kernel would have seen. + */ +function routingSocketFactory(routes: Record): { + factory: RemoteHttpSocketFactory; + dialled: string[]; +} { + const dialled: string[] = []; + const factory: RemoteHttpSocketFactory = (target) => { + dialled.push(target.address); + const port = routes[target.address]; + if (port === undefined) throw new Error(`test network has no route to ${target.address}`); + const socket = netConnect({ host: "127.0.0.1", port }); + openSockets.push(socket); + Object.defineProperty(socket, "remoteAddress", { get: () => target.address, configurable: true }); + return socket; + }; + return { factory, dialled }; +} + +/** A name server that answers public first and loopback afterwards. */ +function rebindingLookup(): { lookup: () => Promise>; calls: () => number } { + let calls = 0; + return { + lookup: async () => { + calls += 1; + return calls === 1 + ? [{ address: PUBLIC_ADDRESS, family: 4 }] + : [{ address: "127.0.0.1", family: 4 }]; + }, + calls: () => calls, + }; +} + +describe("guarded remote HTTP fetch (PAP-17098 DNS rebinding)", () => { + it("pins the connection to the approved address so a rebind never reaches loopback", async () => { + const upstream = await startServer(); + const internal = await startServer(); + const dns = rebindingLookup(); + const network = routingSocketFactory({ + [PUBLIC_ADDRESS]: upstream.port, + "127.0.0.1": internal.port, + }); + + const response = await guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: dns.lookup, + socketFactory: network.factory, + error: guardError, + }); + + await expect(response.json()).resolves.toEqual({ ok: true }); + // One resolution, and the connection went to the address that resolution + // approved. The rebind answer is never consulted, so it cannot win a race. + expect(dns.calls()).toBe(1); + expect(network.dialled).toEqual([PUBLIC_ADDRESS]); + expect(upstream.requests).toHaveLength(1); + expect(internal.connections).toBe(0); + expect(internal.requests).toHaveLength(0); + }); + + it("keeps the original Host header even though it dialled an IP address", async () => { + const upstream = await startServer(); + const dns = rebindingLookup(); + const network = routingSocketFactory({ [PUBLIC_ADDRESS]: upstream.port }); + + await guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, { method: "POST", body: "{}" }, { + allowPrivateNetwork: false, + lookup: dns.lookup, + socketFactory: network.factory, + error: guardError, + }); + + expect(upstream.requests[0]?.host).toBe(REBIND_HOST); + expect(upstream.requests[0]?.url).toBe("/mcp"); + }); + + it("sends the hostname as TLS SNI rather than the pinned address", async () => { + // The handshake is never completed: capturing the ClientHello is enough to + // prove SNI carries the configured hostname, and needs no test certificate. + const clientHello = new Promise((resolve) => { + const server = createServer(); + openServers.push(server); + server.on("connection", (socket) => { + socket.once("data", (chunk: Buffer) => { + resolve(chunk); + socket.destroy(); + }); + }); + server.listen(0, "127.0.0.1", () => { + const port = (server.address() as AddressInfo).port; + const network = routingSocketFactory({ [PUBLIC_ADDRESS]: port }); + void guardedRemoteHttpFetch(`https://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [{ address: PUBLIC_ADDRESS, family: 4 }], + socketFactory: network.factory, + error: guardError, + }).catch(() => {}); + }); + }); + + const hello = await clientHello; + expect(hello.includes(REBIND_HOST)).toBe(true); + expect(hello.includes(PUBLIC_ADDRESS)).toBe(false); + }); + + it("rejects a peer that turns out to be loopback without writing a request", async () => { + const internal = await startServer(); + // A rebind below DNS: resolution approved a public address, but the socket + // lands on loopback anyway. The peer check has to catch it, and has to catch + // it before any request byte is written. + const factory: RemoteHttpSocketFactory = () => { + const socket = netConnect({ host: "127.0.0.1", port: internal.port }); + openSockets.push(socket); + return socket; + }; + + await expect(guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [{ address: PUBLIC_ADDRESS, family: 4 }], + socketFactory: factory, + error: guardError, + })).rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + + expect(internal.requests).toHaveLength(0); + }); + + it("rejects a hostname whose only resolution is link-local metadata", async () => { + const internal = await startServer(); + const network = routingSocketFactory({ "169.254.169.254": internal.port }); + + await expect(guardedRemoteHttpFetch("http://metadata.rebind.test/latest/meta-data/", {}, { + allowPrivateNetwork: false, + lookup: async () => [{ address: "169.254.169.254", family: 4 }], + socketFactory: network.factory, + error: guardError, + })).rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + + expect(network.dialled).toEqual([]); + expect(internal.requests).toHaveLength(0); + }); + + it("rejects a hostname that resolves to a mix of public and private addresses", async () => { + await expect(guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [ + { address: PUBLIC_ADDRESS, family: 4 }, + { address: "10.1.2.3", family: 4 }, + ], + socketFactory: () => { + throw new Error("must not dial"); + }, + error: guardError, + })).rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + }); + + it("does not follow redirects, so the caller re-runs the guard on the next hop", async () => { + const upstream = await startServer((_req, res) => { + res.writeHead(302, { location: "http://127.0.0.1:9/internal" }); + res.end(); + }); + const network = routingSocketFactory({ [PUBLIC_ADDRESS]: upstream.port }); + + const response = await guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [{ address: PUBLIC_ADDRESS, family: 4 }], + socketFactory: network.factory, + error: guardError, + }); + + expect(response.status).toBe(302); + expect(response.headers.get("location")).toBe("http://127.0.0.1:9/internal"); + }); + + it("carries an MCP Streamable HTTP exchange, SSE framing included", async () => { + // The pinned transport replaces `fetch` on the tools/call path, so an MCP + // JSON-RPC reply delivered as a single SSE event has to survive it intact. + const upstream = await startServer((req, res) => { + const chunks: Buffer[] = []; + req.on("data", (chunk: Buffer) => chunks.push(chunk)); + req.on("end", () => { + const request = JSON.parse(Buffer.concat(chunks).toString("utf8")) as { method: string }; + res.writeHead(200, { "content-type": "text/event-stream" }); + res.end(`event: message\ndata: ${JSON.stringify({ jsonrpc: "2.0", id: "1", result: { echo: request.method } })}\n\n`); + }); + }); + const network = routingSocketFactory({ [PUBLIC_ADDRESS]: upstream.port }); + + const response = await guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, { + method: "POST", + headers: { accept: "application/json, text/event-stream", "content-type": "application/json" }, + body: JSON.stringify({ jsonrpc: "2.0", id: "1", method: "tools/call", params: {} }), + }, { + allowPrivateNetwork: false, + lookup: async () => [{ address: PUBLIC_ADDRESS, family: 4 }], + socketFactory: network.factory, + error: guardError, + }); + + expect(response.headers.get("content-type")).toBe("text/event-stream"); + await expect(response.text()).resolves.toContain('"echo":"tools/call"'); + }); + + it("decodes a compressed response body the way fetch would", async () => { + const upstream = await startServer((_req, res) => { + res.writeHead(200, { "content-type": "application/json", "content-encoding": "gzip" }); + res.end(gzipSync(Buffer.from(JSON.stringify({ token_endpoint: "https://auth.example/token" }), "utf8"))); + }); + const network = routingSocketFactory({ [PUBLIC_ADDRESS]: upstream.port }); + + const response = await guardedRemoteHttpFetch(`http://${REBIND_HOST}/.well-known/oauth-authorization-server`, {}, { + allowPrivateNetwork: false, + lookup: async () => [{ address: PUBLIC_ADDRESS, family: 4 }], + socketFactory: network.factory, + error: guardError, + }); + + await expect(response.json()).resolves.toEqual({ token_endpoint: "https://auth.example/token" }); + expect(response.headers.get("content-encoding")).toBeNull(); + }); + + it("still refuses a private IP literal before any transport runs", async () => { + await expect(guardedRemoteHttpFetch("http://169.254.169.254/latest/meta-data/", {}, { + allowPrivateNetwork: false, + error: guardError, + unpinnedFetch: async () => { + throw new Error("must not fetch"); + }, + socketFactory: () => { + throw new Error("must not dial"); + }, + })).rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + }); + + it("uses platform fetch for a public IP literal, where nothing can be re-resolved", async () => { + const calls: string[] = []; + const response = await guardedRemoteHttpFetch("https://93.184.216.34/mcp", { method: "POST" }, { + allowPrivateNetwork: false, + error: guardError, + unpinnedFetch: async (input, init) => { + calls.push(`${init?.method} ${String(input)} ${init?.redirect}`); + return new Response("{}", { status: 200 }); + }, + socketFactory: () => { + throw new Error("must not dial"); + }, + }); + + expect(response.status).toBe(200); + expect(calls).toEqual(["POST https://93.184.216.34/mcp manual"]); + }); + + it("uses platform fetch when the deployment allows private endpoints", async () => { + // Nothing to pin: an operator who is allowed to point at 127.0.0.1 directly + // gains nothing from rebinding, so this mode keeps `fetch` semantics. + const calls: string[] = []; + const response = await guardedRemoteHttpFetch("http://127.0.0.1:9/mcp", {}, { + allowPrivateNetwork: true, + error: guardError, + unpinnedFetch: async (input) => { + calls.push(String(input)); + return new Response("{}", { status: 200 }); + }, + socketFactory: () => { + throw new Error("must not dial"); + }, + }); + + expect(response.status).toBe(200); + expect(calls).toEqual(["http://127.0.0.1:9/mcp"]); + }); +}); diff --git a/server/src/services/remote-http-endpoint-guard.ts b/server/src/services/remote-http-endpoint-guard.ts index 4ed1fdb30e9..e4204d2da58 100644 --- a/server/src/services/remote-http-endpoint-guard.ts +++ b/server/src/services/remote-http-endpoint-guard.ts @@ -39,7 +39,29 @@ export async function assertPublicRemoteHttpEndpoint( options: RemoteHttpEndpointGuardOptions, error: RemoteHttpEndpointErrorFactory, ): Promise { - if (options.allowPrivateNetwork) return; + await resolveApprovedRemoteHttpAddresses(endpoint, options, error); +} + +/** + * Validate a remote endpoint and return the exact address set that was approved. + * + * Callers must dial one of the returned addresses instead of letting the socket + * layer resolve the hostname a second time. A second resolution reopens a + * DNS-rebinding TOCTOU window: an attacker-controlled name server can answer + * with a public address while this guard is looking, then with a loopback, + * private or link-local address a moment later when the connection is made + * (PAP-17098). Returning the resolved set — rather than a bare `void` — is what + * lets `guardedRemoteHttpFetch` close that window. + * + * An empty result means "no address pinning required": the deployment allows + * private endpoints, so there is no boundary left to enforce. + */ +export async function resolveApprovedRemoteHttpAddresses( + endpoint: URL, + options: RemoteHttpEndpointGuardOptions, + error: RemoteHttpEndpointErrorFactory, +): Promise { + if (options.allowPrivateNetwork) return []; const hostname = endpoint.hostname.replace(/^\[|\]$/g, "").toLowerCase(); if (hostname === "localhost" || hostname.endsWith(".localhost")) { @@ -51,7 +73,7 @@ export async function assertPublicRemoteHttpEndpoint( if (isPrivateOrReservedIp(hostname)) { throw error("Remote MCP connection URL cannot target private or reserved network addresses", "remote_http_private_endpoint"); } - return; + return [hostname]; } let results: LookupResult[]; @@ -70,6 +92,22 @@ export async function assertPublicRemoteHttpEndpoint( if (results.some((result) => isPrivateOrReservedIp(result.address))) { throw error("Remote MCP connection URL cannot resolve to private or reserved network addresses", "remote_http_private_endpoint"); } + return results.map((result) => result.address); +} + +/** + * Reduce an address to the form used for comparing an approved address against + * the peer a socket actually connected to. `socket.remoteAddress` may come back + * as an IPv4-mapped IPv6 address or carry an IPv6 zone index, neither of which + * appears in a DNS answer. + */ +export function normalizeIpAddress(address: string): string { + const lower = address.trim().toLowerCase().replace(/^\[|\]$/g, "").split("%")[0] ?? ""; + const mappedIpv4 = lower.match(/^::ffff:(\d{1,3}(?:\.\d{1,3}){3})$/); + if (mappedIpv4?.[1]) return mappedIpv4[1]; + const mappedIpv4Hex = parseMappedIpv4Hex(lower); + if (mappedIpv4Hex) return mappedIpv4Hex; + return lower; } function defaultLookup(hostname: string): Promise { @@ -93,7 +131,7 @@ async function lookupWithTimeout(hostname: string, lookup: RemoteHttpEndpointLoo } } -function isPrivateOrReservedIp(address: string): boolean { +export function isPrivateOrReservedIp(address: string): boolean { const lower = address.toLowerCase(); const mappedIpv4 = lower.match(/^::ffff:(\d{1,3}(?:\.\d{1,3}){3})$/); if (mappedIpv4?.[1]) return isPrivateOrReservedIpv4(mappedIpv4[1]); diff --git a/server/src/services/remote-http-fetch.ts b/server/src/services/remote-http-fetch.ts new file mode 100644 index 00000000000..18883ad8cf6 --- /dev/null +++ b/server/src/services/remote-http-fetch.ts @@ -0,0 +1,303 @@ +import { request as httpRequest, type IncomingMessage } from "node:http"; +import { request as httpsRequest } from "node:https"; +import { connect as netConnect, isIP, type Socket } from "node:net"; +import { Readable } from "node:stream"; +import { connect as tlsConnect, type TLSSocket } from "node:tls"; +import { createBrotliDecompress, createGunzip, createInflate } from "node:zlib"; + +import { + isPrivateOrReservedIp, + normalizeIpAddress, + resolveApprovedRemoteHttpAddresses, + type RemoteHttpEndpointErrorFactory, + type RemoteHttpEndpointGuardOptions, +} from "./remote-http-endpoint-guard.js"; + +/** Statuses whose HTTP semantics forbid a response body. */ +const NULL_BODY_STATUSES = new Set([101, 103, 204, 205, 304]); + +const DEFAULT_CONNECT_TIMEOUT_MS = 10_000; + +/** How a verified socket is opened. Overridable so tests can simulate a rebind. */ +export type RemoteHttpSocketFactory = (target: { + address: string; + port: number; + hostname: string; + useTls: boolean; +}) => Socket; + +export type GuardedRemoteHttpFetchOptions = RemoteHttpEndpointGuardOptions & { + /** Builds the rejection thrown when the guard or the peer check fails. */ + error: RemoteHttpEndpointErrorFactory; + /** + * Test seam: opens the raw TCP socket. Production leaves this unset so the + * socket is dialled at the approved address. A factory that connects + * somewhere else stands in for a rebind below the DNS layer, and must still + * be caught by the peer-address check. + */ + socketFactory?: RemoteHttpSocketFactory; + connectTimeoutMs?: number; + /** + * Platform `fetch`, used only when the deployment allows private endpoints and + * there is therefore no egress boundary to pin against. + */ + unpinnedFetch?: typeof fetch; +}; + +/** + * Fetch a remote endpoint without reopening the DNS-rebinding window the guard + * just closed (PAP-17098). + * + * `assertPublicRemoteHttpEndpoint` used to hand its verdict to a bare global + * `fetch`, which resolved the hostname a second time. An attacker who controls + * the name server for the hostname could answer with a public address for the + * guard's lookup and a loopback, RFC 1918 or link-local address for the + * connection, reaching internal services and cloud metadata from the Paperclip + * server. This function instead: + * + * 1. resolves and validates the hostname exactly once, keeping the approved + * address set; + * 2. dials one of those approved addresses directly, so the socket layer never + * performs its own lookup; + * 3. preserves the original hostname for the `Host` header, for TLS SNI and for + * certificate identity checking, so pinning is invisible to the peer; + * 4. re-checks the address the socket actually connected to before a single + * request byte is written, which also covers a rebind below DNS; and + * 5. never follows redirects — it behaves as `redirect: "manual"` so the caller + * re-runs the whole guard against every `Location` it decides to follow. + * + * Two cases need no pinning and keep platform `fetch` semantics: + * + * - the deployment allows private endpoints, where the guard is a documented + * no-op and rebinding cannot reach anything an operator could not reach by + * typing the private URL in directly; and + * - the URL already carries an IP literal, where no name resolution happens on + * either side of the guard, so there is no second answer to disagree with the + * first. `URL` has already normalised the literal (`0x7f.1`, `::ffff:7f00:1`) + * by the time the guard classifies it. + */ +export async function guardedRemoteHttpFetch( + url: string | URL, + init: RequestInit, + options: GuardedRemoteHttpFetchOptions, +): Promise { + const endpoint = url instanceof URL ? url : new URL(url); + const approved = await resolveApprovedRemoteHttpAddresses(endpoint, options, options.error); + const literalHost = isIP(endpoint.hostname.replace(/^\[|\]$/g, "")) !== 0; + const platformFetch = options.unpinnedFetch ?? fetch; + if (approved.length === 0 || literalHost) { + return platformFetch(endpoint.toString(), { ...init, redirect: "manual" }); + } + return pinnedRequest(endpoint, approved, init, options); +} + +async function pinnedRequest( + endpoint: URL, + approved: string[], + init: RequestInit, + options: GuardedRemoteHttpFetchOptions, +): Promise { + const useTls = endpoint.protocol === "https:"; + const hostname = endpoint.hostname.replace(/^\[|\]$/g, ""); + const port = endpoint.port ? Number(endpoint.port) : useTls ? 443 : 80; + const approvedSet = new Set(approved.map(normalizeIpAddress)); + const signal = init.signal ?? null; + + signal?.throwIfAborted?.(); + + const socket = await openVerifiedSocket({ + address: approved[0]!, + approvedSet, + port, + hostname, + useTls, + signal, + options, + }); + + try { + return await sendRequest({ endpoint, hostname, port, useTls, socket, init, signal }); + } catch (error) { + socket.destroy(); + throw error; + } +} + +/** + * Connect to `address`, confirm the peer really is that approved address, and + * only then complete the TLS handshake. Verifying before the handshake keeps + * even a ClientHello off an internal service. + */ +async function openVerifiedSocket(input: { + address: string; + approvedSet: Set; + port: number; + hostname: string; + useTls: boolean; + signal: AbortSignal | null; + options: GuardedRemoteHttpFetchOptions; +}): Promise { + const { address, approvedSet, port, hostname, useTls, signal, options } = input; + const connectTimeoutMs = options.connectTimeoutMs ?? DEFAULT_CONNECT_TIMEOUT_MS; + const factory = options.socketFactory + ?? ((target) => netConnect({ host: target.address, port: target.port })); + + const raw = factory({ address, port, hostname, useTls }); + + await once(raw, "connect", { signal, timeoutMs: connectTimeoutMs, what: "connect to" }); + + const peer = raw.remoteAddress ? normalizeIpAddress(raw.remoteAddress) : null; + if (!peer || isPrivateOrReservedIp(peer) || !approvedSet.has(peer)) { + raw.destroy(); + throw options.error( + "Remote MCP connection resolved to an address that was not approved", + "remote_http_private_endpoint", + ); + } + + if (!useTls) return raw; + + const secure = tlsConnect({ + socket: raw, + // The certificate is checked against the hostname the operator configured, + // not the pinned address, and SNI carries that hostname too. + servername: isIP(hostname) === 0 ? hostname : undefined, + host: hostname, + }); + try { + await once(secure, "secureConnect", { signal, timeoutMs: connectTimeoutMs, what: "negotiate TLS with" }); + } catch (error) { + secure.destroy(); + raw.destroy(); + throw error; + } + return secure; +} + +async function sendRequest(input: { + endpoint: URL; + hostname: string; + port: number; + useTls: boolean; + socket: Socket; + init: RequestInit; + signal: AbortSignal | null; +}): Promise { + const { endpoint, hostname, port, useTls, socket, init, signal } = input; + const headers = new Headers(init.headers); + const body = readRequestBody(init.body); + const method = (init.method ?? "GET").toUpperCase(); + + if (body !== undefined && !headers.has("content-length") && !headers.has("transfer-encoding")) { + headers.set("content-length", String(body.byteLength)); + } + if (init.body instanceof URLSearchParams && !headers.has("content-type")) { + headers.set("content-type", "application/x-www-form-urlencoded;charset=UTF-8"); + } + // `URL.host` already drops the port when it is the protocol default, so the + // peer sees the same `Host` a plain `fetch` would have sent. + headers.set("host", endpoint.host); + + const requestFn = useTls ? httpsRequest : httpRequest; + const message = await new Promise((resolve, reject) => { + const req = requestFn({ + method, + // `createConnection` returns the socket that was already verified, so no + // hostname reaches the socket layer and no second lookup can happen. It is + // only honoured while `agent` stays unset, so do not pass one. + createConnection: () => socket, + host: hostname, + port, + path: `${endpoint.pathname}${endpoint.search}`, + headers: Object.fromEntries(headers.entries()), + setHost: false, + ...(signal ? { signal } : {}), + }, resolve); + req.on("error", reject); + if (body !== undefined) req.write(body); + req.end(); + }); + + const responseHeaders = new Headers(); + for (const [key, value] of Object.entries(message.headers)) { + if (Array.isArray(value)) { + for (const entry of value) responseHeaders.append(key, entry); + } else if (value !== undefined) { + responseHeaders.append(key, value); + } + } + + const status = message.statusCode ?? 502; + const nullBody = NULL_BODY_STATUSES.has(status) || method === "HEAD"; + if (nullBody) message.resume(); + + return new Response( + nullBody ? null : (Readable.toWeb(decodedBody(message, responseHeaders)) as unknown as ReadableStream), + { status, statusText: message.statusMessage ?? "", headers: responseHeaders }, + ); +} + +/** + * Match `fetch`'s content decoding. `node:http` hands back the raw bytes, so a + * server that compresses without being asked would otherwise turn into a JSON + * parse failure that looks like a broken MCP server. + */ +function decodedBody(message: IncomingMessage, headers: Headers): Readable { + const encoding = (message.headers["content-encoding"] ?? "").trim().toLowerCase(); + const decoder = encoding === "gzip" || encoding === "x-gzip" + ? createGunzip() + : encoding === "deflate" + ? createInflate() + : encoding === "br" + ? createBrotliDecompress() + : null; + if (!decoder) return message; + headers.delete("content-encoding"); + headers.delete("content-length"); + message.on("error", (error) => decoder.destroy(error)); + return message.pipe(decoder); +} + +function readRequestBody(body: RequestInit["body"]): Buffer | undefined { + if (body === undefined || body === null) return undefined; + if (typeof body === "string") return Buffer.from(body, "utf8"); + if (body instanceof URLSearchParams) return Buffer.from(body.toString(), "utf8"); + if (Buffer.isBuffer(body)) return body; + if (body instanceof ArrayBuffer) return Buffer.from(body); + if (ArrayBuffer.isView(body)) return Buffer.from(body.buffer, body.byteOffset, body.byteLength); + throw new TypeError("Guarded remote HTTP requests only support string, URLSearchParams and buffer bodies"); +} + +function once( + emitter: Socket, + event: string, + input: { signal: AbortSignal | null; timeoutMs: number; what: string }, +): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + settle(new Error(`Timed out trying to ${input.what} the remote MCP endpoint`)); + }, input.timeoutMs); + timer.unref?.(); + + const onAbort = () => settle(input.signal?.reason ?? new Error("Remote MCP request was aborted")); + + function settle(error?: Error) { + clearTimeout(timer); + emitter.off(event, onSuccess); + emitter.off("error", onError); + input.signal?.removeEventListener?.("abort", onAbort); + if (error) reject(error); + else resolve(); + } + function onSuccess() { + settle(); + } + function onError(error: Error) { + settle(error); + } + + emitter.once(event, onSuccess); + emitter.once("error", onError); + input.signal?.addEventListener?.("abort", onAbort, { once: true }); + }); +} diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index d3faee6cda3..95f94222399 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -120,6 +120,7 @@ import { badRequest, conflict, forbidden, HttpError, notFound, unprocessable } f import { logActivity } from "./activity-log.js"; import { mcpHttpRequestHeaders, parseMcpHttpResponseBody } from "./mcp-http.js"; import { assertPublicRemoteHttpEndpoint, parseRemoteHttpEndpoint } from "./remote-http-endpoint-guard.js"; +import { guardedRemoteHttpFetch, type GuardedRemoteHttpFetchOptions } from "./remote-http-fetch.js"; import { secretService } from "./secrets.js"; import { toolAccessPolicyService } from "./tool-access-policy.js"; import { readSignedToolArgumentsPayload } from "./tool-content-guards.js"; @@ -1608,12 +1609,29 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} return endpoint.toString(); } + function remoteHttpFetchOptions(): GuardedRemoteHttpFetchOptions { + return { + allowPrivateNetwork: allowPrivateRemoteEndpoints(), + error: (message, code) => badRequest(message, { code }), + }; + } + + /** + * Fetch an operator-supplied remote URL with the egress guard bound to the + * connection itself. + * + * `guardedRemoteHttpFetch` resolves the hostname once and dials the approved + * address, so a name server that answers public-then-private cannot move the + * connection onto a loopback or metadata address after validation + * (PAP-17098). Redirects stay manual and run the full guard again on the next + * hop, because a `Location` is just as attacker-controlled as the first URL. + */ async function fetchRemoteHttpUrl(value: string, init: RequestInit = {}): Promise { let currentUrl = value; const method = (init.method ?? "GET").toUpperCase(); for (let redirectCount = 0; redirectCount <= MAX_REMOTE_HTTP_REDIRECTS; redirectCount += 1) { - const safeUrl = await assertRemoteHttpUrlAllowed(currentUrl); - const response = await fetch(safeUrl, { ...init, redirect: "manual" }); + const endpoint = parseRemoteHttpEndpoint(currentUrl, (message, code) => badRequest(message, { code })); + const response = await guardedRemoteHttpFetch(endpoint, init, remoteHttpFetchOptions()); const location = REMOTE_HTTP_REDIRECT_STATUSES.has(response.status) ? response.headers?.get?.("location") ?? null : null; @@ -1624,7 +1642,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} if (redirectCount >= MAX_REMOTE_HTTP_REDIRECTS) { throw new HttpError(502, "Remote OAuth endpoint redirected too many times", { code: "oauth_redirect_limit" }); } - currentUrl = new URL(location, safeUrl).toString(); + currentUrl = new URL(location, endpoint).toString(); } throw new HttpError(502, "Remote OAuth endpoint redirected too many times", { code: "oauth_redirect_limit" }); } @@ -3082,8 +3100,10 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} async function remoteTools(connection: typeof toolConnections.$inferSelect): Promise { const headers = { ...projectedConnectionHeaders(connection), ...await resolveCredentialHeaders(connection) }; - const endpoint = await assertRemoteEndpointAllowed(connection.config); - const response = await fetch(endpoint, { + // Pinned to the address the guard approved: `config.url` is operator-supplied, + // so a second DNS resolution here would reopen the rebinding window that + // PAP-17098 closed for the OAuth endpoints. + const response = await guardedRemoteHttpFetch(remoteEndpoint(connection.config), { method: "POST", // MCP Streamable HTTP requires advertising that we accept both a JSON body // and an SSE stream; spec-compliant servers 406 without it (see mcp-http.ts). @@ -3094,7 +3114,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} method: "tools/list", params: {}, }), - }); + }, remoteHttpFetchOptions()); if (!response.ok) { const authenticate = response.headers.get("www-authenticate") ?? ""; if (response.status === 401 && /bearer|oauth|authorization/i.test(authenticate)) { diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index f40776735e2..cd6577a4569 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -55,7 +55,8 @@ import { logActivity, type LogActivityInput } from "./activity-log.js"; import { secretService } from "./secrets.js"; import { mcpHttpRequestHeaders, parseMcpHttpResponseBody } from "./mcp-http.js"; import { projectedConnectionHeaders } from "./tool-access.js"; -import { assertPublicRemoteHttpEndpoint, parseRemoteHttpEndpoint } from "./remote-http-endpoint-guard.js"; +import { parseRemoteHttpEndpoint } from "./remote-http-endpoint-guard.js"; +import { guardedRemoteHttpFetch, type GuardedRemoteHttpFetchOptions } from "./remote-http-fetch.js"; import { toolAccessPolicyService } from "./tool-access-policy.js"; import { issueThreadInteractionService } from "./issue-thread-interactions.js"; import { @@ -2140,14 +2141,19 @@ export function createToolGatewayService( return options.deploymentMode !== "authenticated" || options.deploymentExposure !== "public"; } - async function assertRemoteEndpointAllowed(config: Record): Promise { - const endpoint = new URL(remoteEndpoint(config)); - await assertPublicRemoteHttpEndpoint( - endpoint, - { allowPrivateNetwork: allowPrivateRemoteEndpoints() }, - (message, code) => new ToolGatewayHttpError(422, message, code), - ); - return endpoint.toString(); + /** + * Guard options for every outbound call to an operator-supplied MCP endpoint. + * + * The private-network check no longer runs as a standalone pre-flight: it is + * part of `guardedRemoteHttpFetch`, which keeps the approved address and dials + * it directly. Splitting validation from dispatch is what created the + * DNS-rebinding TOCTOU in PAP-17098. + */ + function remoteHttpFetchOptions(): GuardedRemoteHttpFetchOptions { + return { + allowPrivateNetwork: allowPrivateRemoteEndpoints(), + error: (message, code) => new ToolGatewayHttpError(422, message, code), + }; } function headerName(value: unknown): string | null { @@ -3050,7 +3056,7 @@ export function createToolGatewayService( callerHeaders?: ExecuteGatewayToolInput["callerHeaders"], ): Promise { const { entry, connection } = await resolveConnectedRemoteTool(session, tool); - const endpoint = await assertRemoteEndpointAllowed(connection.config ?? {}); + const endpoint = remoteEndpoint(connection.config ?? {}); // Method-defined headers are trusted catalog configuration. Treat them as // managed headers so callers cannot override the scope that was reviewed // during tools/list. Credentials remain authoritative on collisions. @@ -3081,7 +3087,11 @@ export function createToolGatewayService( const timer = setTimeout(() => controller.abort(), ms); timer.unref?.(); try { - const response = await fetch(endpoint, { + // The guard runs inside this call and the connection is pinned to the + // address it approved, so an operator-supplied hostname cannot be rebound + // onto a loopback or metadata address between validation and dispatch + // (PAP-17098). + const response = await guardedRemoteHttpFetch(endpoint, { method: "POST", redirect: "manual", // MCP Streamable HTTP requires the Accept header advertising both a JSON @@ -3097,7 +3107,7 @@ export function createToolGatewayService( arguments: parameters ?? {}, }, }), - }); + }, remoteHttpFetchOptions()); const body = await readBoundedRemoteResponse(response); execution.response = { httpStatus: response.status,