diff --git a/.env.example b/.env.example index f5f1ea7..140f19f 100644 --- a/.env.example +++ b/.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. diff --git a/README.md b/README.md index c4420af..7fb4ede 100644 --- a/README.md +++ b/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) | diff --git a/server/src/account.ts b/server/src/account.ts index 80f769b..d695208 100644 --- a/server/src/account.ts +++ b/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 }; diff --git a/server/src/app.ts b/server/src/app.ts index c43b301..04e1ea1 100644 --- a/server/src/app.ts +++ b/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 { 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 { "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 { return app; } +/** Fail a stream that runs past `max` bytes, whatever its headers claimed. */ +function byteCap(max: number): TransformStream { + let total = 0; + return new TransformStream({ + transform(chunk, controller) { + total += chunk.byteLength; + if (total > max) controller.error(new Error("upload too large")); + else controller.enqueue(chunk); + }, + }); +} + async function readJson(c: Context): Promise { 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"); diff --git a/server/src/clientip.test.ts b/server/src/clientip.test.ts new file mode 100644 index 0000000..fdbcfd9 --- /dev/null +++ b/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, " — 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"); +}); diff --git a/server/src/clientip.ts b/server/src/clientip.ts new file mode 100644 index 0000000..d6378a0 --- /dev/null +++ b/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, `: + * 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(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; +} diff --git a/server/src/config.ts b/server/src/config.ts index be3e13f..abd942f 100644 --- a/server/src/config.ts +++ b/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), diff --git a/server/src/sessions.test.ts b/server/src/sessions.test.ts index 91ae914..9290584 100644 --- a/server/src/sessions.test.ts +++ b/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(); + 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?`, + ); +});