Skip to content

sandbox: the write-target walk believes a verb wherever it stands, so a mention is refused — and its tokenizer lets a backticked writer through #1469

Description

@argszero

_extract_write_targets — the walk that decides which paths a command writes — visits every token and matches its word against the verb sets (rm, cp, touch, patch, sed, git, …). There is no command-position test. That is deliberate: it is how sudo rm … and find . -exec rm … are reached. The cost is measured below in both directions, and the same walk's tokenizer carries the other half.

All rows below are pure predicate calls on master 347f023e (_extract_write_targets / _check_sandbox at tier read-only). Nothing is executed.

A. A mention is read as a write

grep -n "patch" f.txt        -> BLOCK   targets ['f.txt']
grep -n "rm"    f.txt        -> BLOCK   targets ['f.txt']
grep -e mv      f.txt        -> BLOCK   targets ['f.txt']
echo rm -rf /tmp/x           -> BLOCK   targets ['/tmp/x']
echo touch /tmp/x            -> BLOCK   targets ['/tmp/x']
printf '%s\n' sed -i f.txt   -> BLOCK   targets ['f.txt']
cat mv.txt                   -> ALLOW
test -f patch                -> ALLOW

This is not hypothetical. Two commands refused during a real evolution cycle were exactly this shape:

  • grep -n "patch" <file> — refused for targeting '<file>';
  • $PY script.py 2>&1 ; echo "=== widened pin (from the patch) ===" — refused with targets [')', '===']. Here the wrapper reader (_unresolved_wrapper_payloads) turns everything after a variable reference into a command text, and inside one of those texts the word patch standing in prose named the ) two words after it.

Sweep of the verb set the walk dispatches on × the contexts a shell can put a word in — 26 verbs × 24 contexts = 624 rows — finds 104 refusals that this change removes, and every one of them is a data context (echo, printf, cat, test -f of a verb word).

B. A backticked writer is not seen

The walk tokenizes with _split_command_tokens, whose shlex keeps the default punctuation set ();<>|& — the backtick is not in it — and leaves \n in lex.whitespace, so a newline separator never becomes a token either. A backticked writer therefore arrives glued to its backtick (`rm is not rm) and names no target.

Measured, all 26 writer verbs behind a backtick are ALLOWED at read-only, while the same poisoned path in every other context is refused:

`rm -rf /tmp/x`          ALLOW        rm -rf /tmp/x            BLOCK
`touch /tmp/x`           ALLOW        sudo rm -rf /tmp/x       BLOCK
`rmdir /tmp/x`           ALLOW        env FOO=1 rm -rf /tmp/x  BLOCK
`cp a.txt /tmp/x`        ALLOW        FOO=1 rm -rf /tmp/x      BLOCK
`mv a.txt /tmp/x`        ALLOW        xargs rm -rf /tmp/x      BLOCK
`tee /tmp/x`             ALLOW        find . -exec rm … {} +   BLOCK
`sed -i s/a/b/ /tmp/x`   ALLOW        timeout 5 rm -rf /tmp/x  BLOCK
`patch -o /tmp/x a.txt`  ALLOW        if true; then rm …; fi   BLOCK
`gzip -f /tmp/x`         ALLOW        for f in a; do rm …; done BLOCK
`sort -o /tmp/x a.txt`   ALLOW        (rm -rf /tmp/x)          BLOCK
`curl -o /tmp/x http://…` ALLOW       ! rm -rf /tmp/x          BLOCK
`zip /tmp/x a.txt`       ALLOW        $(rm -rf /tmp/x)         BLOCK
… 26/26 ALLOW                         cd /tmp \n rm -rf /tmp/x BLOCK

The git-mutator walk was migrated to the separator-preserving tokenizer for exactly this reason, and the repository already pins that migration (tests/test_bash_tool_sandbox.py::test_command_substitution_is_not_a_polite_spelling, which records the same backtick measurement). The write-target walk was not migrated, so the hole is still open there.

The two halves hold each other up

_tokenize_command's docstring records that the write-target extractor "is not affected, and I checked rather than assumed: it walks tokens matching command words (rm, mv, tee) wherever they sit, so it needs no separator and already found rm -rf /tmp/a at the end of a newline-separated line". The second half of that sentence is exactly why the first half is true — a position-blind walk needs no separator, and the separator is gone from the stream it reads.

So the position question cannot be added on its own: asking it of the walk's own token stream fails the repository's own pin,

tests/test_command_position_contexts.py::test_an_escaped_backslash_before_a_writer_verb_is_seen
E   AssertionError: assert [] == ['f.txt']

because echo a\\ + newline + rm -f f.txt is two real commands and the newline never reaches the walk. The change has to be the pair: ask the question, and read the stream that can answer it.

The change, measured both ways

One change, two textual halves: the walk asks _runs_as_a_command before believing a verb spelling, and it tokenizes with _tokenize_command instead of _split_command_tokens.

arm false blocks (12 read rows, want 0) real writers (33 rows, want all) pinned row backtick rows full suite
master 347f023e 6 33 pass 26 ALLOW 38 failures
gate only 0 33 FAIL 26 ALLOW 39 (one flip)
gate + separator-preserving tokenizer 0 33 pass 26 BLOCK 38 — identical to a pristine twin
mutation arm: gate replaced by i == 0 0 10

The mutation arm matters: the obvious gate — "only the first word can be an invocation", the one _runs_as_a_command's own docstring warns about — loses 23 of the 33 real writers, which is what makes the battery able to see a broken gate at all.

Sweep, 624 rows, diffed arm to arm:

  • refusals removed: 104, all data contexts;
  • refusals added: 26, all the backtick context;
  • zero changed rows in any invocation context (bare, ;, &&, |, newline, sudo, env, FOO=1, xargs, find -exec, if/then, do, ( ), { }, !, $( ), bash -c).

Full suite on an isolated tree: 38 failures on a pristine twin and 38 on the fixed tree, the same set — no regression (the 38 are repo-shape artifacts of the isolated tree). tests/test_command_position_contexts.py gains three rows; two of them fail on master and pass on the fixed tree, the third is the complement direction (a change that refused nothing must not pass).

Not addressed here

patch) is still refused for targeting ')'. It is a shell syntax error (syntax error near unexpected token ')' in /bin/sh and /bin/bash), so the refusal is fail-closed and costs nothing — but the message names a target for a command that cannot run, which is misleading rather than wrong.

