Merge pull request #18 from LINUXexpert-org/audit-hardening

Harden four things the security audit turned up

.env.example

@@ -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.

README.md

@@ -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) |

server/src/account.ts

@@ -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 };

server/src/app.ts

@@ -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");

server/src/clientip.test.ts

@@ -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");
+});

server/src/clientip.ts

@@ -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;
+}

server/src/config.ts

@@ -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),

server/src/sessions.test.ts

@@ -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?`,
+  );
+});