From a467000b3dd3886a0256ec8510faebb7a2933d79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Wed, 9 Sep 2026 23:34:18 +0100 Subject: [PATCH] a symlink the owner put inside the root may lead out of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failures on 2026-09-09, one cause each, both found by a 212-track album that would not convert. ── Symlinks ── paths.ts ran realpath FIRST and required the resolved path to be inside WORK_ROOT, so a link in the owner's home pointing at another disk was refused. On a machine whose home is BUILT from such links — ~/Music, ~/Movies, ~/Torrents each pointing at a mount — that leaves av-club able to see almost nothing. It refused every track of a folder the owner had open in the file browser at the time. That is the tell. The file browser decides what the owner may touch and it launches these jobs; its resolveUserPath checks the path as written and never calls realpath. Two confinement rules over the same files means the platform offers a right-click action its own sidecar cannot perform. What the strict rule was worth is smaller than it looks: this process runs AS the owner, so it reads nothing the owner cannot already read, and a link out of home reaches only more of the owner's disks. realpath-first was never the privilege boundary — the OS is. `..` is now refused rather than normalised, which is what realpath-first actually bought. Normalising is not what the kernel does: for /home/me/Music/.. the string says /home/me while the kernel follows Music to another filesystem first. Refusing removes the divergence instead of picking a side. The narrower claim that remains, and is the one to defend: a caller cannot NAME a path outside the root; they can only follow a link the owner put inside it. ── The token ── requireToken() threw when AV_CLUB_TOKEN was unset. That is right for a service a person starts with a shell and wrong for a sidecar the platform starts: not a safe default, a crash loop nobody sees. It was exactly that — 15 restarts, every proxied request 503, and an error that named the files rather than the missing variable. Generating one loses nothing: authorised() already accepts the platform's X-Officer-User header with no token at all, so no real caller carried this secret. It guards the direct loopback door, and 32 random bytes guard it better than a value copied into a shell profile. Kept 0600 beside the assets so replacing the working tree does not change the service's identity. AV_CLUB_TOKEN still wins when set. There is no unauthenticated mode; this only decides where the token comes from. --- docs/SECURITY.md | 35 ++++++++++++++++++------ sidecar/config.ts | 61 ++++++++++++++++++++++++++++++++++------- sidecar/paths.ts | 70 ++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 138 insertions(+), 28 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 3e6b855..dfbb23f 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -6,10 +6,11 @@ change can be checked against the reasoning rather than against a vibe. **Read this before adding an endpoint, an option, or a filter.** -## The one invariant, as revised on 2026-08-29 +## The one invariant, as revised on 2026-09-09 > **The caller may name a path. `sidecar/paths.ts` is the ONLY thing that decides whether it is one -> this service will touch, and it confines every one of them to `WORK_ROOT`.** +> this service will touch, and every path a caller NAMES is inside `WORK_ROOT`. A symlink the owner +> put inside the root may lead out of it.** Read the section below for what it replaced and why, because the change is not a relaxation of the threat model — it is a correction to a rule that had stopped describing what av-club is for. @@ -44,18 +45,36 @@ What is gone is only the claim that a path never appears. What replaced it is on to read in a minute, that every path goes through: - **absolute**, because a relative path resolves against a cwd the caller cannot see -- **`realpath` FIRST**, so a symlink inside the root pointing at `/etc/shadow` is resolved and then - rejected — checking the path as written and opening the resolved one is the classic way this fails -- **inside the root as `root` or `root + sep`**, never a bare `startsWith`, which accepts +- **no `..` segment**, refused rather than normalised. Normalising is not what the kernel does: for + `/home/me/Music/..` the string says `/home/me`, while the kernel follows `Music` to another disk + first and says something else entirely. Refusing removes the divergence rather than picking a side +- **inside the root as written**, as `root` or `root + sep`, never a bare `startsWith`, which accepts `/home/user-backup` for a root of `/home/user` -- for an output, the **parent** is resolved and checked, and it must already exist — creating +- for an output, the **parent** is checked the same way, and it must already exist — creating directories toward an output is a side effect nobody asked for - errors never distinguish "outside the root" from "does not exist", which would make this an oracle for what exists elsewhere on the disk +**Symlinks leave the root, deliberately, changed 2026-09-09.** `realpath` used to run FIRST and the +resolved path had to be inside the root, so a link in the owner's home pointing at another disk was +refused. On a machine whose home is BUILT from such links — `~/Music`, `~/Movies`, `~/Torrents` each +pointing at a mount — that rule left av-club able to see almost nothing. It refused all 212 tracks of +an album the owner had open in the file browser at the time. + +The file browser is the component that decides what the owner may touch, and it launches these jobs; +its `resolveUserPath` checks the path as written and never calls `realpath`. Two different rules over +the same files means the platform offers a right-click action its own sidecar cannot perform. + +What that strictness was worth is smaller than it looks: this process runs AS the owner, so it reads +nothing the owner cannot already read, and a link out of home reaches only more of the owner's disks. +`realpath`-first was never the privilege boundary — the OS is. The claim that remains true, and is the +one to defend: **a caller cannot NAME a path outside the root; they can only follow a link the owner +put inside it.** + Verified 2026-08-29 against the running service: `..` traversal, an absolute path outside the root, a -write outside the root, a relative path, a symlink pointing into `/etc`, and a write through a -symlinked directory are all refused; nothing was written outside the root. +write outside the root, and a relative path are all refused; nothing was written outside the root. The +two symlink cases in that list — a link into `/etc`, and a write through a symlinked directory — are +now permitted by design, per the paragraph above. ### What that invariant is defending against diff --git a/sidecar/config.ts b/sidecar/config.ts index 2bb44da..d01678d 100644 --- a/sidecar/config.ts +++ b/sidecar/config.ts @@ -1,4 +1,6 @@ -import { resolve } from "node:path"; +import { randomBytes } from "node:crypto"; +import { mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { dirname, resolve } from "node:path"; /** * Configuration, read once at boot. @@ -127,16 +129,55 @@ export const MAX_CONCURRENT_JOBS = num("AV_CLUB_MAX_CONCURRENT_JOBS", 2, 1); * read — the doc generator imports the sharp vocabulary, which imports MAX_OUTPUT_DIMENSION from * here — and a module-scope throw meant that generating documentation required a production secret. * - * The check itself still happens at boot: server.ts calls this before it listens, so a missing token - * is a refusal to start rather than a service that quietly accepts everyone. There is no - * "unauthenticated by default" mode; this process runs decoders over untrusted bytes. + * `AV_CLUB_TOKEN` still wins when it is set — that is how the standalone deployment pins a value + * something else already knows. When it is not set, one is generated and kept (see below) rather than + * refused. There is still no "unauthenticated by default" mode: the door always wants a token, this + * only decides where that token comes from. */ export function requireToken(): string { - const value = process.env.AV_CLUB_TOKEN; - if (!value || value.length < 16) { - throw new Error( - "AV_CLUB_TOKEN must be set and at least 16 characters. Generate one with: openssl rand -hex 32", - ); + const fromEnv = process.env.AV_CLUB_TOKEN; + if (fromEnv) { + if (fromEnv.length < 16) { + throw new Error("AV_CLUB_TOKEN, when set, must be at least 16 characters."); + } + return fromEnv; } - return value; + + return tokenFromStateDir(); +} + +/** + * The token, generated and kept on disk when nothing supplied one. + * + * This used to throw, and that was wrong for a plugin. A sidecar is started by the platform, not by a + * person with a shell — so "refuse to boot until an operator exports a variable" is not a safe default, + * it is a crash loop nobody sees. On 2026-09-09 it was exactly that: av-club restarted 15 times, and + * every request the platform proxied to it came back 503. A whole 212-file conversion failed with an + * error that named the files, not the missing variable. + * + * Generating one loses nothing. `authorised()` already accepts the platform's `X-Officer-User` header + * without a token at all, so no real caller was ever carrying this secret — it only guards the direct + * loopback door, and a random 32 bytes guards that better than a value copied into a shell profile. + * + * Written 0600 next to the assets, for the same reason they live there: a deploy that replaces the + * working tree must not change the service's identity. + */ +function tokenFromStateDir(): string { + const file = resolve(ASSETS_DIR, "..", "token"); + + // Missing is the normal first-boot case, not an error. Anything else — a directory in the way, a + // permission problem — is also survivable: fall through and mint a fresh one rather than refuse to start. + const existing = (() => { + try { + return readFileSync(file, "utf8").trim(); + } catch { + return ""; + } + })(); + if (existing.length >= 16) return existing; + + const generated = randomBytes(32).toString("hex"); + mkdirSync(dirname(file), { recursive: true }); + writeFileSync(file, `${generated}\n`, { mode: 0o600 }); + return generated; } diff --git a/sidecar/paths.ts b/sidecar/paths.ts index 28a91e2..951bf4f 100644 --- a/sidecar/paths.ts +++ b/sidecar/paths.ts @@ -24,13 +24,39 @@ import { invalid } from "./errors"; * ABSOLUTE. A relative path resolves against this process's cwd, which is not the caller's and is not * anything they can see. There is no useful meaning for it, so it is refused rather than guessed at. * - * REAL. `realpath` follows every symlink before the check, so a link inside the root pointing at - * /etc/shadow is resolved and then rejected. Checking the path as written and opening the resolved one - * is the classic way this goes wrong. + * NO `..` SEGMENT, refused rather than normalised. This is what `realpath`-first used to buy, and it + * buys it without the cost below. Normalising `..` in the string is not the same operation the kernel + * performs: for `/home/me/Music/..`, `resolve` says `/home/me` while the kernel — following `Music` + * to another filesystem first — says the parent of the link's TARGET. Refusing the segment outright + * removes the divergence instead of picking a side, and no legitimate caller sends one. * - * INSIDE THE ROOT, tested as `root` or `root + sep`. A bare `startsWith` accepts `/home/user-backup` - * for a root of `/home/user`, which is a different person's files — the same bug the platform's own - * file browser documents in `isInside`. + * INSIDE THE ROOT AS WRITTEN, tested as `root` or `root + sep`. A bare `startsWith` accepts + * `/home/user-backup` for a root of `/home/user`, which is a different person's files — the same bug + * the platform's own file browser documents in `isInside`. + * + * ── Symlinks LEAVE the root, deliberately. This reverses docs/SECURITY.md. ── + * + * The old rule ran `realpath` FIRST and required the resolved path to be inside the root, so a link in + * the owner's home pointing anywhere else was refused. On a machine whose home is BUILT from such + * links — `~/Music`, `~/Movies`, `~/Torrents` each pointing at a mounted disk — that rule means av-club + * can only see the few files physically on the home partition. On 2026-09-09 it refused all 212 tracks + * of an album with "inputPath is not a readable path", for a folder the owner was looking at in the + * file browser at the time. + * + * That is the tell. The file browser is the component that decides what the owner may touch, and it + * launched this job; its `resolveUserPath` checks the path AS WRITTEN and never calls `realpath`. Two + * different confinement rules over the same files means a right-click action the platform offers is one + * the sidecar structurally cannot perform. The file browser's rule is the correct one, because it is + * the one the owner can see the consequences of. + * + * What the strict rule was actually worth here is smaller than it looks. This process runs AS the owner, + * so it can read nothing the owner cannot already read, and a symlink out of home reaches only more of + * the owner's own disks. `realpath`-first was never an OS privilege boundary — the OS is that. It + * defends against a *path* being attacker-chosen, and absolute-only, no-`..`, and inside-the-root all + * still hold that line. + * + * The narrower claim that remains true: a caller cannot name a path outside the root. They can only + * follow a link the OWNER put inside it. */ /** A path this service is willing to read from or write to. Branded so it cannot be confused with a string. */ @@ -38,6 +64,15 @@ export type SafePath = string & { readonly __safe: unique symbol }; const inside = (root: string, target: string) => target === root || target.startsWith(root + sep); +/** + * A `..` anywhere in the path, as the caller wrote it. + * + * Checked on the RAW string, before any normalising, because normalising is the thing being avoided — + * see the header. Split on `sep` so a filename that merely contains dots (`..hidden`, `a..b.flac`) is + * untouched; only a whole segment counts. + */ +const hasDotDot = (raw: string) => raw.split(sep).includes(".."); + /** * For an INPUT: the file must exist, so its real path can be resolved and checked. * @@ -47,9 +82,16 @@ const inside = (root: string, target: string) => target === root || target.start export async function safeInputPath(raw: unknown, field: string): Promise { if (typeof raw !== "string" || !raw.trim()) throw invalid(`${field} must be a path`, field); if (!isAbsolute(raw)) throw invalid(`${field} must be an absolute path`, field); + if (hasDotDot(raw)) throw invalid(`${field} must not contain '..'`, field); - const real = await realpath(resolve(raw)).catch(() => null); - if (!real || !inside(WORK_ROOT, real)) throw invalid(`${field} is not a readable path`, field); + const full = resolve(raw); + if (!inside(WORK_ROOT, full)) throw invalid(`${field} is not a readable path`, field); + + // Resolved AFTER the check, and not re-checked: following the owner's own symlink off the home + // partition is the point. This still runs, because ffmpeg should be handed a path that cannot change + // meaning under it, and because a file that does not exist must fail here rather than inside a decoder. + const real = await realpath(full).catch(() => null); + if (!real) throw invalid(`${field} is not a readable path`, field); return real as SafePath; } @@ -64,12 +106,20 @@ export async function safeOutputPath(raw: unknown, field: string): Promise null); - if (!parent || !inside(WORK_ROOT, parent)) throw invalid(`${field} is not a writable path`, field); + const parentAsWritten = full.slice(0, full.lastIndexOf(sep)) || sep; + if (!inside(WORK_ROOT, parentAsWritten)) throw invalid(`${field} is not a writable path`, field); + + // As above: resolved after the check, so a write lands on the disk the owner's link points at rather + // than being refused. The parent must already exist — creating directories on the way to an output is + // a side effect nobody asked for, and a typo would scatter empty folders through somebody's home. + const parent = await realpath(parentAsWritten).catch(() => null); + if (!parent) throw invalid(`${field} is not a writable path`, field); return `${parent}${sep}${name}` as SafePath; }