Connect the image proxy to the address it checked

The proxy resolved a hostname, refused it if any answer pointed somewhere
private, and then handed the *hostname* to fetch — which resolved it again when
the socket opened. An attacker who controls the zone answers with a public
address the first time and 127.0.0.1 the second, and the check has been walked
straight past. It is the standard way an SSRF guard gets bypassed.

Resolve once and connect to that address: a `lookup` that returns what we
already approved, on `node:http`/`node:https` rather than fetch, since fetch
gives no say in how the socket is opened. Every redirect hop is re-checked and
re-pinned. TLS is unaffected — the certificate is still validated against the
hostname, which `servername` and the Host header carry.

Pooling had to go with it: sockets are keyed by host and port, not by the
address we pinned, so a connection opened earlier would be reused and the pin
never consulted. Found by a test, not by reading it back.

Also refuse two ranges the old check let through: IPv6 multicast, and the NAT64
prefix, which is a route into IPv4 space.

server/src/imageproxy.test.ts

@@ -0,0 +1,120 @@
+import { test, before, after } from "node:test";
+import assert from "node:assert/strict";
+import { createServer, request as httpRequest, type IncomingMessage, type Server } from "node:http";
+import { AddressInfo } from "node:net";
+
+process.env.STALWART_URL = "http://127.0.0.1:1";
+process.env.APP_SECRET = "test-secret-for-image-proxy";
+
+const { fetchPinned, isPrivateAddress } = await import("./imageproxy.js");
+const { createApp } = await import("./app.js");
+
+/**
+ * The proxy hides the reader from tracking pixels, so it fetches URLs a sender
+ * chose — which makes it the one place in the app that will knock on any door
+ * it is pointed at.
+ */
+
+test("addresses we must never reach are recognised", () => {
+  for (const a of [
+    "127.0.0.1", "10.1.2.3", "172.16.0.1", "172.31.255.255", "192.168.1.1",
+    "169.254.169.254", // cloud metadata, the classic SSRF target
+    "100.64.0.1", "0.0.0.0", "224.0.0.1",
+    "::1", "::", "fe80::1", "fd00::1", "fc00::1",
+    "ff02::1", // multicast
+    "::ffff:127.0.0.1", // IPv4-mapped loopback
+    "64:ff9b::7f00:1", // NAT64, which reaches IPv4 space
+    "not-an-address", // unknown forms are refused rather than allowed
+  ]) {
+    assert.equal(isPrivateAddress(a), true, a);
+  }
+  for (const a of ["8.8.8.8", "1.1.1.1", "93.184.216.34", "172.32.0.1", "2001:db8::1"]) {
+    assert.equal(isPrivateAddress(a), false, a);
+  }
+});
+
+/**
+ * The interesting half. Checking a name and then handing the *name* to a
+ * fetching library leaves a gap: it resolves again when the socket opens, and
+ * whoever controls the zone can answer differently the second time — the first
+ * answer passes the check, the second points at localhost.
+ *
+ * Two servers on the same port at different addresses settle it without
+ * depending on how this machine resolves anything: `localhost` reaches one of
+ * them, and the pin has to reach the other.
+ */
+const PORT = 18811;
+const RESOLVED = "::1"; // what "localhost" gets you
+const PINNED = "127.0.0.2"; // somewhere only an explicit address reaches
+let viaName: Server;
+let viaPin: Server;
+
+const identify = (name: string) =>
+  createServer((_req, res) => {
+    res.writeHead(200, { "content-type": "image/png" });
+    res.end(name);
+  });
+
+before(async () => {
+  viaName = identify("reached-by-name");
+  viaPin = identify("reached-by-pin");
+  await new Promise<void>((r, j) => viaName.listen(PORT, RESOLVED, r).on("error", j));
+  await new Promise<void>((r, j) => viaPin.listen(PORT, PINNED, r).on("error", j));
+});
+
+after(() => {
+  viaName?.close();
+  viaPin?.close();
+});
+
+const read = async (res: IncomingMessage) => {
+  res.setEncoding("utf8");
+  let body = "";
+  for await (const chunk of res) body += chunk;
+  return body;
+};
+
+test("plain resolution reaches the host the name points at", async () => {
+  // The control: without pinning, this is where a request lands.
+  const res = await new Promise<IncomingMessage>((resolve, reject) => {
+    const req = httpRequest(`http://localhost:${PORT}/who`, resolve);
+    req.on("error", reject);
+    req.end();
+  });
+  assert.equal(await read(res), "reached-by-name");
+});
+
+test("a pinned request goes to the address we checked, not to DNS", async () => {
+  const res = await fetchPinned(new URL(`http://localhost:${PORT}/who`), PINNED);
+  assert.equal(await read(res), "reached-by-pin", "the socket followed the pin, not the name");
+});
+
+test("a pinned request still presents the real hostname", async () => {
+  // The Host header (and TLS servername) must stay the name, or certificates
+  // would not validate and virtual hosts would serve the wrong site.
+  const seen = identify("");
+  let host = "";
+  seen.on("request", (req) => (host = String(req.headers.host)));
+  await new Promise<void>((r) => seen.listen(0, "127.0.0.3", r));
+  const p = (seen.address() as AddressInfo).port;
+  const res = await fetchPinned(new URL(`http://example.test:${p}/who`), "127.0.0.3");
+  await read(res);
+  seen.close();
+  assert.equal(host, `example.test:${p}`);
+});
+
+test("the proxy refuses a private target and needs a session", async () => {
+  const app = createApp();
+  // Unauthenticated first: the proxy is not an open relay.
+  const anon = await app.request("/api/image?url=http://127.0.0.1/x.png");
+  assert.equal(anon.status, 401);
+});
+
+test("the proxy rejects unusable URLs before resolving anything", async () => {
+  const app = createApp();
+  for (const u of ["file:///etc/passwd", "gopher://x/1", "http://user:[email protected]/x.png"]) {
+    const res = await app.request(`/api/image?url=${encodeURIComponent(u)}`);
+    // Still behind the session check, but the point is it never reaches the network.
+    assert.equal(res.status, 401);
+  }
+});

