Keep the default write paths out of a read-denied directory - #506
Keep the default write paths out of a read-denied directory#506ronleizrowice-ant wants to merge 6 commits into
Conversation
~/.npm/_logs and ~/.claude/debug are added to every write allowlist as a convenience; under a denyRead that covers the home they were bound back over the deny on Linux, readable and writable, and writable on macOS. getDefaultWritePaths now drops a default that lies at or under a read-denied directory; a caller who wants it lists it in allowWrite.
getDefaultWritePaths compares each recommended path, as listed and as normalizePathForSandbox spells it, against the normalized denyRead entries through one at-or-under helper instead of four inline clauses. wrapWithSandbox computes the effective denyRead (configured entries unioned with credential deny paths) once, ahead of the write config, and hands that to getDefaultWritePaths rather than spelling the customConfig/config fallback a second time.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Could getDefaultWritePaths() account for denyRead globs too? On Linux, ~/.npm/* is expanded later to a concrete _logs deny, but this helper ignores non-trailing globs and still adds _logs to allowOnly, which can bind it back over the read deny. That seems to leave the same fail-open path for glob-based policies.
…orce getDefaultWritePaths(denyRead) now prunes just ~/.npm/_logs and ~/.claude/debug. The device nodes and /tmp/claude are what the sandbox itself needs (stdio, TMPDIR) and stay whatever is read-denied, so a denyRead of '/', '/tmp' or '/dev' behaves as it did before. A 'dir/**' entry is stripped before it is normalized, so '~/**' and '<home>/**' prune like '~'; the earlier code stripped the suffix only for its glob test and then compared the unstripped spelling, which matched nothing. A glob that reaches the function unexpanded prunes a convenience when it matches that directory or one above it, instead of being skipped. All three call sites pass the resolved read-deny list: wrapWithSandbox its expanded list (credential denies unioned, globs expanded on Linux), getFsWriteConfig and the Linux violation monitor getFsReadConfig().denyOnly, so the reported write config agrees with what a wrap enforces. Containment goes through the shared isAtOrUnder. Tests: the '~/**' spellings, a glob over the parent directory, the sandbox's own paths under root-level denies, and one case through SandboxManager (a filesystem deny and a credential file deny) that fails if a call site stops passing the list.
|
Merged
Local run (Linux x86-64): eslint, prettier and |
The subtree-extended regex for a glob deny lived privately in the macOS backend. It moves, unchanged, to sandbox-utils beside globToRegex and is exported, so the default-write-path check can ask "does this glob deny cover this directory" with the same rule the profile is built from.
getDefaultWritePaths() now takes { denyRead, allowRead } as configured,
not an expanded deny list, and the manager feeds it through one helper
that adds the credential file denies from the pure
getCredentialDenyReadPaths(). getFsWriteConfig() is a plain getter again:
it no longer calls getFsReadConfig(), so it runs no credential masking
(which reads and rewrites files, warns, and throws for
onExtractNoMatch: "error"), lists nothing from disk, and compiles no
pattern from a file name. On Linux the expanded list turned a real file
called backup[2024-01-15].env into a regex that failed to compile, out
of every wrapWithSandbox() and getFsWriteConfig().
The check itself:
- returns the constant list, touching neither cwd nor disk, when there
are no read rules (the exported zero-argument call);
- normalizes each entry once, and spells a home directory from the home
directory that exists rather than from a leaf that may not, so a HOME
under /tmp or /var on macOS is covered before ~/.npm/_logs exists;
- matches globs with the shared denyGlobRegex, and compares an entry
whose brackets are no valid class as the literal it may be;
- keeps a home directory an allowRead entry beneath the covering deny
re-opens: with denyRead ['~'] and allowRead ['~/.claude'] nothing is
bound back over a deny, and macOS was losing the write allow it had.
normalizePathForSandbox() reads the cwd only for a relative path, so an
absolute or ~ entry does not throw under Node once the cwd is removed.
The Linux violation monitor goes back to the full defaults. A directory a
wrap leaves out sits under the read-deny's writable tmpfs, so a write
there succeeds; filtering the monitor's list reported it as a violation,
and an initialize()-time snapshot cannot follow per-call or updated
configs anyway. The write config getFsWriteConfig() reports is what
agrees with a wrap; the monitor never did.
The two kinds of default are named constants, the contract is in the
exported function's JSDoc, and the README says when the home directories
are dropped. Tests: each deny spelling by name, the allowRead cases, an
invariant that nothing stays under a read-denied home, and, in a process
with its own HOME where both directories exist, getFsWriteConfig() and
the wrapped command for a filesystem deny, a credential deny and a
per-call allowRead; on Linux, a bracketed file under a denyRead glob and
a failing credential mask leave the getter and the wrap alone.
|
Second round on top of
Both go away with one change: the check takes the entries as configured ( Also in this round: an 23 cases, seven of eight mutations turn the file red (the survivor is behaviourally equivalent). eslint, prettier, |
2 participants
Problem
getDefaultWritePaths()adds ten paths to every write allow-list. Eight are what the sandbox itself needs (the child's stdio,/tmp/claude). Two are conveniences under the home directory that the caller never asked for:~/.npm/_logsand~/.claude/debug.With
denyRead: ['~'](or['~/.npm'], or a credential file deny on~/.claude), Linux mounts a tmpfs over the denied directory and then binds the convenience directory back over it read-write, so its host contents are readable again inside the sandbox and writes land on the host. On macOS the directory stays writable under the read deny.Fix
getDefaultWritePaths({ denyRead, allowRead })leaves a home convenience directory out when adenyReadentry names it or a directory above it. The explicit deny wins over the implicit allow; a caller who wants the directory writable lists it inallowWrite./devnodes and/tmp/claudeare kept whatever is read-denied, sodenyRead: ['/']withallowReadcarve-outs behaves as before.allowReadentry beneath the covering deny keeps the directory. WithdenyRead: ['~'], allowRead: ['~/.claude']the caller has already made it readable, nothing is bound back over a deny, and the write allow stays.dir/**counts asdir; a glob covers a directory when it matches that directory or one above it (the samedenyGlobRegexthe macOS profile is built from, now shared). Nothing is listed from disk and no pattern is compiled from a file name. On Linux, where the backend expands globs, a glob that matches nothing on disk still counts (this only ever drops a convenience path), and a glob whose match is a symlink to one of these directories is not seen. A globallowReadentry is not counted as re-opening anything.getFsWriteConfig()stays a plain getter. It andwrapWithSandbox()go through one helper that adds the credential file denies from the puregetCredentialDenyReadPaths(): no credential masking, no glob expansion, no new way to throw. The zero-argumentgetDefaultWritePaths()is the constant list and touches neither the cwd nor the disk.normalizePathForSandbox()reads the cwd only for a relative path.No change for a config that does not read-deny one of the two directories. Not behind a flag. The README gains a sentence on when the two directories are dropped.
Not addressed, deliberately: a deny spelled in different case than the on-disk name on a case-insensitive volume does not cover the directory (Seatbelt matches case-sensitively, so this is consistent with what is enforced), and Windows enforcement never used the default write paths.
Testing
bun test test/sandbox/default-write-paths.test.ts(23 cases on Linux):<home>,~/,~/**,<home>/**,/,/**), a glob over the parent directory, siblings that must not match, an entry whose brackets are not a valid class, and theallowReadcases;HOME, where both directories exist:getFsWriteConfig()and the wrapped command for a filesystem deny, a credential file deny and a per-callallowRead; and a covering deny when~/.npm/_logsdoes not exist yet (on macOS thatHOMEis under/var, which normalizes to/private/var);backup[2024-01-15].envunder adenyReadglob and a credential mask withonExtractNoMatch: "error"leavegetFsWriteConfig()and the wrap alone.Removing any one of the call-site arguments, the credential denies, the regex fallback or the
allowReadexemption turns the file red.Run end to end on Linux with the built CLI under real bubblewrap and a
HOMEholding host content in both directories: withdenyRead: ['~/.npm']main prints the host log and its write lands on the host; this branch prints "No such file" and the write stays on the tmpfs.~/**,~/.n*, a credential deny on~/.claude,denyRead: ['/']with carve-outs (/dev/nulland/tmp/claudewritable),updateConfig()and a per-call override behave the same way. The macOS profile output is covered by the macOS CI legs only.Full suite on Linux: 1076 pass, 8 fail, 230 skip; the 8 failures are the same 8 that fail on main on that machine (they need the seccomp helper binary built).