Harden four things the audit turned up

**The login rate limiter could be sidestepped.** X-Forwarded-For is a list each hop appends to, and nginx's $proxy_add_x_forwarded_for appends ours — so a client sending "X-Forwarded-For: 1.2.3.4" arrives as "1.2.3.4, <their real address>". Reading the leftmost entry, as we did, handed the caller a rate-limit key they could change per request: unlimited password guessing against a deployment that looks correctly configured. Read from the right instead, skip hops that are themselves trusted proxies, and believe the header only when the peer is one (loopback and the private ranges by default, TRUSTED_PROXIES to be explicit). **The upload cap was a suggestion.** It read content-length, which a chunked request simply omits. Count the bytes through a stream, as the image proxy already does. **App password secrets were drawn with a modulo.** 256 is not a multiple of 33, so the first 25 characters of the alphabet came up on 8 byte values and the last 8 on only 7. Rejection sampling instead. The test weighs the whole tail of the alphabet rather than single characters, because a 7/8 skew is invisible per character against the noise — and it does fail when the bias is put back. **Upstream headers were relayed wholesale.** Anything the mail server set — cookies, auth challenges, CORS grants — landed on our origin, where it means something else. Allowlist what is actually wanted.
2026-08-24 11:10:15 -07:00
parent f29a504ead
commit c5f2e2c7f2
8 changed files with 288 additions and 19 deletions
@@ -15,6 +15,12 @@ PORT=8080
 # Set to "1" when running behind a TLS-terminating reverse proxy (trusts
 # X-Forwarded-* and marks cookies Secure). Set to "0" for plain-HTTP dev.
 TRUST_PROXY=1
+# Peers whose X-Forwarded-* headers are believed. Unset means loopback and the
+# private ranges, which covers a reverse proxy on the same host or Docker
+# network. A request from anywhere else is attributed to its socket address,
+# whatever the headers claim -- otherwise anyone could pick their own key for
+# the login rate limiter.
+# TRUSTED_PROXIES=10.0.0.0/8,192.168.1.5
 SECURE_COOKIES=auto

 # Session lifetime (idle timeout) in seconds. "Remember me" extends to SESSION_REMEMBER_TTL.
@@ -141,7 +141,8 @@ All configuration is via environment variables (see `.env.example`):
 | `STALWART_URL` | `https://mail.example.com` | Base URL of Stalwart; the JMAP session is discovered at `/.well-known/jmap` |
 | `APP_SECRET` | *(required in production)* | Secret used to derive session encryption keys |
 | `PORT` / `HOST` | `8080` / `0.0.0.0` | Listen address |
-| `TRUST_PROXY` | `1` | Honour `X-Forwarded-*` from a reverse proxy |
+| `TRUST_PROXY` | `1` | Honour `X-Forwarded-*`, but only from a peer listed in `TRUSTED_PROXIES` |
+| `TRUSTED_PROXIES` | *(loopback + private ranges)* | Comma-separated CIDRs or addresses whose forwarding headers are believed. Anything else is attributed by its socket address, whatever it claims |
 | `SECURE_COOKIES` | `auto` | `auto` (Secure on https), `1`, or `0` for plain-HTTP dev |
 | `SESSION_TTL` / `SESSION_REMEMBER_TTL` | `43200` / `2592000` | Idle session lifetime (seconds), with/without "keep me signed in" |
 | `SESSION_FILE` | *(unset)* | Persist sessions across restarts (ciphertext only) |
@@ -371,16 +371,25 @@ async function assertCurrentPassword(ctx: Ctx, current: string, otpCode?: string
  if (!res.ok) throw new UpstreamError(`Could not verify the current password (${res.status})`, 502);
 }