Relationship to other open scanner issues

Distinct mechanism from #1464 (a word a spaced option ate), #1466 (the heredoc mask under a wrapper), #1467 (an expansion token in argument position) and #1468 (an fd-prefixed redirect read as an operand): this one reproduces identically on master and on the #1468 branch, so it shares no code path with them.

The change, as a diff against master (both directions verified: the patch rebuilds the target byte-for-byte and reverses to the base)
--- a/emrg/tools/bash_tool.py
+++ b/emrg/tools/bash_tool.py
@@ -2634,6 +2634,24 @@
         )
     out.extend(_patch_directory_values(tokens, i))
     return out
+
+
+# Every word the write-target walk below dispatches on as a verb. It exists so the
+# shell's own question can be asked *once*, before a verb spelling is believed: a
+# word that is a verb only in spelling, standing where the shell passes it as data,
+# is not an invocation and names no target (`_runs_as_a_command`).
+_WRITE_VERB_WORDS: frozenset[str] = frozenset().union(
+    _REMOVER_VERBS,
+    _CREATING_VERBS,
+    _DESTINATION_LAST_VERBS,
+    _METADATA_VERBS,
+    _INPLACE_WRITER_VERBS,
+    _COMPRESSOR_VERBS,
+    _LZ4_VERBS,
+    _PZSTD_VERBS,
+    _OPTION_DESTINATION_VERBS,
+    {"git", "rsync", "split", "dd", "patch", "sed", "perl", "find", "csplit", "zip"},
+)
 
 
 def _extract_write_targets(cmd: str, _depth: int = 0) -> list[str]:
