From de51b8afae0573c44e0c9e9b4fae33b35d6d522d Mon Sep 17 00:00:00 2001 From: Zejiang Guo Date: Sat, 24 Oct 2026 23:12:04 +0000 Subject: [PATCH] fix(security): rm floor reads reserved words, empty substitutions and ${...} spans The recursive-force rm floor now refuses a wipe of / or ~ behind then, do, else, {, !, or time; with a quoted substitution that expands to nothing in the program word or a target; and after a # or << inside an unpaired ${...} or $[...]. An unclosed span is also read as if it were not there. Deny-only. --- docs/system-specs/modules/security.md | 2 +- src/kiro_crew/security/rm_floor.py | 39 +++++++++---- test/test_denied_commands_security_rows.py | 68 ++++++++++++++++++++++ test/test_security_gate_liveness.py | 8 ++- 4 files changed, 105 insertions(+), 12 deletions(-) diff --git a/docs/system-specs/modules/security.md b/docs/system-specs/modules/security.md index e96b26a75b8..cf6b070fac4 100644 --- a/docs/system-specs/modules/security.md +++ b/docs/system-specs/modules/security.md @@ -1659,7 +1659,7 @@ command. Local assignments remain available across the preserved separators. The existing deny catalog, nested-payload checks and real self-kill checks continue to apply. -**Recursive-force `rm` floor (`rm_floor.py`)**: a small seatbelt, not a parser. The two `local-destructive-rm-rf-*` rows match one contiguous spelling; this also refuses the common respellings. It cuts the text where the shared `_iter_shell_chars` marks `;` `&` `|` newline backtick or paren live (so quoted text stays one word) skips a `#` comment to its newline and restarts unquoted there, reads a `$(…)` or a backtick body inside double quotes on its own from an unquoted start (the `$(…)` span from the shared `_matching_close_paren`, the backtick span up to the next unescaped backtick) and resumes the double quote after it, starts a new word at a live `<` or `>` (so a redirection glued to an operand is its own word), keeps an escaped or single-quoted `$` and any quoted `~` literal, and stops at a live `<<` (heredoc bodies are data), reads each segment's words with `_shell_tokens` (quotes off, nothing expanded), skips leading `VAR=val` words, and denies when the program basename is `rm`, the flags hold recursive (`r`, `R`, `--recursive`) AND force (`f`, `--force`), and an operand is exactly root (`/`, `//`, `/*`) or home (`~`, `$HOME`, `${HOME}`, any quoting, with an optional trailing `/` or `/*`). It runs only when the text holds a literal `rm` (quote splices allowed); a floor error fails closed with `_RM_FLOOR_UNREAD_NOTE` for a command that names `rm` as a word of its own (`_names_rm_word`: not inside a longer word such as `format`, checked on the raw text and with `${X:-…}` defaults resolved), and lets any other command pass. A refusal names the row and carries the `argv-floor` component. It runs only after every regex view cleared, and each target only while its row is enabled. Out of scope by design, left to the rows and the OS sandbox: descendants (`rm -fr /tmp/x` and `rm -rf $HOME/.cache` stay allowed), heredocs, arrays, arithmetic, `sudo` and other wrappers, `bash -c` payloads and parameter-expansion defaults. Some contrived shapes are over-refused (a variable named `rm`, a quoted `'/*'`); that is accepted rather than read further. +**Recursive-force `rm` floor (`rm_floor.py`)**: a small seatbelt, not a parser. The two `local-destructive-rm-rf-*` rows match one contiguous spelling; this also refuses the common respellings. It cuts the text where the shared `_iter_shell_chars` marks `;` `&` `|` newline backtick or paren live (so quoted text stays one word) skips a `#` comment to its newline and restarts unquoted there, reads a `$(…)` or a backtick body inside double quotes on its own from an unquoted start (the `$(…)` span from the shared `_matching_close_paren`, the backtick span up to the next unescaped backtick) and resumes the double quote after it, starts a new word at a live `<` or `>` (so a redirection glued to an operand is its own word), keeps an escaped or single-quoted `$` and any quoted `~` literal, and stops at a live `<<` (heredoc bodies are data). Inside an open `${…}` or `$[…]` (opened by an odd run of live `$`, so `$${` is the PID and opens nothing) a `#` or `<<` is data rather than a comment or heredoc; because that tracking can misread a span, every command is also read once without it and the targets of both readings are joined, so the tracked reading can only add refusals, and each reading passes its mode to the nested substitution bodies so the work stays linear in nesting depth. It reads each segment's words with `_shell_tokens` (quotes off, nothing expanded), skips leading `VAR=val` words, the reserved words that keep command position (`shell_normalizer._KEEPS_COMMAND_POSITION`: `then`, `do`, `else`, `{`, `!`, `time` and its `-p`, and the rest) and a leading redirection with its target, drops a substitution that sits in the program word because it may expand to nothing (a substitution inside an operand stays part of that path), and denies when the program basename is `rm`, the flags hold recursive (`r`, `R`, `--recursive`) AND force (`f`, `--force`), and an operand is exactly root (`/`, `//`, `/*`) or home (`~`, `$HOME`, `${HOME}`, any quoting, with an optional trailing `/` or `/*`). It runs only when the text holds a literal `rm` (quote splices allowed); a floor error fails closed with `_RM_FLOOR_UNREAD_NOTE` for a command that names `rm` as a word of its own (`_names_rm_word`: not inside a longer word such as `format`, checked on the raw text and with `${X:-…}` defaults resolved), and lets any other command pass. A refusal names the row and carries the `argv-floor` component. It runs only after every regex view cleared, and each target only while its row is enabled. Out of scope by design, left to the rows and the OS sandbox: descendants (`rm -fr /tmp/x` and `rm -rf $HOME/.cache` stay allowed), heredocs, arrays, evaluating arithmetic or parameter-expansion defaults (their spans are only tracked as above), an unquoted `$(…)` or backtick inside the argv, `sudo` and other wrappers, and `bash -c` payloads. Some contrived shapes are over-refused (a variable named `rm`, a quoted `'/*'`, a quoted keyword such as `"then" rm -fr ~`, an `rm -fr ~` written inside a `${x:-…}` default); that is accepted rather than read further. **Quote-normalized segment view (`_deny_segment_views` + `_shell_tokens`, `security/`)** — both deny tiers match TEXT, while a shell strips quoting, de-escapes backslashes, resolves ANSI-C / locale quoting, collapses empty-string splices and collapses whitespace runs before the program ever sees its argv. A rule authored as a command SHAPE would therefore be defeated by re-spelling any single token: `rm -rf "/"`, `"rm" -rf /`, `rm "-rf" /`, `r''m -rf /`, `rm -rf /`, `rm -rf \/` and `rm -rf $'/'` all run exactly what `rm -rf /` runs, and none of them *contains* the rule's own text. Only the self-protection floor rules (`_SELF_PROTECTION_FLOOR_RULE_IDS`), git-publish and the two recursive-force `rm` rows (below) have an argv-structural floor closing this; the rest of the built-in catalog — including every destructive-operation and credential-exfiltration rule listed in this section — is spelling-dependent, so Pass 2 evaluates each segment in **two views**: the raw text first (byte-identical to what it matched before), then a quote/escape-normalized re-join, appended **only when it differs**, so an unquoted command pays no second pass over the catalog. Four properties are load-bearing: diff --git a/src/kiro_crew/security/rm_floor.py b/src/kiro_crew/security/rm_floor.py index 1cd97c940bb..8f0edc94f62 100644 --- a/src/kiro_crew/security/rm_floor.py +++ b/src/kiro_crew/security/rm_floor.py @@ -36,6 +36,7 @@ #: :func:`_segments` keeps a redirection and its fd prefix in one word (below). _HOME_WORDS = frozenset({"~", "$HOME", "${HOME}"}) _CUT_CHARS = frozenset(";&|\n`()") +_SUBST = "\1" # a substitution: may expand to nothing, unlike a literal ``\0`` #: A STANDALONE fd prefix (a bare number or ``{name}``) directly before a redirection #: operator: ``2>`` and ``{fd}>`` redirect fd 2 / a named fd, but the ``2`` in #: ``/tmp/2>x`` is part of the path, so the prefix counts only at a word boundary. @@ -75,18 +76,24 @@ def _target_kind(word: str) -> "str | None": return "home" if base in _HOME_WORDS else None -def _segments(command: str) -> "list[str]": +def _segments(command: str, track: bool = True) -> "list[str]": """*command* cut at separators the shared quote machine marks live; stops at a heredoc.""" out: list[str] = [] seg: list[str] = [] in_redirect = False + #: closers of open ``${…}`` / ``$[…]``: a ``#`` or ``<<`` there is data, so + #: neither a comment nor a heredoc starts until they close + expansion: list[str] = [] pos, state = 0, 0 while pos < len(command): text, resume, resume_state = command[pos:], len(command), 0 + dollars = 0 # live ``$`` run ending here: ``$$`` is the PID, never an opener for step in _shell._iter_shell_chars(text, state): - if step.active and text.startswith("<<", step.offset): + dollars = dollars + 1 if step.active and step.char == "$" else 0 + live_syntax = step.active and not expansion + if live_syntax and text.startswith("<<", step.offset): break # heredoc bodies are data; the rows and the sandbox own them - if step.active and step.char == "#" and _shell._opens_comment(text, step.offset): + if live_syntax and step.char == "#" and _shell._opens_comment(text, step.offset): newline = text.find("\n", step.offset) # a comment's quotes open nothing resume = len(command) if newline < 0 else pos + newline break @@ -94,8 +101,8 @@ def _segments(command: str) -> "list[str]": # bash reads "$(…)" from an unquoted start, then the double quote resumes body = text[step.offset + 2 :] close, proven = _shell._matching_close_paren(body, 0) - out.extend(_segments(body[: close - 1 if proven else close])) - seg.append("\0") + out.extend(_segments(body[: close - 1 if proven else close], track)) + seg.append(_SUBST) in_redirect = False resume, resume_state = pos + step.offset + 2 + close, 2 break @@ -104,11 +111,15 @@ def _segments(command: str) -> "list[str]": closer = _shell._backtick_closer(text, step.offset + 1) closed = closer != -1 end = closer + 1 if closed else len(text) - out.extend(_segments(text[step.offset + 1 : end - closed])) - seg.append("\0") + out.extend(_segments(text[step.offset + 1 : end - closed], track)) + seg.append(_SUBST) in_redirect = False resume, resume_state = pos + end, 2 break + if track and dollars % 2 and text.startswith(("${", "$["), step.offset): + expansion.append("}" if text[step.offset + 1] == "{" else "]") + elif step.active and expansion and step.char == expansion[-1]: + expansion.pop() if step.active and in_redirect and step.char == "&": seg.append(step.text) # fd-dup operator (``2>&1``), not a command separator elif step.active and step.char in _CUT_CHARS: @@ -138,11 +149,14 @@ def _segments(command: str) -> "list[str]": def _rm_wipe_targets(command: str) -> "set[str]": """The targets (``root`` / ``home``) a recursive-force ``rm`` in *command* wipes.""" kinds: set[str] = set() - for segment in _segments(command): + # read with and without ``${…}`` tracking: the untracked reading is the + # base one, so tracking can only add refusals + for segment in [*_segments(command), *_segments(command, track=False)]: words = _shell._shell_tokens(segment) while words: - if ENV_ASSIGNMENT_RE.match(words[0]): - words = words[1:] + if ENV_ASSIGNMENT_RE.match(words[0]) or words[0] in _shell._KEEPS_COMMAND_POSITION: + # ``then`` / ``do`` / ``!`` / ``time`` … leave the next word the program + words = words[2:] if words[0] == "time" and words[1:2] == ["-p"] else words[1:] continue is_redirection, consumes_next = _shell._push_token_redirection(words[0]) if is_redirection: @@ -152,6 +166,11 @@ def _rm_wipe_targets(command: str) -> "set[str]": if consumes_next and words: words = words[1:] continue + if _SUBST in words[0]: + # a substitution in command position may expand to nothing + bare = words[0].replace(_SUBST, "") + words = [bare, *words[1:]] if bare else words[1:] + continue break if not words or _shell._program_basename(words[0]) != "rm": continue diff --git a/test/test_denied_commands_security_rows.py b/test/test_denied_commands_security_rows.py index 6772a7d5e4f..2003db93109 100644 --- a/test/test_denied_commands_security_rows.py +++ b/test/test_denied_commands_security_rows.py @@ -539,6 +539,47 @@ class TestRecursiveForceRmFloor: "> /dev/null RM -fr /", "X=1 Y=2 >log RM -rf /", "&>/dev/null RM -fr /", + # A reserved word or ``!`` / ``time`` leaves the next word the program. + "if :; then RM -fr ~; fi", + "if false; then :; else RM -fr ~; fi", + "for i in 1; do RM -fr ~; done", + "while :; do RM -fr /; done", + "{ RM -fr ~; }", + "! RM -fr ~", + "time RM -fr ~", + "time -p RM -fr /", + "! time RM -fr ~", + "case x in x) RM -fr /;; esac", + "echo ${x:-$(RM -fr ~)}", + # A substitution in command position may expand to nothing. + "$(true)RM -fr ~", + "$(true) RM -fr ~", + "echo hi | `true`RM -fr /", + '"$(true)"RM -fr ~', + # An unquoted backtick still cuts the argv, as on base. + "RM -fr ~/`true`", + # A leading redirection keeps its substitution target. + ">$(mktemp) RM -fr ~", + '>"$(mktemp)" RM -fr ~', + "2>`mktemp` RM -fr /", + "<$(x) RM -fr ~", + "2>$(printf /dev/null) RM -fr ~", + # ``#`` and ``<<`` inside ``${…}`` / ``$[…]`` are data, not a comment or heredoc. + "echo ${x:- #}; RM -fr /", + "echo ${x:- </dev/null", ">> log RM -rf ./build", + # A keyword as an argument, or a substitution operand, is not a root/home target. + "echo then RM -fr ~", + "RM -fr $(mktemp -d)", + "echo ${a[1]} # RM -fr ~", + "echo $((1 << 2)) # RM -fr ~", + # A substitution inside a target is a real path: cache clears stay allowed. + 'RM -rf "$(pip cache dir)"/*', + 'RM -rf "$(go env GOCACHE)"/*', + 'cd build && RM -rf "$(pwd)"/*', + 'RM -rf "$HOME/$(basename "$PWD")"', ], ) def test_descendants_and_partial_flags_stay_allowed(self, command): assert is_denied(command.replace("RM", "rm")) is None + def test_nested_open_expansions_are_read_a_bounded_number_of_times(self, monkeypatch): + """Twenty ``"$( ${`` levels cost a linear number of walks, not 2**20.""" + from kiro_crew.security import rm_floor + + calls = 0 + real = rm_floor._segments + + def _counted(*args, **kwargs): + nonlocal calls + calls += 1 + return real(*args, **kwargs) + + monkeypatch.setattr(rm_floor, "_segments", _counted) + command = 'echo "$( ${ ' * 20 + "rm -fr /tmp/x" + ' )"' * 20 + rm_floor._rm_wipe_targets(command) + assert calls <= 4 * 20 + 2 + def test_the_floor_reports_the_catalog_row(self): reason = is_denied("rm -fr /") assert reason is not None diff --git a/test/test_security_gate_liveness.py b/test/test_security_gate_liveness.py index b755617106e..9e64c33933a 100644 --- a/test/test_security_gate_liveness.py +++ b/test/test_security_gate_liveness.py @@ -526,7 +526,13 @@ def _url_payload_command(n: int) -> str: #: ``paths``: one import and one ``or`` clause on the write gate, plus the blank line #: isort puts after the import. The matcher lives outside the package #: (``kiro_crew.edition_install_fence``). -_PACKAGE_LINE_BUDGET = 32_616 +#: +#: Raised by 19, from 32,616, for the ``rm`` floor's own segment walk: reserved +#: words (``then``, ``do``, ``!``, ``time`` …) keep command position, a quoted +#: substitution may expand to nothing in the program word, and +#: ``#`` / ``<<`` inside an unpaired ``${…}`` / ``$[…]`` open no comment or heredoc. +#: Each command is read with and without that tracking. Deny-only: no allow widened. +_PACKAGE_LINE_BUDGET = 32_635 #: Ceiling on any ONE file in the package. This is what the bound is really for -- #: a package total says nothing about a single file growing back into a second