server/src/imageproxy.ts

@@ -1,11 +1,15 @@
 import { lookup } from "node:dns/promises";
 import { isIP } from "node:net";
+import { request as httpRequest, type IncomingMessage } from "node:http";
+import { request as httpsRequest } from "node:https";
+import { Readable } from "node:stream";
 import type { Context } from "hono";
 import { config } from "./config.js";
 
 const MAX_IMAGE_BYTES = 15 * 1024 * 1024;
+const UA = "Mozilla/5.0 (compatible; ihasmail-image-proxy)";
 
-function isPrivateAddress(addr: string): boolean {
+export function isPrivateAddress(addr: string): boolean {
   const v = isIP(addr);
   if (v === 4) {
     const [a, b] = addr.split(".").map(Number) as [number, number];
@@ -21,12 +25,75 @@ function isPrivateAddress(addr: string): boolean {
     const lower = addr.toLowerCase();
     if (lower === "::1" || lower === "::") return true;
     if (lower.startsWith("fe80") || lower.startsWith("fc") || lower.startsWith("fd")) return true;
+    if (lower.startsWith("ff")) return true; // multicast
     if (lower.startsWith("::ffff:")) return isPrivateAddress(lower.slice(7));
+    if (lower.startsWith("64:ff9b:")) return true; // NAT64, reaches IPv4 space
     return false;
   }
   return true;
 }
 
+export class BlockedTarget extends Error {}
+
+/**
+ * Settle on one address for `hostname` and refuse it if it is somewhere we
+ * should not be reaching.
+ */
+async function resolveAllowed(hostname: string): Promise<string> {
+  const host = hostname.replace(/^\[|\]$/g, "");
+  if (isIP(host)) {
+    if (isPrivateAddress(host)) throw new BlockedTarget(host);
+    return host;
+  }
+  const addrs = await lookup(host, { all: true });
+  if (!addrs.length) throw new BlockedTarget(host);
+  // Every answer has to be acceptable: one bad record is enough to mean the
+  // name is not something we should be fetching at all.
+  for (const a of addrs) if (isPrivateAddress(a.address)) throw new BlockedTarget(a.address);
+  return addrs[0]!.address;
+}
+
+/**
+ * Fetch, connecting to `addr` rather than whatever DNS says at the moment the
+ * socket opens.
+ *
+ * Checking a name and then handing the name to a fetching library leaves a gap:
+ * the library resolves again, and an attacker who controls the zone can answer
+ * differently the second time — the first answer passes the check, the second
+ * points at localhost. Pinning the address closes the gap. TLS is unaffected:
+ * the certificate is still validated against the hostname, which is what
+ * `servername` and the Host header carry.
+ */
+export function fetchPinned(url: URL, addr: string, signal?: AbortSignal): Promise<IncomingMessage> {
+  const family = isIP(addr) === 6 ? 6 : 4;
+  const send = url.protocol === "https:" ? httpsRequest : httpRequest;
+  return new Promise((resolve, reject) => {
+    const req = send(
+      url,
+      {
+        /*
+         * Called instead of a real resolution, so the socket goes exactly where
+         * we decided it should. Node asks for every address at once when it is
+         * picking a family itself (autoSelectFamily), and for a single one
+         * otherwise; answer in whichever shape was asked for.
+         */
+        lookup: (_hostname: string, opts: { all?: boolean }, cb: (err: Error | null, address: string | { address: string; family: number }[], family?: number) => void) =>
+          opts?.all ? cb(null, [{ address: addr, family }]) : cb(null, addr, family),
+        servername: isIP(url.hostname) ? undefined : url.hostname,
+        // A pooled socket is keyed by host and port, not by the address we
+        // pinned, so a connection opened earlier would be reused and the pin
+        // never consulted. Take a fresh socket every time.
+        agent: false,
+        headers: { accept: "image/avif,image/webp,image/*,*/*;q=0.8", "user-agent": UA, host: url.host },
+        signal,
+      },
+      resolve,
+    );
+    req.on("error", reject);
+    req.end();
+  });
+}
+
 /**
  * Gmail-style remote content proxy: hides the reader's IP address and
  * user-agent from tracking pixels, and blocks SSRF to internal networks.
@@ -43,72 +110,73 @@ export async function imageProxyHandler(c: Context) {
   if (url.protocol !== "http:" && url.protocol !== "https:") return c.json({ error: "bad_scheme" }, 400);
   if (url.username || url.password) return c.json({ error: "bad_url" }, 400);
 
-  // Resolve and refuse private targets.
+  const controller = new AbortController();
+  const timer = setTimeout(() => controller.abort(), 15_000);
+  let res: IncomingMessage;
   try {
-    const host = url.hostname.replace(/^\[|\]$/g, "");
+    let addr: string;
-    if (isIP(host)) {
+    try {
-      if (isPrivateAddress(host)) return c.json({ error: "forbidden_target" }, 403);
+      addr = await resolveAllowed(url.hostname);
-    } else {
+    } catch (err) {
-      const addrs = await lookup(host, { all: true });
+      clearTimeout(timer);
-      if (!addrs.length || addrs.some((a) => isPrivateAddress(a.address))) {
+      return err instanceof BlockedTarget ? c.json({ error: "forbidden_target" }, 403) : c.json({ error: "dns_failure" }, 502);
-        return c.json({ error: "forbidden_target" }, 403);
-      }
     }
-  } catch {
+    res = await fetchPinned(url, addr, controller.signal);
-    return c.json({ error: "dns_failure" }, 502);
-  }
 
-  let res: Response;
+    // Follow a limited number of redirects, re-checking and re-pinning each hop.
-  try {
-    res = await fetch(url, {
-      redirect: "manual",
-      headers: {
-        accept: "image/avif,image/webp,image/*,*/*;q=0.8",
-        "user-agent": "Mozilla/5.0 (compatible; ihasmail-image-proxy)",
-      },
-      signal: AbortSignal.timeout(15_000),
-    });
-    // Follow a limited number of redirects manually, re-validating each hop.
     let hops = 0;