@@ -2747,7 +2765,11 @@
     delimiter word as write targets (`_mask_data_heredoc_bodies`).
     """
     masked = _mask_data_heredoc_bodies(cmd)
-    tokens = _split_command_tokens(masked)
+    # The *separator-preserving* tokenizer (see its docstring): the walk below asks a
+    # position question now, and `_split_command_tokens` drops a newline separator, so
+    # `echo a\\` + newline + `rm -f f` would answer "no separator" about a stream that
+    # lost the one the shell acts on.
+    tokens = _tokenize_command(masked)
     # A quoted operator is not an operator: `'>'` dequotes to `>`, so the token
     # stream alone cannot say which one the shell will act on. `is_operator` is
     # the one question — shape *and* not quoted — asked wherever the walk needs
@@ -2823,6 +2845,17 @@
                     targets.append(tokens[j])
                 i = j + 1
                 continue
+        elif word in _WRITE_VERB_WORDS and not _runs_as_a_command(tokens, i):
+            # A verb *spelling* is not an invocation. The walk below visits every
+            # token and matches its word against the verb sets wherever it stands,
+            # which is what reaches `sudo rm` and `find . -exec rm`; the cost was
+            # that a verb word the shell passes as *data* was believed too.
+            # `_runs_as_a_command` is the guard's own rule for the difference and
+            # is already the question the git-mutator scan asks: a separator, a
+            # grouping operator, `!`, a shell keyword, a wrapper prefix or a
+            # `VAR=value` puts a word in command position; anything else is an
+            # argument of whatever the command really is.
+            pass
         elif word in _REMOVER_VERBS:
             # Any operand is removed — NOT only with a recursive flag.
             # `rm a.txt` destroys uncommitted work exactly like `rm -rf dir`;
--- a/tests/test_command_position_contexts.py
+++ b/tests/test_command_position_contexts.py
@@ -414,3 +414,79 @@
         "grep -n ')' x.txt",
     ):
         assert _reads(cmd) is True, cmd
+
+
+# ── the same two directions at the *write-target* walk (#14xx) ────────────
+
+WRITE_MENTIONS = [
+    'grep -n "rm" f.txt',
+    'grep -n "patch" f.txt',
+    'grep -e mv f.txt',
+    "echo rm -rf /tmp/x",
+    "echo touch /tmp/x",
+    "printf '%s\\n' sed -i f.txt",
+    "cat mv.txt",
+    "test -f patch",
+]
+
+WRITE_INVOCATIONS = [
+    "rm -rf /tmp/x",
+    "sudo rm -rf /tmp/x",
+    "env FOO=1 rm -rf /tmp/x",
+    "FOO=1 rm -rf /tmp/x",
+    "xargs rm -rf /tmp/x",
+    "find . -exec rm -rf /tmp/x {} +",
+    "timeout 5 rm -rf /tmp/x",
+    "nice -n 5 rm -rf /tmp/x",
+    "if true; then rm -rf /tmp/x; fi",
+    "for f in a; do rm -rf /tmp/x; done",
+    "(rm -rf /tmp/x)",
+    "! rm -rf /tmp/x",
+    "cd /tmp\nrm -rf /tmp/x",
+]
+
+
+def test_a_writer_verb_in_argument_position_is_data_not_an_invocation():
+    """`grep -n "rm" f.txt` searches for the word; it does not run `rm`.
+
+    The walk visits every token and matched its word against the verb sets wherever it
+    stood, which is what reaches `sudo rm` and `find -exec rm` — and also what read the
+    word as an invocation when the shell passes it as an argument. `_runs_as_a_command`
+    is the guard's own rule for the difference, and it is the question the git-mutator
+    scan already asks.
+    """
+    for cmd in WRITE_MENTIONS:
+        assert _reads(cmd) is True, f"{cmd!r} mentions a verb and writes nothing"
+        assert _extract_write_targets(cmd) == [], cmd
+
+
+def test_every_context_that_runs_a_writer_still_names_it():
+    """The complement: a change that refused nothing would pass the test above."""
+    for cmd in WRITE_INVOCATIONS:
+        assert _reads(cmd) is False, f"{cmd!r} really runs the verb"
+        assert _extract_write_targets(cmd), cmd
+
+
+def test_a_backticked_writer_is_seen_by_the_write_target_walk():
+    """The other tokenizer's backtick half, at the *second* site (#14xx).
+
+    `_tokenize_command` was given `\\n` and the backtick in its punctuation set (issue
+    #1156, #1233), and the write-target walk kept `_split_command_tokens`, whose `shlex`
+    punctuation defaults to `();<>|&`. A backticked writer therefore stayed glued to its
+    backtick — `` `rm `` is not `rm` — and named no target: measured, every one of 26
+    writer verbs behind a backtick was ALLOWED at `read-only` while `$( ... )`, `( ... )`,
+    `if`, `do`, a pipe and a bare newline all named the same path. The walk now reads the
+    separator-preserving tokenizer, which is what the position question needs.
+    """
+    for cmd in (
+        "`rm -rf /tmp/x`",
+        "`touch /tmp/x`",
+        "`tee /tmp/x`",
+        "`sed -i s/a/b/ /tmp/x`",
+    ):
+        assert _reads(cmd) is False, f"{cmd!r} runs the verb"
+    # The complement, at the same site: a backticked read stays allowed.
+    assert _reads("`cat f.txt`") is True
+    assert _reads("echo `date`") is True
+
+

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions