From 4c0ffe0884a79940cc3cfed414d199b2a72f3f39 Mon Sep 17 00:00:00 2001 From: subrih Date: Fri, 13 Mar 2026 22:44:11 -0700 Subject: [PATCH] fix(access-policy): bwrap file-path tmpfs guard, seatbelt /tmp exec bit, mtime cache --- src/infra/access-policy-file.test.ts | 53 +++++++++++++++++++++++++ src/infra/access-policy-file.ts | 40 ++++++++++++++++++- src/infra/exec-sandbox-bwrap.test.ts | 17 ++++++++ src/infra/exec-sandbox-bwrap.ts | 15 ++++++- src/infra/exec-sandbox-seatbelt.test.ts | 27 +++++++++++++ src/infra/exec-sandbox-seatbelt.ts | 3 ++ 6 files changed, 153 insertions(+), 2 deletions(-) diff --git a/src/infra/access-policy-file.test.ts b/src/infra/access-policy-file.test.ts index afd54177624..6255ae48b96 100644 --- a/src/infra/access-policy-file.test.ts +++ b/src/infra/access-policy-file.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { BROKEN_POLICY_FILE, + _resetFileCacheForTest, _resetNotFoundWarnedForTest, loadAccessPolicyFile, mergeAccessPolicy, @@ -29,6 +30,7 @@ const FP_DIR = path.dirname(FP_FILE); beforeEach(() => { fs.mkdirSync(FP_DIR, { recursive: true }); _resetNotFoundWarnedForTest(); + _resetFileCacheForTest(); }); afterEach(() => { @@ -254,6 +256,57 @@ describe("loadAccessPolicyFile", () => { }); }); +// --------------------------------------------------------------------------- +// loadAccessPolicyFile — mtime cache +// --------------------------------------------------------------------------- + +describe("loadAccessPolicyFile — mtime cache", () => { + it("returns cached result on second call without re-reading the file", () => { + writeFile({ version: 1, base: { default: "r--" } }); + const spy = vi.spyOn(fs, "readFileSync"); + loadAccessPolicyFile(); // populate cache + loadAccessPolicyFile(); // should hit cache + // readFileSync should only be called once despite two loadAccessPolicyFile calls. + expect(spy.mock.calls.filter((c) => String(c[0]).includes("access-policy")).length).toBe(1); + spy.mockRestore(); + }); + + it("re-reads when mtime changes (file updated)", () => { + writeFile({ version: 1, base: { default: "r--" } }); + loadAccessPolicyFile(); // populate cache + // Rewrite the file — on most filesystems this bumps mtime. Force a detectable + // mtime change by setting it explicitly via utimesSync. + writeFile({ version: 1, base: { default: "rwx" } }); + const future = Date.now() / 1000 + 1; + fs.utimesSync(FP_FILE, future, future); + const result = loadAccessPolicyFile(); + expect(result).not.toBe(BROKEN_POLICY_FILE); + if (result === null || result === BROKEN_POLICY_FILE) { + throw new Error("unexpected"); + } + expect(result.base?.default).toBe("rwx"); + }); + + it("clears cache when file is deleted", () => { + writeFile({ version: 1, base: { default: "r--" } }); + loadAccessPolicyFile(); // populate cache + fs.unlinkSync(FP_FILE); + expect(loadAccessPolicyFile()).toBeNull(); + }); + + it("caches BROKEN_POLICY_FILE result for broken files", () => { + const spy = vi.spyOn(console, "error").mockImplementation(() => {}); + fs.writeFileSync(FP_FILE, "not json {{ broken"); + loadAccessPolicyFile(); // populate cache with BROKEN + const spy2 = vi.spyOn(fs, "readFileSync"); + const result = loadAccessPolicyFile(); // should hit cache + expect(result).toBe(BROKEN_POLICY_FILE); + expect(spy2.mock.calls.filter((c) => String(c[0]).includes("access-policy")).length).toBe(0); + spy.mockRestore(); + spy2.mockRestore(); + }); +}); + // --------------------------------------------------------------------------- // resolveAccessPolicyForAgent // --------------------------------------------------------------------------- diff --git a/src/infra/access-policy-file.ts b/src/infra/access-policy-file.ts index 60955ffaf5d..87c613ffad3 100644 --- a/src/infra/access-policy-file.ts +++ b/src/infra/access-policy-file.ts @@ -138,19 +138,57 @@ function validateAccessPolicyFileStructure(filePath: string, parsed: unknown): s */ export const BROKEN_POLICY_FILE = Symbol("broken-policy-file"); +// In-process mtime cache — avoids re-parsing the file on every agent turn. +// Cache is keyed by mtime so any write to the file automatically invalidates it. +type _FileCacheEntry = { + mtimeMs: number; + result: AccessPolicyFile | typeof BROKEN_POLICY_FILE; +}; +let _fileCache: _FileCacheEntry | undefined; + +/** Reset the in-process file cache. Only for use in tests. */ +export function _resetFileCacheForTest(): void { + _fileCache = undefined; +} + /** * Read and parse the sidecar file. * - Returns null if the file does not exist (opt-in not configured). * - Returns BROKEN_POLICY_FILE if the file exists but is malformed/unreadable * (callers must treat this as default:"---" — fail-closed). * - Returns the parsed file on success. + * + * Result is cached in-process by mtime — re-parses only when the file changes. */ export function loadAccessPolicyFile(): AccessPolicyFile | null | typeof BROKEN_POLICY_FILE { const filePath = resolveAccessPolicyPath(); - if (!fs.existsSync(filePath)) { + + // Single statSync per call: cheap enough and detects file changes. + let mtimeMs: number; + try { + mtimeMs = fs.statSync(filePath).mtimeMs; + } catch { + // File does not exist — clear cache and return null (feature is opt-in). + _fileCache = undefined; return null; } + // Cache hit: same mtime means same content — skip readFileSync + JSON.parse. + if (_fileCache && _fileCache.mtimeMs === mtimeMs) { + return _fileCache.result; + } + + // Cache miss: parse fresh and store result (including BROKEN_POLICY_FILE). + const result = _parseAccessPolicyFile(filePath); + _fileCache = { mtimeMs, result: result ?? BROKEN_POLICY_FILE }; + // _parseAccessPolicyFile returns null only for the non-existent case, which we + // handle above via statSync. If it somehow returns null here treat as broken. + return result ?? BROKEN_POLICY_FILE; +} + +function _parseAccessPolicyFile( + filePath: string, +): AccessPolicyFile | typeof BROKEN_POLICY_FILE | null { let parsed: unknown; try { const raw = fs.readFileSync(filePath, "utf8"); diff --git a/src/infra/exec-sandbox-bwrap.test.ts b/src/infra/exec-sandbox-bwrap.test.ts index bbd37064ee3..cddd53e0835 100644 --- a/src/infra/exec-sandbox-bwrap.test.ts +++ b/src/infra/exec-sandbox-bwrap.test.ts @@ -177,6 +177,23 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(bindMounts).not.toContain(`${HOME}/secret`); }); + it("narrowing rule on an existing file does not emit --tmpfs (bwrap only accepts dirs)", () => { + // Reproducer: { "default": "r--", "rules": { "~/secrets.key": "---" } } + // where ~/secrets.key is an existing file. The old code emitted --tmpfs on the + // file path, causing bwrap to abort with "Not a directory". Fix: mirror the + // isDir guard already present in the deny[] branch. + // process.execPath is always an existing file — use it as the test target. + const filePath = process.execPath; + const config: AccessPolicyConfig = { + default: "r--", + rules: { [filePath]: "---" }, + }; + const args = generateBwrapArgs(config, HOME); + const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); + // Must NOT emit --tmpfs for a file path. + expect(tmpfsMounts).not.toContain(filePath); + }); + it('"--x" rule in permissive mode gets --tmpfs overlay to block reads', () => { // Execute-only rules have no read bit — same treatment as "---" in permissive mode. const config: AccessPolicyConfig = { diff --git a/src/infra/exec-sandbox-bwrap.ts b/src/infra/exec-sandbox-bwrap.ts index 279193de2aa..7f2aa197ee6 100644 --- a/src/infra/exec-sandbox-bwrap.ts +++ b/src/infra/exec-sandbox-bwrap.ts @@ -205,7 +205,20 @@ export function generateBwrapArgs( // path is hidden even though --ro-bind / / made it readable by default. // This mirrors what deny[] does — without this, "---" rules under a permissive // default are silently ignored at the bwrap layer. - args.push("--tmpfs", p); + // Guard: bwrap --tmpfs only accepts a directory as the mount point. If the + // resolved path is a file, skip the mount and warn — same behaviour as deny[]. + // Non-existent paths are assumed to be directories (forward-protection). + let isDir = true; + try { + isDir = fs.statSync(p).isDirectory(); + } catch { + // Non-existent — assume directory. + } + if (isDir) { + args.push("--tmpfs", p); + } else { + _warnBwrapFileDenyOnce(p); + } } // Permissive base + read-only rule: already covered by --ro-bind / /; no extra mount. } diff --git a/src/infra/exec-sandbox-seatbelt.test.ts b/src/infra/exec-sandbox-seatbelt.test.ts index eaa97e95dec..b66d64db877 100644 --- a/src/infra/exec-sandbox-seatbelt.test.ts +++ b/src/infra/exec-sandbox-seatbelt.test.ts @@ -183,6 +183,33 @@ describe("generateSeatbeltProfile", () => { expect(profile).not.toMatch(/allow file-read[^)]*\(subpath "\/private\/tmp"\)/); }); + it("exec-only /tmp rule grants process-exec* on /private/tmp in restrictive mode", () => { + // Regression: when default:"---" and a rule grants exec on /tmp (e.g. "--x"), + // the seatbelt profile must emit (allow process-exec* (subpath "/private/tmp")). + // Without this fix, no exec allowance was emitted and binaries in /tmp could not + // be executed even though the policy explicitly permitted it. + const config: AccessPolicyConfig = { + default: "---", + rules: { "/tmp/**": "--x" }, + }; + const profile = generateSeatbeltProfile(config, HOME); + expect(profile).toMatch(/allow process-exec\*[^)]*\(subpath "\/private\/tmp"\)/); + // Read and write must NOT be granted. + expect(profile).not.toMatch(/allow file-read[^)]*\(subpath "\/private\/tmp"\)/); + expect(profile).not.toMatch(/allow file-write\*[^)]*\(subpath "\/private\/tmp"\)/); + }); + + it("r-x /tmp rule grants both read and exec on /private/tmp in restrictive mode", () => { + const config: AccessPolicyConfig = { + default: "---", + rules: { "/tmp/**": "r-x" }, + }; + const profile = generateSeatbeltProfile(config, HOME); + expect(profile).toMatch(/allow file-read[^)]*\(subpath "\/private\/tmp"\)/); + expect(profile).toMatch(/allow process-exec\*[^)]*\(subpath "\/private\/tmp"\)/); + expect(profile).not.toMatch(/allow file-write\*[^)]*\(subpath "\/private\/tmp"\)/); + }); + // --------------------------------------------------------------------------- // Symlink attack mitigation — profile ordering // diff --git a/src/infra/exec-sandbox-seatbelt.ts b/src/infra/exec-sandbox-seatbelt.ts index bb8ce918067..081110fc03d 100644 --- a/src/infra/exec-sandbox-seatbelt.ts +++ b/src/infra/exec-sandbox-seatbelt.ts @@ -218,6 +218,9 @@ export function generateSeatbeltProfile( if (tmpPerm[1] === "w") { lines.push(`(allow file-write* (subpath "/private/tmp"))`); } + if (tmpPerm[2] === "x") { + lines.push(`(allow ${SEATBELT_EXEC_OPS} (subpath "/private/tmp"))`); + } lines.push(`(allow process-fork)`); lines.push(`(allow signal)`); lines.push(`(allow mach*)`);