From 579f6b6e2485d8efde09fb0bae71fbf4323365f8 Mon Sep 17 00:00:00 2001 From: axisrow Date: Mon, 20 Jul 2026 10:47:36 +0800 Subject: [PATCH] fix(security): neutralize symlink replay via core.symlinks=false (closes #15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REDESIGN after four cycles of detection-based fixes each found a new TOCTOU bypass (index, refs/replace). Same pattern as worktree-cleanup whack-a-mole — remove the vulnerable path rather than guard it. Neutralize: apply with git -c core.symlinks=false apply --index. Symlink-mode entries materialize as regular text files (target path as content), NOT symlinks. Cannot point at host file, cannot exfil. Class disappears by construction. Byte-preserving patch and --no-ext-diff kept; detection logic removed. Tests (3, green): new symlink -> regular file; retargeted -> regular file; regular file mentioning mode 120000 -> applied normally. Verified: worktree 16/16; full npm test 159 pass / 4 pre-existing. Co-Authored-By: Claude Opus 4.8 (1M context) --- plugins/codex/scripts/lib/git.mjs | 41 ++++++++++-------- tests/worktree.test.mjs | 72 ++++++++++++++++++++++--------- 2 files changed, 74 insertions(+), 39 deletions(-) diff --git a/plugins/codex/scripts/lib/git.mjs b/plugins/codex/scripts/lib/git.mjs index bd302284..08290cbf 100644 --- a/plugins/codex/scripts/lib/git.mjs +++ b/plugins/codex/scripts/lib/git.mjs @@ -370,29 +370,32 @@ export function applyWorktreePatch(repoRoot, worktreePath, baseCommit) { try { // Byte-preserving: write the patch via `git diff --cached --binary --output` // (git→file, never through a JS string). runCommand decodes stdout as UTF-8, - // which would replace invalid bytes (0xff, NUL, legacy encodings) with U+FFFD - // and apply a corrupted patch as "success". Detect empty via file size. + // which would replace invalid bytes (0xff, NUL, legacy encodings) with U+FFFD. + // --no-ext-diff/--no-textconv pin diff to built-in behavior so a poisoned + // diff.external driver (writable via the linked-worktree shared commondir) + // cannot affect patch generation. gitChecked(worktreePath, ["add", "-A"]); - gitChecked(worktreePath, ["diff", "--cached", "--binary", baseCommit, "--output", patchPath]); + gitChecked(worktreePath, [ + "diff", "--no-ext-diff", "--no-textconv", "--binary", "--cached", baseCommit, "--output", patchPath + ]); if (!fs.existsSync(patchPath) || fs.statSync(patchPath).size === 0) { return { applied: false, detail: "No tracked changes to apply." }; } - // SECURITY (#15): a symlink created in the worktree (ln -s ~/.ssh/authorized_keys - // ./steal) is captured as a mode-120000 diff entry with the target verbatim. - // `git apply --index` would recreate the symlink in repoRoot pointing at the - // attacker-chosen host path → exfil/append primitive. git apply rejects literal - // "../" filename paths, but symlink-mode entries bypass that guard (the repo-side - // filename is normal; only the symlink target points outside). Reject the patch - // if any mode-120000 line appears. Leave-branch preserved: worktree stays for - // manual inspection. - const patchContent = fs.readFileSync(patchPath, "utf8"); - if (/^(?:new file|deleted file|old|new) mode 120000$/m.test(patchContent)) { - return { - applied: false, - detail: `Worktree contains symlinks (mode 120000); patch not applied to prevent a host-file redirect via repoRoot. Inspect and copy them manually from ${worktreePath}.` - }; - } - const applyResult = git(repoRoot, ["apply", "--index", patchPath]); + // SECURITY (#15, redesign — neutralize, not detect): a symlink created or + // retargeted in the worktree would be replayed by `git apply` into repoRoot + // pointing at an attacker-chosen host path → exfil/append primitive. Four + // cycles of DETECTION (patch-text scan, --raw, frozen-tree) each found a new + // TOCTOU/mutable-indirection bypass (index, refs/replace, …). git has too many + // layers of mutable object resolution to make two-phase detection airtight. + // + // Instead, NEUTRALIZE: apply with `core.symlinks=false`. git then materializes + // any symlink-mode entry as a regular text file containing the target path, + // NOT as a symlink — it cannot point at a host file, cannot exfil, cannot + // append. The symlink-replay class disappears by construction, with no + // detection race to win. Legitimate symlinks in the worktree also land as + // text files (rare in code; the render layer already warns to inspect + // special files manually). + const applyResult = git(repoRoot, ["-c", "core.symlinks=false", "apply", "--index", patchPath]); if (applyResult.status !== 0) { return { applied: false, detail: applyResult.stderr.trim() || "Patch apply failed (conflicts?)." }; } diff --git a/tests/worktree.test.mjs b/tests/worktree.test.mjs index 10dc9b05..6f3bd530 100644 --- a/tests/worktree.test.mjs +++ b/tests/worktree.test.mjs @@ -334,38 +334,39 @@ test("renderWorktreeTaskResult inspection command references baseCommit (staged- } }); -// SECURITY regression (#15): a symlink created in the worktree is captured as a -// mode-120000 diff entry. Without the guard, `git apply --index` would recreate -// the symlink in repoRoot pointing at an attacker-chosen host path → host-file -// exfil/append. keep must detect mode 120000 in the patch and refuse to apply, -// leaving the worktree in place. -test("keep refuses to apply a patch containing a symlink (mode 120000) and preserves the worktree", () => { +// SECURITY regression (#15, neutralize redesign): a symlink created in the +// worktree is applied with `core.symlinks=false`, so git materializes it as a +// REGULAR TEXT FILE containing the target path — NOT as a symlink. It cannot +// point at a host file, cannot exfil, cannot append. Four cycles of detection +// (patch-text scan, --raw, frozen-tree) each found a new TOCTOU bypass; the +// neutralize approach removes the symlink-replay class by construction. +test("keep applies a worktree symlink as a regular file (core.symlinks=false neutralize), not a host redirect", () => { const { repoRoot } = createRepoWithInitialCommit(); const session = createWorktreeSession(repoRoot); try { - // Plant a symlink in the worktree pointing at an absolute host path. fs.symlinkSync("/etc/passwd", path.join(session.worktreePath, "steal")); const result = cleanupWorktreeSession(session, { keep: true }); - assert.equal(result.applied, false); - assert.match(result.detail, /symlinks/i); - // The symlink must NOT have been recreated in repoRoot. - assert.equal(fs.existsSync(path.join(repoRoot, "steal")), false, "symlink not replayed into repoRoot"); - // Worktree preserved (leave-branch). - assert.equal(result.preserved, true); + assert.equal(result.applied, true); + // The entry MUST exist as a regular file, NOT a symlink. + const target = path.join(repoRoot, "steal"); + const st = fs.lstatSync(target); + assert.equal(st.isFile(), true, "symlink applied as a regular file, not a symlink"); + assert.equal(st.isSymbolicLink(), false, "no host-file symlink created in repoRoot"); + // Content is the defused target path as text. + assert.equal(fs.readFileSync(target, "utf8"), "/etc/passwd"); } finally { removeSession(session); } }); -// SECURITY regression (#15, false-positive guard): a regular file whose CONTENT -// happens to contain the literal "mode 120000" string (e.g. documentation of git -// modes) must NOT trip the symlink guard. Only real diff mode headers -// ("new file mode 120000" etc.) are symlinks. The regex matches line-anchored -// mode headers, not arbitrary content. -test("keep applies a regular file whose content literally mentions mode 120000 (no false positive)", () => { +// SECURITY regression (#15, false-positive was a detection artifact — under +// neutralize there is nothing to false-positive on). A regular file whose +// content happens to mention "mode 120000" applies normally (it always did; +// this test now just confirms the redesign didn't regress ordinary applies). +test("keep applies a regular file whose content literally mentions mode 120000", () => { const { repoRoot } = createRepoWithInitialCommit(); const session = createWorktreeSession(repoRoot); @@ -377,9 +378,40 @@ test("keep applies a regular file whose content literally mentions mode 120000 ( const result = cleanupWorktreeSession(session, { keep: true }); - assert.equal(result.applied, true, "regular file applied despite the literal mode string in content"); + assert.equal(result.applied, true); assert.match(fs.readFileSync(path.join(repoRoot, "modes.md"), "utf8"), /mode 120000/); } finally { removeSession(session); } }); + +// SECURITY regression (#15 reopen): an existing tracked symlink that Codex +// RETARGETS. Under neutralize (core.symlinks=false apply), the retarget is +// applied but the result in repoRoot is a regular file with the new target as +// text — the host-pointing symlink is NOT created. (Pre-neutralize this was the +// bypass that broke #19's regex; frozen-tree closed the TOCTOU but not the +// replace-ref vector. Neutralize closes the class.) +test("keep applies a retargeted existing symlink as a regular file (no host redirect)", () => { + const { repoRoot } = createRepoWithInitialCommit(); + fs.symlinkSync("/tmp/codex-benign-target", path.join(repoRoot, "existing_link")); + assert.equal(run("git", ["add", "existing_link"], { cwd: repoRoot }).status, 0); + assert.equal(run("git", ["commit", "-m", "add benign symlink"], { cwd: repoRoot }).status, 0); + + const session = createWorktreeSession(repoRoot); + + try { + fs.rmSync(path.join(session.worktreePath, "existing_link")); + fs.symlinkSync("/etc/passwd", path.join(session.worktreePath, "existing_link")); + + const result = cleanupWorktreeSession(session, { keep: true }); + + assert.equal(result.applied, true); + const target = path.join(repoRoot, "existing_link"); + const st = fs.lstatSync(target); + assert.equal(st.isSymbolicLink(), false, "no host-pointing symlink in repoRoot"); + assert.equal(st.isFile(), true, "retarget applied as a regular file"); + assert.equal(fs.readFileSync(target, "utf8"), "/etc/passwd"); + } finally { + removeSession(session); + } +});