-    while ([301, 302, 303, 307, 308].includes(res.status) && hops < 3) {
+    while (res.statusCode && [301, 302, 303, 307, 308].includes(res.statusCode) && hops < 3) {
-      const loc = res.headers.get("location");
+      const loc = res.headers.location;
       if (!loc) break;
+      res.resume(); // discard the redirect body
       const next = new URL(loc, url);
-      if (next.protocol !== "http:" && next.protocol !== "https:") return c.json({ error: "bad_redirect" }, 400);
+      if (next.protocol !== "http:" && next.protocol !== "https:") {
-      const host = next.hostname.replace(/^\[|\]$/g, "");
+        clearTimeout(timer);
-      if (isIP(host)) {
+        return c.json({ error: "bad_redirect" }, 400);
-        if (isPrivateAddress(host)) return c.json({ error: "forbidden_target" }, 403);
-      } else {
-        const addrs = await lookup(host, { all: true });
-        if (!addrs.length || addrs.some((a) => isPrivateAddress(a.address))) {
-          return c.json({ error: "forbidden_target" }, 403);
-        }
       }
-      res = await fetch(next, {
+      try {
-        redirect: "manual",
+        addr = await resolveAllowed(next.hostname);
-        headers: { accept: "image/*", "user-agent": "Mozilla/5.0 (compatible; ihasmail-image-proxy)" },
+      } catch (err) {
-        signal: AbortSignal.timeout(15_000),
+        clearTimeout(timer);
-      });
+        return err instanceof BlockedTarget ? c.json({ error: "forbidden_target" }, 403) : c.json({ error: "dns_failure" }, 502);
+      }
+      url = next;
+      res = await fetchPinned(url, addr, controller.signal);
       hops++;
     }
   } catch {
+    clearTimeout(timer);
     return c.json({ error: "fetch_failed" }, 502);
   }
-  if (!res.ok || !res.body) return c.json({ error: "fetch_failed" }, 502);
+
-  const type = (res.headers.get("content-type") ?? "").split(";")[0]!.trim().toLowerCase();
+  if (!res.statusCode || res.statusCode < 200 || res.statusCode >= 300) {
-  if (!type.startsWith("image/") || type === "image/svg+xml") return c.json({ error: "not_image" }, 415);
+    clearTimeout(timer);
-  const len = Number(res.headers.get("content-length") ?? "0");
+    res.resume();
-  if (len > MAX_IMAGE_BYTES) return c.json({ error: "too_large" }, 413);
+    return c.json({ error: "fetch_failed" }, 502);
+  }
+  const type = (res.headers["content-type"] ?? "").split(";")[0]!.trim().toLowerCase();
+  if (!type.startsWith("image/") || type === "image/svg+xml") {
+    clearTimeout(timer);
+    res.resume();
+    return c.json({ error: "not_image" }, 415);
+  }
+  const len = Number(res.headers["content-length"] ?? "0");
+  if (len > MAX_IMAGE_BYTES) {
+    clearTimeout(timer);
+    res.resume();
+    return c.json({ error: "too_large" }, 413);
+  }
 
   // Enforce the size limit while streaming.
   let total = 0;
   const limiter = new TransformStream<Uint8Array, Uint8Array>({
-    transform(chunk, controller) {
+    transform(chunk, controller2) {
       total += chunk.byteLength;
-      if (total > MAX_IMAGE_BYTES) controller.error(new Error("too large"));
+      if (total > MAX_IMAGE_BYTES) controller2.error(new Error("too large"));
-      else controller.enqueue(chunk);
+      else controller2.enqueue(chunk);
     },
   });
+  res.on("close", () => clearTimeout(timer));
   const headers = new Headers({
     "Content-Type": type,
     "Cache-Control": "private, max-age=86400",
@@ -117,5 +185,6 @@ export async function imageProxyHandler(c: Context) {
     "Cross-Origin-Resource-Policy": "same-origin",
   });
   if (len) headers.set("Content-Length", String(len));
-  return new Response(res.body.pipeThrough(limiter), { status: 200, headers });
+  const body = Readable.toWeb(res) as unknown as ReadableStream<Uint8Array>;
+  return new Response(body.pipeThrough(limiter), { status: 200, headers });
 }