diff --git a/server/__tests__/codex-mutate.test.js b/server/__tests__/codex-mutate.test.js index 291aa7ad..f4f7438f 100644 --- a/server/__tests__/codex-mutate.test.js +++ b/server/__tests__/codex-mutate.test.js @@ -207,6 +207,65 @@ describe("codex-mutate", () => { fs.rmSync(outside, { recursive: true, force: true }); }); + it("refuses a brand-new artifact whose subdir (not the leaf) is a symlink escaping the root", () => { + const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-outside-")); + const linkPath = path.join(TMP, "agents"); + // "agents" doesn't exist yet in this fresh suite of tests for this scope — + // remove it first if an earlier test already created it as a real dir. + fs.rmSync(linkPath, { recursive: true, force: true }); + fs.symlinkSync(outside, linkPath, "dir"); + + assert.throws( + () => + cm.writeArtifact({ + scope: "user", + type: "agents", + name: "brand-new", + content: "pwned", + cwd: REPO, + }), + (e) => e.code === "EOUTOFROOT" + ); + assert.equal(fs.existsSync(path.join(outside, "brand-new.toml")), false); + fs.rmSync(linkPath, { force: true }); + fs.rmSync(outside, { recursive: true, force: true }); + }); + + it("still writes correctly when $CODEX_HOME itself is a symlink (a dotfiles checkout)", () => { + const realHome = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-realhome-")); + const linkHome = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-linkhome-")); + fs.rmdirSync(linkHome); // symlinkSync needs the link path itself to not exist + fs.symlinkSync(realHome, linkHome, "dir"); + + const prev = process.env.CODEX_HOME; + process.env.CODEX_HOME = linkHome; + delete require.cache[require.resolve("../lib/codex-home")]; + delete require.cache[require.resolve("../lib/codex-mutate")]; + const linkedCm = require("../lib/codex-mutate"); + try { + const { file, created } = linkedCm.writeArtifact({ + scope: "user", + type: "rules", + name: "via-link", + content: "ok", + cwd: REPO, + }); + assert.equal(created, true); + assert.equal(fs.readFileSync(file, "utf8"), "ok"); + assert.equal( + fs.readFileSync(path.join(realHome, "rules", "via-link.rules"), "utf8"), + "ok", + "the write must land in the real directory the symlinked CODEX_HOME points to" + ); + } finally { + process.env.CODEX_HOME = prev; + delete require.cache[require.resolve("../lib/codex-home")]; + delete require.cache[require.resolve("../lib/codex-mutate")]; + fs.rmSync(linkHome, { force: true }); + fs.rmSync(realHome, { recursive: true, force: true }); + } + }); + for (const badName of ["..", ".", "/etc/passwd", "a/b", "a\0b"]) { it(`rejects the bad name ${JSON.stringify(badName)} with EBADNAME and writes nothing`, () => { const target = path.join(TMP, "agents", `${badName.replace(/[/\0]/g, "_")}.toml`); diff --git a/server/lib/codex-mutate.js b/server/lib/codex-mutate.js index e4d96795..29b644f6 100644 --- a/server/lib/codex-mutate.js +++ b/server/lib/codex-mutate.js @@ -18,11 +18,14 @@ * - Writes are atomic via temp file + fs.renameSync (see config-write.js). * - Names are validated against a strict allowlist regex; resolved paths * are double-checked to live under the expected containment root before - * any I/O. That string-only check does not see through a symlink, so - * any existing target is additionally realpath-resolved and required to - * land inside a real Codex root (see `assertNotSymlinkEscape`) — a - * symlink that merely reorganises files within the roots (a supported - * install pattern) keeps working; one pointing outside is refused. + * any I/O. That string-only check does not see through a symlink placed + * anywhere along the path — including the type's own subdir (e.g. + * `/skills`) — so the nearest existing ancestor of the target is + * additionally realpath-resolved and required to land inside the + * realpath'd scope root (see `assertNotSymlinkEscape`). A symlink that + * merely reorganises files within the root (a supported install + * pattern, and a symlinked CODEX_HOME itself) keeps working; one + * pointing outside is refused. * - Codex has no `project-local` scope and no auto-memory: only "user" and * "project" are valid scopes. * @author Nguyễn Ngọc Trí Vĩ @@ -75,10 +78,17 @@ function fixedPathFor(type, scope, opts = {}) { } /** - * Resolve the on-disk target for a (scope, type, name) tuple AND the - * containment root used for path-traversal checks. + * Resolve the on-disk target for a (scope, type, name) tuple, the + * containment root used for string-only path-traversal checks, and the + * trusted root used for the realpath-based symlink-escape check. * - * Returns: { kind: "file" | "dir", target, filePath, containmentRoot } + * `trustedRoot` is deliberately the OUTER root (CODEX_HOME / project .codex + * dir / project root), not `containmentRoot`: for skills/agents/rules, + * `containmentRoot` is `/` (e.g. `/skills`), and that + * subdir itself is exactly what an attacker could replace with a symlink — + * checking a target against its own compromised parent would always pass. + * + * Returns: { kind: "file" | "dir", target, filePath, containmentRoot, trustedRoot } */ function resolveTarget(scope, type, name, opts = {}) { const spec = TYPES[type]; @@ -90,14 +100,18 @@ function resolveTarget(scope, type, name, opts = {}) { throw makeError("EBADNAME", `${type} is a fixed file and takes no name`); } const filePath = fixedPathFor(type, scope, opts); - return { kind: "file", target: filePath, filePath, containmentRoot: path.dirname(filePath) }; + // No attacker-placeable segment between the trusted root and the fixed + // filename, so the containment root doubles as the trusted root. + const root = path.dirname(filePath); + return { kind: "file", target: filePath, filePath, containmentRoot: root, trustedRoot: root }; } if (typeof name !== "string" || !NAME_RE.test(name)) { throw makeError("EBADNAME", `name must match ${NAME_RE}`); } - const subdirAbs = path.join(rootForScope(scope, opts), spec.subdir); + const root = rootForScope(scope, opts); + const subdirAbs = path.join(root, spec.subdir); if (spec.kind === "dir") { const target = path.join(subdirAbs, name); @@ -106,11 +120,12 @@ function resolveTarget(scope, type, name, opts = {}) { target, filePath: path.join(target, spec.filename), containmentRoot: subdirAbs, + trustedRoot: root, }; } const target = path.join(subdirAbs, name + spec.ext); - return { kind: "file", target, filePath: target, containmentRoot: subdirAbs }; + return { kind: "file", target, filePath: target, containmentRoot: subdirAbs, trustedRoot: root }; } /** /codex-config-backups//, always rooted the same way as the read side's . */ @@ -122,33 +137,51 @@ function rmTreeSync(p) { fs.rmSync(p, { recursive: true, force: true }); } -/** realpath if it exists, else the plain resolved path — for roots that may not exist yet. */ -function safeRealpath(p) { - try { - return fs.realpathSync(p); - } catch { - return path.resolve(p); - } -} - /** * Symlinks are a supported install pattern (Task 3's reader follows them), - * so a symlink at `target` is not rejected outright — only one that - * dereferences outside both Codex roots. A missing/dangling symlink is a - * fresh create, not an error. Must run before anything reads through - * `target` (backup or write), since string-only containment checks never - * see through a link. + * so a symlink under `trustedRoot` is not rejected outright — only one that + * dereferences outside it. `trustedRoot` itself may be a symlink (a + * dotfiles-managed CODEX_HOME is a normal setup) — that's fine, it just + * means everything is compared against wherever it really points. + * + * `target` may not exist yet (a fresh create). Realpath can't resolve a + * path that isn't there, so this walks up to the nearest ancestor that DOES + * exist, realpath's that, and requires it to land inside the realpath'd + * root. Everything below that existing ancestor is plain path segments this + * module built itself from a NAME_RE-validated name or a fixed literal, so + * they can't smuggle in a symlink — only an EXISTING ancestor can. + * + * `trustedRoot` is created first (mkdirSync recursive — safe, it is never + * attacker-controlled) so the walk is guaranteed to bottom out at or below + * it, never wander past it into whatever happens to contain it. + * + * ponytail: this check and the write/delete that follows are not atomic — + * a symlink swapped in between them could still redirect one write. Closing + * that needs O_NOFOLLOW-style fd work with real platform variance; out of + * scope for this task (see task-5 fix round 2). */ -function assertNotSymlinkEscape(target, cwd) { +function assertNotSymlinkEscape(trustedRoot, target) { + fs.mkdirSync(trustedRoot, { recursive: true }); + const realRoot = fs.realpathSync(trustedRoot); + + let dir = target; let real; - try { - real = fs.realpathSync(target); - } catch { - return; + for (;;) { + try { + real = fs.realpathSync(dir); + break; + } catch { + const parent = path.dirname(dir); + if (parent === dir) { + real = dir; // hit the filesystem root without finding anything real + break; + } + dir = parent; + } } - const roots = [safeRealpath(getCodexHome()), safeRealpath(getProjectCodexDir(cwd))]; - if (!roots.some((root) => isUnder(root, real))) { - throw makeError("EOUTOFROOT", `resolved path escapes the Codex roots: ${real}`); + + if (!isUnder(realRoot, real)) { + throw makeError("EOUTOFROOT", `resolved path escapes the Codex root: ${real}`); } } @@ -169,7 +202,7 @@ function writeArtifact(args) { const r = resolveTarget(scope, type, name, { cwd }); assertUnder(r.containmentRoot, r.target); - assertNotSymlinkEscape(r.target, cwd); + assertNotSymlinkEscape(r.trustedRoot, r.target); const existedBefore = fs.existsSync(r.filePath); const backup = existedBefore @@ -201,7 +234,7 @@ function deleteArtifact(args) { if (!fs.existsSync(r.target)) { throw makeError("ENOTFOUND", `${type}/${name || TYPES[type].file} does not exist`); } - assertNotSymlinkEscape(r.target, cwd); + assertNotSymlinkEscape(r.trustedRoot, r.target); const backup = createBackup({ source: r.target,