-/** A legacy app password a person can read off a screen and type. */
-function readableSecret(): string {
+/**
+ * A legacy app password a person can read off a screen and type.
+ *
+ * Drawn by rejection sampling. Plain `% alphabet.length` would favour the
+ * first 25 characters, because 256 is not a multiple of 33: each of those
+ * would come up on 8 byte values and the remaining 8 on only 7.
+ */
+export function readableSecret(): string {
  const alphabet = "abcdefghijkmnopqrstuvwxyz23456789"; // no l/1/0 lookalikes
-  const bytes = randomBytes(20);
-  let out = "";
-  for (let i = 0; i < 20; i++) {
-    if (i > 0 && i % 5 === 0) out += "-";
-    out += alphabet[bytes[i]! % alphabet.length];
+  const limit = 256 - (256 % alphabet.length);
+  const chars: string[] = [];
+  while (chars.length < 20) {
+    for (const b of randomBytes(32)) {
+      if (b >= limit) continue; // the tail that would skew the alphabet
+      chars.push(alphabet[b % alphabet.length]!);
+      if (chars.length === 20) break;
    }
-  return out;
+  }
+  return (chars.join("").match(/.{5}/g) ?? []).join("-");
 }

 export { MASKED };
@@ -5,6 +5,7 @@ import { getConnInfo } from "@hono/node-server/conninfo";
 import { config } from "./config.js";
 import { SessionStore, type LiveSession } from "./sessions.js";
 import { RateLimiter } from "./ratelimit.js";
+import { resolveClientIp } from "./clientip.js";
 import {
  type AccountInfo,
  UpstreamError,
@@ -57,17 +58,13 @@ const HOP_BY_HOP = new Set([
 ]);

 export function clientIp(c: Context): string {
-  if (config.trustProxy) {
-    const xff = c.req.header("x-forwarded-for");
-    if (xff) return xff.split(",")[0]!.trim();
-    const realIp = c.req.header("x-real-ip");
-    if (realIp) return realIp.trim();
-  }
+  let peer = "unknown";
  try {
-    return getConnInfo(c).remote.address ?? "unknown";
+    peer = getConnInfo(c).remote.address ?? "unknown";
  } catch {
-    return "unknown";
+    /* no socket information available */
  }
+  return resolveClientIp(peer, { forwardedFor: c.req.header("x-forwarded-for"), realIp: c.req.header("x-real-ip") }, config);
 }

 function isSecureRequest(c: Context): boolean {
@@ -450,6 +447,9 @@ export function createApp(): Hono<Env> {
    const accountId = c.req.param("accountId");
    const len = Number(c.req.header("content-length") ?? "0");
    if (len > config.maxUploadBytes) return c.json({ error: "too_large" }, 413);
+    // content-length is absent on a chunked request, so the header alone is a
+    // suggestion; count the bytes as they go past.
+    const body = c.req.raw.body ? c.req.raw.body.pipeThrough(byteCap(config.maxUploadBytes)) : null;
    try {
      const upstream = await getUpstreamSession(session.id, session.authorization);
      const url = absoluteUpstream(expandTemplate(upstream.uploadUrl, { accountId }));
@@ -460,7 +460,7 @@ export function createApp(): Hono<Env> {
          "content-type": c.req.header("content-type") ?? "application/octet-stream",
          accept: "application/json",
        },
-        body: c.req.raw.body,
+        body,
        duplex: "half",
        signal: AbortSignal.timeout(Math.max(config.upstreamTimeout, 5 * 60_000)),
      });
@@ -550,6 +550,18 @@ export function createApp(): Hono<Env> {
  return app;
 }

+/** Fail a stream that runs past `max` bytes, whatever its headers claimed. */
+function byteCap(max: number): TransformStream<Uint8Array, Uint8Array> {
+  let total = 0;
+  return new TransformStream<Uint8Array, Uint8Array>({
+    transform(chunk, controller) {
+      total += chunk.byteLength;
+      if (total > max) controller.error(new Error("upload too large"));
+      else controller.enqueue(chunk);
+    },
+  });
+}
+
 async function readJson<T>(c: Context): Promise<T | null> {
  try {
    return (await c.req.json()) as T;
@@ -582,10 +594,17 @@ function sessionExtras(session: LiveSession, info: AccountInfo = { locale: null,
  };
 }

+/**
+ * Headers worth relaying from the mail server. An allowlist rather than a
+ * denylist: everything else it might set — cookies, auth challenges, CORS
+ * grants — would be landing on *our* origin, where it means something else.
+ */
+const PASSTHROUGH_HEADERS = new Set(["content-type", "content-disposition", "content-language", "etag", "last-modified", "retry-after"]);
+
 function passthrough(res: Response): Response {
  const headers = new Headers();
  res.headers.forEach((v, k) => {
-    if (!HOP_BY_HOP.has(k.toLowerCase())) headers.set(k, v);
+    if (PASSTHROUGH_HEADERS.has(k.toLowerCase())) headers.set(k, v);
  });
  if (!headers.has("content-type")) headers.set("content-type", "application/json");
  headers.set("Cache-Control", "no-store");
@@ -0,0 +1,94 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { inRange, isTrustedProxy, resolveClientIp } from "./clientip.js";
+
+/**
+ * The rate limiter keys on whatever this returns, so anything a client can
+ * choose is a way to sidestep it. nginx's `$proxy_add_x_forwarded_for`
+ * *appends*, so a client sending `X-Forwarded-For: 1.2.3.4` reaches us as
+ * "1.2.3.4, <their real address>" — reading the leftmost entry hands them a
+ * key they can change per request.
+ */
+
+const cfg = { trustProxy: true, trustedProxies: [] as string[] };
+const direct = { trustProxy: false, trustedProxies: [] as string[] };
+
+test("CIDR matching covers both families and single addresses", () => {
+  assert.equal(inRange("10.1.2.3", "10.0.0.0/8"), true);
+  assert.equal(inRange("11.1.2.3", "10.0.0.0/8"), false);
+  assert.equal(inRange("172.16.5.4", "172.16.0.0/12"), true);
+  assert.equal(inRange("172.32.5.4", "172.16.0.0/12"), false);
+  assert.equal(inRange("127.0.0.1", "127.0.0.1"), true, "a bare address is a /32");
+  assert.equal(inRange("::1", "::1/128"), true);
+  assert.equal(inRange("fd00::5", "fc00::/7"), true);
+  assert.equal(inRange("2001:db8::1", "fc00::/7"), false);
+  assert.equal(inRange("10.1.2.3", "not-a-range"), false);
+  assert.equal(inRange("10.1.2.3", "::1/128"), false, "families do not cross");
+});
+
+test("loopback and private peers are trusted by default", () => {
+  for (const p of ["127.0.0.1", "::1", "10.0.0.5", "172.17.0.1", "192.168.1.9", "fd00::2"]) {
+    assert.equal(isTrustedProxy(p, cfg), true, p);
+  }
+  for (const p of ["8.8.8.8", "2001:db8::1"]) {
+    assert.equal(isTrustedProxy(p, cfg), false, p);
+  }
+});
+
+test("the real client is taken from the right, not the left", () => {
+  // What nginx produces when the client sent a forged header of their own.
+  const ip = resolveClientIp("172.17.0.1", { forwardedFor: "1.2.3.4, 203.0.113.9" }, cfg);
+  assert.equal(ip, "203.0.113.9", "the entry our own proxy observed");
+});
+
+test("a forged chain cannot move the rate-limit key", () => {
+  const forged = ["9.9.9.9", "8.8.8.8, 7.7.7.7", "203.0.113.1, 203.0.113.2, 203.0.113.3"];
+  const seen = forged.map((f) => resolveClientIp("127.0.0.1", { forwardedFor: `${f}, 198.51.100.7` }, cfg));
+  assert.deepEqual(seen, ["198.51.100.7", "198.51.100.7", "198.51.100.7"], "always the same real client");
+});
+
+test("hops we run ourselves are skipped over", () => {
+  // client → our edge proxy → our app proxy → us
+  const ip = resolveClientIp("127.0.0.1", { forwardedFor: "198.51.100.7, 10.0.0.2, 10.0.0.3" }, cfg);
+  assert.equal(ip, "198.51.100.7");
+});
+
+test("a peer we do not run is believed only about itself", () => {
+  const ip = resolveClientIp("8.8.8.8", { forwardedFor: "1.2.3.4" }, cfg);
+  assert.equal(ip, "8.8.8.8", "an untrusted peer cannot name its own client");
+});
+
+test("forwarding headers are ignored entirely when the proxy is not trusted", () => {
+  assert.equal(resolveClientIp("203.0.113.5", { forwardedFor: "1.2.3.4", realIp: "5.6.7.8" }, direct), "203.0.113.5");
+});
+
+test("X-Real-IP is a fallback, never an override", () => {
+  assert.equal(resolveClientIp("127.0.0.1", { realIp: "198.51.100.7" }, cfg), "198.51.100.7");
+  assert.equal(
+    resolveClientIp("127.0.0.1", { forwardedFor: "198.51.100.7", realIp: "1.2.3.4" }, cfg),
+    "198.51.100.7",
+    "the chain wins where there is one",
+  );
+});
+
+test("junk in the chain is discarded rather than used as a key", () => {
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "not-an-ip, 198.51.100.7" }, cfg), "198.51.100.7");
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "not-an-ip" }, cfg), "127.0.0.1", "falls back to the peer");
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "" }, cfg), "127.0.0.1");
+});
+
+test("bracketed and IPv4-mapped forms are normalised", () => {
+  assert.equal(resolveClientIp("::1", { forwardedFor: "[2001:db8::5]" }, cfg), "2001:db8::5");
+  assert.equal(resolveClientIp("::1", { forwardedFor: "::ffff:198.51.100.7" }, cfg), "198.51.100.7");
+});
+
+test("an explicit trusted list replaces the defaults", () => {
+  const only = { trustProxy: true, trustedProxies: ["203.0.113.0/24"] };
+  assert.equal(resolveClientIp("203.0.113.9", { forwardedFor: "198.51.100.7" }, only), "198.51.100.7");
+  // Loopback is no longer trusted once a list is given.
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "198.51.100.7" }, only), "127.0.0.1");
+});
+
+test("a chain of nothing but our own proxies still yields an address", () => {
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "10.0.0.2, 10.0.0.3" }, cfg), "10.0.0.2");
+});
@@ -0,0 +1,99 @@
+import { isIP } from "node:net";
+
+/**
+ * Work out who is really talking to us, for rate limiting and session records.
+ *
+ * `X-Forwarded-For` is a list that each hop appends to, so the entry nearest
+ * the right is the one our own proxy observed and the entries to its left were
+ * supplied by whoever came before — including the client. nginx's
+ * `$proxy_add_x_forwarded_for` appends, so a client sending
+ * `X-Forwarded-For: 1.2.3.4` arrives as `1.2.3.4, <their real address>`:
+ * reading the leftmost entry hands an attacker a rate-limit key they can
+ * change at will. Read from the right instead, skipping hops we run ourselves,
+ * and only believe the header at all when the peer is a proxy we trust.
+ */
+
+/** Peers whose forwarding headers are believed when none are configured. */
+const DEFAULT_TRUSTED = ["127.0.0.0/8", "::1/128", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "fc00::/7"];
+
+export interface TrustConfig {
+  trustProxy: boolean;
+  /** CIDRs or bare addresses; empty means DEFAULT_TRUSTED. */
+  trustedProxies: string[];
+}
+
+function toBits(addr: string): { value: bigint; width: number } | null {
+  const v = isIP(addr);
+  if (v === 4) {
+    const parts = addr.split(".").map(Number);
+    if (parts.length !== 4 || parts.some((n) => !Number.isInteger(n) || n < 0 || n > 255)) return null;
+    return { value: parts.reduce((acc, n) => (acc << 8n) | BigInt(n), 0n), width: 32 };
+  }
+  if (v === 6) {
+    // Expand "::" and any embedded IPv4 tail into eight 16-bit groups.
+    let text = addr;
+    const tail = /:(\d+\.\d+\.\d+\.\d+)$/.exec(text);
+    if (tail) {
+      const b = tail[1]!.split(".").map(Number);
+      text = `${text.slice(0, tail.index)}:${((b[0]! << 8) | b[1]!).toString(16)}:${((b[2]! << 8) | b[3]!).toString(16)}`;
+    }
+    const [head, rest] = text.split("::");
+    const left = head ? head.split(":").filter(Boolean) : [];
+    const right = rest !== undefined ? (rest ? rest.split(":").filter(Boolean) : []) : null;
+    const groups = right === null ? left : [...left, ...Array<string>(8 - left.length - right.length).fill("0"), ...right];
+    if (groups.length !== 8) return null;
+    let value = 0n;
+    for (const g of groups) {
+      const n = parseInt(g, 16);
+      if (!Number.isInteger(n) || n < 0 || n > 0xffff) return null;
+      value = (value << 16n) | BigInt(n);
+    }
+    return { value, width: 128 };
+  }
+  return null;
+}
+
+/** Is `addr` inside `range`, which may be a CIDR or a single address? */
+export function inRange(addr: string, range: string): boolean {
+  const [net, bitsText] = range.trim().split("/");
+  const a = toBits(addr);
+  const n = toBits(net ?? "");
+  if (!a || !n || a.width !== n.width) return false;
+  const bits = bitsText === undefined ? n.width : Number(bitsText);
+  if (!Number.isInteger(bits) || bits < 0 || bits > n.width) return false;
+  if (bits === 0) return true;
+  const shift = BigInt(n.width - bits);
+  return a.value >> shift === n.value >> shift;
+}
+
+export function isTrustedProxy(addr: string, cfg: TrustConfig): boolean {
+  const ranges = cfg.trustedProxies.length ? cfg.trustedProxies : DEFAULT_TRUSTED;
+  return ranges.some((r) => inRange(addr, r));
+}
+
+export interface ForwardHeaders {
+  forwardedFor?: string;
+  realIp?: string;
+}
+
+/**
+ * The client address to attribute a request to. `peer` is the socket address,
+ * which is the only part nobody downstream can forge.
+ */
+export function resolveClientIp(peer: string, headers: ForwardHeaders, cfg: TrustConfig): string {
+  if (!cfg.trustProxy || !peer || peer === "unknown") return peer || "unknown";
+  // A peer we do not run is not allowed to tell us who its client is.
+  if (!isTrustedProxy(peer, cfg)) return peer;
+  const chain = (headers.forwardedFor ?? "")
+    .split(",")
+    .map((s) => s.trim().replace(/^\[|\]$/g, "").replace(/^::ffff:(?=\d+\.\d+\.\d+\.\d+$)/i, ""))
+    .filter((s) => isIP(s) !== 0);
+  // Rightmost first: the last hop we trust is ours, anything left of the first
+  // untrusted entry was written by someone we have no reason to believe.
+  for (let i = chain.length - 1; i >= 0; i--) {
+    if (!isTrustedProxy(chain[i]!, cfg)) return chain[i]!;
+  }
+  if (chain.length) return chain[0]!;
+  const real = headers.realIp?.trim();
+  return real && isIP(real) !== 0 ? real : peer;
+}
@@ -65,6 +65,13 @@ export const config = {
  stalwartUrl,
  appSecret,
  trustProxy: bool("TRUST_PROXY", true),
+  /**
+   * Peers whose X-Forwarded-* headers are believed. Empty falls back to
+   * loopback and the private ranges, which covers the usual reverse proxy on
+   * the same host or Docker network. A peer outside this is attributed by its
+   * socket address whatever it claims.
+   */
+  trustedProxies: (process.env.TRUSTED_PROXIES ?? "").split(",").map((s) => s.trim()).filter(Boolean),
  /** "auto" = Secure when the request arrived over https; "1"/"0" to force. */
  secureCookies: (process.env.SECURE_COOKIES ?? "auto").toLowerCase(),
  sessionTtl: int("SESSION_TTL", 12 * 60 * 60),
@@ -63,3 +63,37 @@ test("normalizes Stalwart account locales to BCP-47 tags", () => {
  assert.equal(normalizeLocale({ locale: "de_DE" }), null);
  assert.equal(normalizeLocale("../etc/passwd"), null);
 });
+
+test("generated app passwords are unbiased and long enough", async () => {
+  const { readableSecret } = await import("./account.js");
+  const alphabet = "abcdefghijkmnopqrstuvwxyz23456789";
+  const counts = new Map<string, number>();
+  let samples = 0;
+  for (let i = 0; i < 2000; i++) {
+    const secret = readableSecret();
+    assert.match(secret, /^[a-z2-9]{5}-[a-z2-9]{5}-[a-z2-9]{5}-[a-z2-9]{5}$/, secret);
+    for (const ch of secret.replace(/-/g, "")) {
+      counts.set(ch, (counts.get(ch) ?? 0) + 1);
+      samples++;
+    }
+  }
+  assert.equal(samples, 2000 * 20);
+
+  /*
+   * `% 33` over a byte maps 25 characters onto 8 values each and the last 8
+   * onto 7, so the digits — the tail of the alphabet — would come up about
+   * 7/8 as often as they should. Testing each character on its own cannot see
+   * a skew that size against the noise, so weigh the whole tail at once:
+   * uniform puts 8/33 of the draw there, the biased version 7/8 of that, and
+   * over 40,000 draws the two are more than four standard deviations apart.
+   */
+  const tail = alphabet.slice(25); // "23456789"
+  const tailSeen = [...tail].reduce((n, ch) => n + (counts.get(ch) ?? 0), 0);
+  const p = tail.length / alphabet.length;
+  const expected = samples * p;
+  const sigma = Math.sqrt(samples * p * (1 - p));
+  assert.ok(
+    Math.abs(tailSeen - expected) < 4 * sigma,
+    `digits appeared ${tailSeen} times, expected ~${Math.round(expected)} (sigma ${sigma.toFixed(1)}) - modulo bias?`,
+  );
+});