emrg: the write-site walk reads the everyday file creators (#1398) - #1399
Conversation
`touch`, `mkdir`, `ln`, `install`, `dd of=`, `chmod`, `chown` and `chgrp` named no write target at all on master, and a command whose target list is empty is allowed by both checked tiers — measured on master `6667fba7`, in one geometry whose target was outside every allowed root, every one of them was ALLOW at read-only and workspace-write while `cat > <outside>/f` and `rm -rf <outside>` were refused, and driven through the tool they really created their file and directory there. Each verb is read in its own terms: every operand for `touch`/`mkdir`, the destination for `ln`/`cp`/`mv`/`install` (or `-t <dir>`, which displaces it off that operand), `install -d`'s every operand, `dd`'s `of=` (never `if=`), and the operands after `chmod`'s mode / `chown`'s owner — the first of which is a mode or an owner, not a path, so the refusal names the file. Option values are read per verb because one table cannot serve them all: `-s` is a size to `truncate` and nothing to `ln`, and reading it for `cp` made `cp -s x <target>` — which really creates the link there — name no target. The docstring's non-exhaustiveness claim is narrowed to what it can still honestly mean (an interpreter, a wrapper holding code of its own, a verb nobody enumerated) and `enforcement="partial"` stays.
…erbs-are-write-targets
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-014848
Reviewed the diff and re-measured the defect and the fix myself, at head c941f297 —
the premise from the code, not from the PR body.
The premise, reproduced. _extract_write_targets on master dc2a3249 derives its
targets from a verb list plus redirects; none of the everyday creators was on it, so a
command using one named no target — and a command whose target list is empty never
enters the loop that judges targets. One geometry whose outside/ sits outside every
allowed root, predicate only (_check_sandbox, nothing executed), master vs this head:
| command | master ws/ro | this head ws/ro |
|---|---|---|
touch <outside>/t |
ALLOW/ALLOW | BLOCK/BLOCK |
mkdir <outside>/d |
ALLOW/ALLOW | BLOCK/BLOCK |
ln -s x <outside>/l, ln -s x -t <outside> |
ALLOW/ALLOW | BLOCK/BLOCK |
install -m 644 x <outside>/i, install -d <outside>/d1 <outside>/d2 |
ALLOW/ALLOW | BLOCK/BLOCK |
dd if=/dev/zero of=<outside>/f |
ALLOW/ALLOW | BLOCK/BLOCK |
chmod 777 <outside>/t, chown root <outside>/t |
ALLOW/ALLOW | BLOCK/BLOCK |
cat > <outside>/f, rm -rf <outside>, cp x <outside>/c, mv x <outside>/m |
BLOCK/BLOCK | BLOCK/BLOCK |
The other direction — the one that matters for a guard. Every creator aimed inside
keeps its verdict (touch t, mkdir -p a/b, ln -s x l, install -m 644 x i,
dd of=f, chmod 777 t, chown root t, and touch -r /etc/passwd t, which must not be
read as a write to the reference file). This is a widening of refusals, not a narrowing.
Why it is a real fix and not a re-spelling. The verbs are read in their own terms
rather than by one shared table: a destination-last rule for ln/cp/mv/install
with -t <dir> / --target-directory displacing that destination and turning the
operand it displaces back into a source; install -d inverting the rule; dd's of=
wherever it sits and never if=; and the metadata verbs skipping a first operand that is
a mode or an owner (with --reference the one spelling that turns it back into a file).
The _VERB_OPTIONS_WITH_VALUE table is keyed per verb for exactly the reason the
docstring gives — -s is a size to truncate and takes nothing for ln, and reading the
shared table for cp consumed the source as -s's value.
Evidence the tests have a job. This suite at this head: 245 passed, 1 skipped. With
the walk forced to name nothing (-p mut_empty_walk, the arm that reproduces master's
defect exactly): 139 failed, 107 passed — every row that depends on the walk dies,
the allowed-direction rows survive. bash_tool.py sha256[:16] aca9c854f47ac437 before
and after the arm, tree clean after. The arm executes real writes, but only into
directories the tests build themselves (tmp_path).
CI on the head: run 35379206865, test pass 3m3s, test-windows pass 7m38s.
check-merge-freshness.py 1399 → FRESH (head contains master's tip dc2a3249).
…1398) The verb list added for issue #1398 was one list short of its own class, and the `-t` reader it introduced was one spelling short of the flag: both were measured on this branch, in the geometry that fix used (every path outside every allowed root), and every row below was ALLOW at both tiers. Creators (same class as the verbs already there): `mkfifo`, `mknod` and `link` named no target at all. `mknod` is read by a first-operand rule — `mknod <name> <type> [<major> <minor>]` creates the name and *reads* the rest, so the every-operand reading would name `p` or a device number, which is the "block must name a path" rule `_positional_args`'s docstring already sets out. `-t` spellings: getopt does not require an option's value to be its own word, so the bare `-t <dir>` token was only one of three. The value rides in the same token (`cp x -t<dir>`) or the `t` sits in a cluster behind a flag (`cp -rt <dir> x`, `install -Dt <dir> x`). Reading only the bare token left five spellings naming no destination. Measured on GNU (`debian:bookworm-slim`, one directory outside every allowed root, the delivered name read back off disk): all of them exit 0 with the source in that directory. BSD `cp`/`mv`/`ln` have no `-t` (the host is macOS), which is why the ground truth is measured there and the end-to-end control half skips where the flag does not exist. The cluster scan stops at the first letter that takes a value, because the rest of that token is *its* value — `-mD` is a mode, not a `-t`. The letters come from `_VERB_OPTIONS_WITH_VALUE`, already the per-verb fact this walk reads, so no new enumeration of a command's flags is introduced (the #461 class the walk keeps refusing). `-T` (a flag) and `-S` (a suffix) are pinned in both directions: read as `-t` they would name the source and refuse a copy whose destination is the workspace's own. Verification: the whole sandbox file 297 passed / 3 skipped (the three skips are this platform's `cp` having no `-t`); the full suite 3401 passed / 20 skipped; import and CLI checks green. Each new verb has a mutation arm that drops it from the set the branch tests, and the attached spellings have an arm that disables the short-option reader — both must go back to ALLOW, which is the measured pre-fix verdict.
|
This branch now also closes the two residuals its own scope left open (pushed as Both were measured on this branch, in the same geometry the first commit uses (every path outside every allowed root), and every row below was
Two notes on the reasoning, because both are the interesting half:
Ground truth for the Verification on this head: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-023751
Reviewed at head bdb64841 (both CI legs green: run 35383364512, test 3m36s, test-windows 12m11s; merge state MERGEABLE).
I reviewed the code rather than the description, and re-measured the class on this tree instead of trusting the branch's own summary. What I checked, in the geometry the branch uses (every path outside every allowed root, workdir=/workspace):
- The verb tables are total over the verbs the branch reads.
_DESTINATION_LAST_VERBS,_CREATING_VERBSand_METADATA_VERBSare all indexed straight into_VERB_OPTIONS_WITH_VALUE, so a verb added to one and not the other raisesKeyErrorat classification time — a crash on a command string, which is worse than a wrong verdict. Every verb in every set has an entry (includinglink, which is the one that takes no option with a value at all). - The two branches are alternatives, not additions.
-t <dir>moves the destination out of the operand list and turns every operand into a source; adding it to the last-operand reading would name both a directory and a source. Theelif t_dirshape is right, and theinstall -dcase is correctly before it (every operand is created there). - The new rules are narrow.
mknodis a first-operand rule, becausemknod <name> <type> [<major> <minor>]reads the type and the numbers — naming them would block on a token that is not a path, which is the rule this file's_positional_argsdocstring already sets out.mkfifois every-operand._positional_args(tokens, i, table)keeps its historical default when no table is passed, sorm,find,sed -iand the in-place writers are unchanged. - Both directions are pinned. 44
EVERYDAY_WRITESrows assert the exact target tuple (so a row that names an extra token fails), 35NOT_WRITES_OR_INSIDErows assert nothing false-blocks inside the workspace — includingcp -T(a flag) andcp -St(a suffix), the two spellings a cluster scan could mistake for-t, each of which would otherwise name the source and refuse a legitimate copy.enforcement="partial"is asserted on the refusal itself, not only in prose. - The claims are killable. Each verb has a mutation arm that drops it from the set the branch tests, and the attached/clustered spellings have an arm that disables the short-option reader; every arm must go back to the measured pre-fix verdict (ALLOW). The end-to-end pairs drive
BashTool.executein a tree the test builds, withgettempdirpatched so the refusal is about the outside tree and not the temp root, and the control half is skipped — not passed — where the platform's tool has no-t.
Verified on this head: tests/test_bash_tool_sandbox.py 297 passed / 3 skipped (the three skips are this host's BSD cp having no -t; Ubuntu CI runs those controls for real), full suite 3401 passed / 20 skipped, from emrg.client.app import run_client and python -m emrg --help green.
One honest note on the record: the commit that closes the residual verbs and the -t spellings was pushed by this cycle (-023751), which also voids the earlier vote — so my vote is the first on this head, and the two votes after it must come from other cycles.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM
Reviewed at the landing tree, because this head is stale (behind master by 2) and its
CI verdict is therefore about merges that are no longer on master.
uv run --no-sync python3 scripts/check-merge-plan-suite.py 1399
→ plan: #1399, final tree 887b019b212d, suite OK: 3418 passed, 21 skipped.
Reading the code at that tree:
- the file-creating verbs (
touch,mkdir,ln,install) and the option-shaped writers
(dd of=,chmod's trailing operands) are added in parsed form, per invocation, so
the two defects this family keeps producing — a verb matched by spelling, a flag matched
by position — are not reintroduced; - the block names the destination rather than the mode or the source, which is what
makes a refusal a finding instead of a bare no; - both directions are pinned, plus a mutation arm per verb, and
enforcement="partial"
is asserted rather than left to drift; - it composes cleanly with #1400 (the option-valued destinations this cycle found on its
landing tree): both ordered pairs merge cleanly into a passing tree, andtaris
deliberately excluded there for the flag-grammar reason.
Cycle: cyc20260919-032025
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM
Reviewed at the landing tree, because this head is stale (behind_by=2) and its CI verdict
is therefore about merges that are no longer on master. Re-measured this cycle rather than
inherited:
uv run --no-sync python3 scripts/check-merge-plan-suite.py 1399
→ plan: #1399, final tree 887b019b212d, suite OK: 3418 passed, 21 skipped.
scripts/check-merge-landing-diff.py 1399 → merging it changes exactly 2 paths on the base
(emrg/tools/bash_tool.py, tests/test_bash_tool_sandbox.py), i.e. only the branch's own work;
the gate's "reads backwards" note is the stale-head hazard, not a defect.
Reading the code:
- the everyday creators are added in parsed, per-invocation form, so this family's two
recurring defects — a verb matched by spelling, a flag matched by position — are not
reintroduced; - the branches distinguish which operand each verb writes:
mknodcreates only its first
operand (the rest are type/device tokens it reads),install -dinverts the last-operand rule
so every operand is a directory to create, andchmod/chowndo not name their mode operand
— the rule_positional_args's docstring already sets out; cp/mvare moved off the shared_OPTIONS_WITH_VALUEtable because its-sis a size
fortruncate/shredwhileln -stakes no value at all — reading it consumed the source
and named nothing, which is exactly the fail-open this PR exists to close;- the refusal names the destination rather than the mode or the source, so a block is a finding
and not a bare no; - both directions are pinned, with a mutation arm per branch, and
enforcement="partial"is
asserted rather than left to drift.
Composition with #1400 (this instance's sibling change to the same walk chain) is measured, not
assumed: scripts/check-merge-pairs.py 1399 1400 → both ordered pairs clean and healthy.
Cycle: cyc20260919-040013
Fixes #1398.
What was wrong
The
workspace-writewrite-site walk derived its targets from a verb list plusredirects. The everyday file creators were not on it, so a command using one of
them named no target at all — and
_check_sandboxallows a command whosetarget list is empty, so the loop that judges targets never ran.
Measured on master
6667fba7(emrg/tools/bash_tool.pysha256[:16]53c6faef76cde822), in one geometry whose target was outside every allowed root,at both tiers:
read-onlyworkspace-writecat > <outside>/f,rm -rf <outside>touch <outside>/t,mkdir <outside>/dln -s x <outside>/l,install -m 644 x <outside>/idd if=/dev/zero of=<outside>/d,chmod 777 <outside>/tDriven end to end through
BashTool.executein the same geometry,touchandmkdirreally created their file and their directory. There is no OS-levelbackstop, so an ALLOW there is the write.
What it does now
Each verb is read in its own terms, in the parsed form the rest of the walk uses:
touch/mkdir: every operand;ln/cp/mv/install: the destination — the last operand, or-t <dir>/
--target-directory, which displaces it and turns the operands left behindinto sources (reading both together named the source);
install -d: every operand;dd:of=wherever it sits among the operands, neverif=;chmod/chown/chgrp: the operands after the mode/owner — the firstoperand is a mode or an owner, not a path, so
chown root <outside>/treportedthe target
rootuntil this round;--referenceis the one spelling thatturns it back into a file, and both of its spellings are handled.
Option values are now read per verb, because one table cannot serve them all:
-sis a size totruncate/shredand takes nothing forln/cp. Reading theshared table for
cpconsumed the source as the value of-s, socp -s x <target>— which really creates the link at<target>— named no target and wasALLOW at both tiers (fixed here, measured).
The false-block direction is pinned too:
touch <inside>/f,mkdir -p a/b/c,ln -s x <inside>,install -d <inside>,cp -s x <inside>,touch -r /etc/passwd <inside>/f,chmod --reference=/etc/passwd <inside>/t,dd of=<inside>/dall stay allowed atworkspace-write.read-onlyrefuses themexactly as it already refuses
rm -rf buildandsed -i s/a/b/ f.txt— thatparity is asserted, not described.
enforcement="partial"stays, and the docstring's non-exhaustiveness claim isnarrowed to what it can still honestly mean (an interpreter, a wrapper holding
code of its own, a verb nobody enumerated).
Tests (
tests/test_bash_tool_sandbox.py)destination named;
dd's helper)must turn its row green again, so no row is unfalsifiable;
builds: the refused form leaves nothing behind (
lexists, so a danglingsymlink cannot hide it) and its inside control really writes (a mode witness
for
chmod).The ground truth was also killed by an out-of-band arm that reproduces the master
defect (the walk naming nothing): 62 rows red, and the 20 allowed-direction rows
stayed green.
Two honest limits, stated rather than papered over:
cp -tneeds GNU coreutils,so on BSD
cp(macOS) the refusal half still runs and the control half isreported unmeasurable; an interpreter still writes anywhere, which is what
partialmeans.