diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b7e837..ab7ddca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **The Bash credential guard read only the directory name, so a search by + filename walked straight past it.** Rules 1 and 2 both ask whether the + command spells some form of `~/.mureo`. A tree search does not have to: + `find ~ -name credentials.json -exec cat {} \;` and + `find ~ -path '*mureo*' -exec cat {} \;` both printed the credentials + file, with no obfuscation and no adversarial intent required. "Look for + any leftover credential files under my home directory" is an ordinary + instruction, and the accident it causes is the one this guard exists to + make less likely. + + Two rules close it. Rule 3 denies a glob metacharacter standing + immediately before the written-out `mureo`; it reads the raw command + text as well as the normalized readings, because the quotes in `-path + '*mureo*'` exist to keep the shell off the pattern so `find` can expand + it, and normalization — which models the shell — correctly erases the + very metacharacter that makes it dangerous. Rule 4 denies the protected + filenames where they stand on their own, with no `/` before them: a name + with a path in front of it is a specific file, not a search, and rules 1 + to 3 have already judged it from the directory. + + Two costs, stated rather than hidden. `config.json` is deliberately not + guarded by name — it is one of the most common filenames in software and + denying it would block real work in every project — so + `find ~ -name config.json` still reads that one file. And a bare + `cat credentials.json` in a project of your own now denies; the reason + says so and points at the Read tool, which is guarded by path and opens + a same-named file anywhere outside `~/.mureo`. The module docstring's + known-open-bypass list gains both, plus the symlink asymmetry between + the two guards: the path guard resolves symlinks in each direction, the + Bash guard never touches the filesystem and cannot. + ### Added - **Delivery-collapse detection and diagnosis, across all platforms** (#546). diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index a1640ba..95247c2 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -25,7 +25,9 @@ cover every file in the directory, not just ``credentials.json``. * Bash guard: normalizes the command *once* into the text a shell would read after quoting, line continuations and expansions are resolved, and - applies two rules to that one string. Either one denies. + applies four rules to it. Any one of them denies. Rules 1 and 2 read + the directory name, rules 3 and 4 the two things a search that never + spells the directory does write down. The single reading is the load-bearing part, and it was learned the expensive way. Earlier versions had one rule scanning the raw command @@ -39,6 +41,19 @@ preserve it — which is what ``_COLLAPSE`` does for expansion boundaries — rather than the rule reaching for a different string. + Rule 3 reads the raw text too, and that is not the thing this paragraph + forbids — read this before adding another rule that does the same, + because the difference is the whole point. The split-brain bug was + *partition*: each rule owned one string and was blind to the other, so + an obfuscation resolved on one axis walked past the rule that owned the + other. Rule 3 is a *union* — it runs against the readings AND the raw + command, so nothing is invisible to it and no fold can open a hole + underneath it. It also does not want anything the fold destroys: it + looks for a pattern that a program other than the shell will expand, + and the fold models the shell alone, so there is nothing for + ``_COLLAPSE`` to preserve on its behalf. A rule reading the raw text + *instead of* the readings would be the old bug returning. + Rule 1 (the name spelled out) denies when the normalized text contains ``.mureo`` where a path component could *start*. Anchoring on the directory name rather than on ``credentials`` also covers a wildcard @@ -104,6 +119,61 @@ Without that restriction the rule would have to deny every glob anyone types, ``fnmatch('.mureo', '*')`` being true. + Rules 1 and 2 both read the *directory* name, and for a long time that + was all the guard read. It meant a command that never spelled the + directory at all walked straight past: ``find ~ -path '*mureo*' -exec + cat {} ;`` and ``find ~ -name credentials.json -exec cat {} ;`` both + printed the credentials, with no obfuscation and no adversarial intent + required. "Look for any leftover credential files under my home + directory" is an ordinary instruction, and it is exactly the accident + this guard exists for. Rules 3 and 4 read the two things such a command + does write down. + + Rule 3 (a pattern reaching into the name without the dot). Rule 2 only + considers components that begin with a literal ``.``, so ``*mureo*`` — + which ``find -path`` happily matches against the full path, leading + period included — was not a candidate. Rule 3 denies when a glob + metacharacter stands immediately before the literal ``mureo``. It is + deliberately narrower than "any pattern that could match": ``mureo`` has + to be written out, so working inside a checkout of this very repository + (``grep -r foo mureo/``) is untouched, while ``-path '*mureo*'`` and + ``-name '*mureo*'`` are not. + + Rule 3 reads the raw command text as well as the normalized readings, + and that is the point of it. Normalization models what the *shell* + expands, so it neutralizes a quoted ``*`` — correctly, for the shell. + But the quotes in ``-path '*mureo*'`` are there precisely to keep the + shell off the pattern so that ``find`` can expand it itself, and by the + time the normalized reading exists the pattern has become ``=mureo=`` + and there is nothing left to match. A pattern meant for a downstream + program is written literally in the command; that is where rule 3 looks + for it. Every other rule stays on the normalized readings, because + every other rule is about what the shell will do. + + Rule 4 (the protected filenames). A tree search can name the file + instead of the directory, so the filenames are candidates in their own + right — but only where the name stands on its own, with no ``/`` before + it. That restriction is the rule. A name with a path in front of it is + not a search but a specific file, and which file it is has already been + settled by rules 1 to 3 from the directory: ``~/.mureo/credentials.json`` + denies on rule 1, while ``~/backups/credentials.json`` is the user's own + file and refusing it would be the guard overreaching into a directory it + does not protect. Without the restriction the rule also contradicted + three cases this file already reasons about and allows — + ``cat "$HOME/.mure?/credentials.json"`` and the two fully-quoted paths — + where the name is written but the shell cannot reach the directory. + + ``config.json`` is deliberately NOT among them. It is one of the most + common filenames in software, and denying it would stop ``cat + config.json`` in every project the agent ever works in — the guard is + judged by whether it makes the common accident less likely *without + blocking real work*, and that trade lands the wrong way. The cost is + stated rather than hidden: ``find ~ -name config.json -exec cat {} ;`` + still reads that one file. The names that are matched are specific + enough that a project file colliding with one is rare, and when it does + the deny reason says to use the Read tool, which is guarded by path and + so allows a same-named file anywhere outside ``~/.mureo``. + Normalization produces that one reading, and it is a left fold over the characters with a five-state quoting automaton — unquoted, single-quoted, double-quoted, and the two escaped states — because that is the only way @@ -261,7 +331,16 @@ ``extglob`` set, and which ``fnmatch`` does not implement; - patterns for *sibling* names (``~/.mur*_backup``): rule 2 asks only whether a pattern matches ``.mureo`` itself, whereas rule 1 does deny - literal siblings such as ``~/.mureo_backup``. + literal siblings such as ``~/.mureo_backup``; + - ``config.json`` reached by a filename search, for the reason given + with rule 4: the name is too common to deny; + - a symlink into the directory under a name that mentions neither the + directory nor a protected filename (``cat ~/notes/backup.json`` where + that path is a link to the credentials file). The *path* guard + resolves symlinks in both directions and closes this; the Bash guard + never touches the filesystem, so it cannot. The two guards protect + the same directory with different reach, and this is where they + differ. The first two are not closable by inspecting command text, and no further rule should be added pretending otherwise. @@ -497,7 +576,7 @@ def _deny_expr(reason: str) -> str: # An expansion swallows the identifier run that names it: `$D` and `%s` are # one unknown thing, not an unknown thing followed by the letters `d`/`s`. -# Collapsing them is what lets a single reading serve both rules — after +# Collapsing them is what lets a single reading serve the rules — after # it, `$D.mureo` reads as `*/.mureo`, whose dot sits at a boundary exactly # like the one in `~/.mureo`, so the literal rule needs no separate scan of # the raw text to find it. This cannot hide a name: it removes only @@ -593,6 +672,48 @@ def _deny_expr(reason: str) -> str: _BASH_REASON = "mureo credential guard: commands that can reach ~/.mureo are blocked" +# The files rule 4 matches by name, so a tree search cannot walk to them +# without naming the directory. ``config.json`` is deliberately absent — +# see rule 4 in the module docstring for why, and for what that costs. +GUARDED_FILENAMES = ( + "credentials.json", + "credentials.json.bak", + "agency.json", + "setup_state.json", +) + +# One alternation over those names, matched only where the name stands on +# its own — no ``/`` before it. That restriction is what keeps the rule on +# its own subject. A name with a path in front of it is not a search, it +# is a specific file, and which file it is has already been decided by +# rules 1 to 3 from the directory: ``~/.mureo/credentials.json`` denies on +# rule 1, and ``~/backups/credentials.json`` is somebody's own file that +# the guard has no business refusing. Only the bare form — ``find ~ -name +# credentials.json``, ``locate credentials.json`` — is the shape rule 4 +# exists for. +# +# Dots become ``[.]`` rather than ``\.`` because the payload may not +# contain a backslash, and the trailing boundary is a character class +# rather than ``$`` because it may not contain one of those either — the +# candidate has a space appended before the search so the end of the +# string counts as a boundary. +_FILENAME_PATTERN = ( + "'(^|[^a-z0-9_./-])(" + + "|".join(n.replace(".", "[.]") for n in GUARDED_FILENAMES) + + ")[^a-z0-9_-]'" +) + +# Rule 4 says what actually matched rather than borrowing _BASH_REASON. +# Told the command "can reach ~/.mureo" when it never mentioned the +# directory, an agent goes looking for a reference that is not there and +# retries; told a credential filename appeared, it can tell at once +# whether it meant its own project file, and the Read tool takes that one. +_FILENAME_REASON = ( + "mureo credential guard: this command names a mureo credential file; " + "if you meant a file of your own with the same name, read it with the " + "Read tool instead" +) + # A refusal is not a match, and the agent reading the reason acts on the # difference: told the command references ~/.mureo, it goes looking for a # reference that is not there and retries. This one says what actually @@ -622,8 +743,8 @@ def _deny_expr(reason: str) -> str: + _QUOTE_STEP + ", initial=(0,0))); " # One reading of the command, built once. Brace expansion turns it into - # the list of readings the shell would produce; both rules see all of - # them, so neither depends on a guess about any single one. + # the list of readings the shell would produce; every rule sees all of + # them, so none depends on a guess about any single one. "t=" + _NORMALIZE + "; " "t=" + _COLLAPSE @@ -633,13 +754,38 @@ def _deny_expr(reason: str) -> str: + "p=[x for s in ls for x in re.findall(" "'(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', s)]; " "g=[x for x in p if set('*?[') & set(x) and fnmatch.fnmatchcase('.mureo', x)]; " + # Rule 3: a metacharacter standing immediately before the written-out + # name. `find -path` matches the whole path, leading period included, + # so `*mureo*` reaches the directory although no component of it + # begins with a dot and rule 2 therefore never sees it. + # + # This one reads the RAW text as well as the normalized readings, and + # that is the whole point. Normalization models what the SHELL expands, + # so it correctly neutralizes a quoted `*` — but the quotes in + # `-path '*mureo*'` exist precisely to keep the shell off the pattern + # so that `find` can expand it itself. Judged on the normalized + # reading alone the pattern has already become `=mureo=` and nothing + # fires. What a downstream program will expand is written literally in + # the command, so that is where to look for it. + "h=[x for x in ls + [cc] if re.search('[]*?[]mureo', x)]; " + # Rule 4: the protected filenames, at a component boundary. A space is + # appended so the end of a candidate counts as a boundary without the + # pattern needing a `$`, which the payload may not contain. + "f=[s for s in ls if re.search(" + _FILENAME_PATTERN + ", s + chr(32))]; " # `un` first: structure the guard could not resolve denies on its own. # `bg` is answered separately below, because it needs its own reason. - "b=un or [s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)] or g; " + "b=un or [s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)]" + " or g or h; " + # Rule 4 carries its own reason: told the command "can reach ~/.mureo" + # when it never mentioned the directory, an agent goes looking for a + # reference that is not there. This one says what actually matched. + "fb=[] if b else f; " + _deny_expr(_OVERSIZE_REASON) + " if bg else (" + _deny_expr(_BASH_REASON) - + " if b else None)" + + " if b else (" + + _deny_expr(_FILENAME_REASON) + + " if fb else None))" ) diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index fae68d3..50a7124 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -15,6 +15,7 @@ from __future__ import annotations +import json import os import re import sys @@ -404,6 +405,162 @@ def test_allows_mureo_own_identifiers(self, fake_home: Path, command: str) -> No assert proc.stdout.strip() == "", command +@pytest.mark.unit +class TestSearchByNameRatherThanByDirectory: + """Rules 3 and 4: a command that never spells the directory. + + Rules 1 and 2 read the directory name, so every test above hands the + guard some spelling of ``~/.mureo``. A tree search does not have to + supply one: ``find ~ -name credentials.json -exec cat {} ;`` and + ``find ~ -path '*mureo*' -exec cat {} ;`` both print the credentials + while mentioning no directory rule 1 or 2 can see. Neither needs + obfuscation, and "find any leftover credential files under my home + directory" is an ordinary instruction rather than an attack — which + is exactly the accident this guard exists to make less likely. + """ + + @pytest.mark.parametrize( + "command", + [ + # Rule 4 — the filename is the only thing written down. + "find ~ -name credentials.json -exec cat {} ;", + "find ~ -name credentials.json | xargs cat", + "find ~ -iname credentials.json", + "find / -name agency.json -exec cat {} ;", + "find ~ -name setup_state.json", + "find ~ -name 'credentials.json'", + "locate credentials.json", + "find ~ -name credentials.json.bak", + "fd credentials.json ~", + # The name reached through a substitution the guard cannot + # read still denies, because the name itself is written down. + "cat $(find ~ -name credentials.json)", + ], + ) + def test_denies_a_protected_filename(self, fake_home: Path, command: str) -> None: + proc = run_guard( + _bash_guard_command(), {"command": command}, fake_home, tool_name="Bash" + ) + assert proc.returncode == 0 + assert deny_decision(proc) == "deny", command + + @pytest.mark.parametrize( + "command", + [ + # Rule 3 — a pattern reaching the directory with no leading + # dot of its own. `find -path` matches the whole path, so the + # period is inside the part `*` covers. + "find ~ -path '*mureo*' -exec cat {} ;", + "find ~ -name '*mureo*'", + "find ~ -path *mureo*", + "grep -rl SECRET ~ --include='*mureo*'", + "ls ~/*mureo*", + "find ~ -path '?mureo'", + ], + ) + def test_denies_a_pattern_reaching_the_name( + self, fake_home: Path, command: str + ) -> None: + proc = run_guard( + _bash_guard_command(), {"command": command}, fake_home, tool_name="Bash" + ) + assert proc.returncode == 0 + assert deny_decision(proc) == "deny", command + + def test_rule_three_reads_the_raw_text_not_only_the_expansion( + self, fake_home: Path + ) -> None: + """A quoted ``*`` is dead to the shell and alive to ``find``. + + Normalization models what the SHELL expands, so it neutralizes the + metacharacters in ``-path '*mureo*'`` — correctly, because the + shell will not expand them. But that is why the quotes are there: + they hand the pattern to ``find`` intact. Judged only on the + normalized reading the pattern has already become ``=mureo=`` and + no rule fires, which is why rule 3 also reads the raw command. + + Pinning both spellings keeps that property from being optimized + away by a future change that moves rule 3 onto the readings. + """ + for command in ("find ~ -path '*mureo*'", "find ~ -path *mureo*"): + proc = run_guard( + _bash_guard_command(), + {"command": command}, + fake_home, + tool_name="Bash", + ) + assert deny_decision(proc) == "deny", command + + @pytest.mark.parametrize( + "command", + [ + # `config.json` is deliberately not guarded by name: it is one + # of the most common filenames in software and denying it + # would block real work in every project. The cost is real and + # is stated in the module docstring rather than hidden. + "cat config.json", + "cat ./config.json", + "vim src/config.json", + "find . -name config.json", + # A project file whose name merely contains a guarded one. + "cat src/my_credentials.jsonl", + "cat app-credentials.jsonc", + "cat credentialsxjson", + # The written-out name with no pattern in front of it: working + # inside a checkout of this repository must stay possible. + "grep -r foo mureo/", + "ls mureo/skills", + "pytest tests/test_credential_guard.py", + # A path in front of the name is not a search. Which file it + # is has already been decided from the directory: this one is + # the user's own, under a directory the guard does not + # protect, and refusing it would be overreach. + "cp ~/backups/credentials.json /tmp/", + "tar cf /tmp/x.tar ~/x/credentials.json.bak", + "cat ./secrets/agency.json", + ], + ) + def test_allows_ordinary_work(self, fake_home: Path, command: str) -> None: + proc = run_guard( + _bash_guard_command(), {"command": command}, fake_home, tool_name="Bash" + ) + assert proc.returncode == 0 + assert proc.stdout.strip() == "", command + + def test_the_filename_rule_says_what_matched(self, fake_home: Path) -> None: + """A wrong reason costs a retry. + + Told the command "can reach ~/.mureo" when it never named the + directory, an agent goes looking for a reference that is not + there. Rule 4's reason names what actually matched and points at + the Read tool, which is guarded by path and so still opens a + same-named file of the user's own. + """ + proc = run_guard( + _bash_guard_command(), + {"command": "find ~ -name credentials.json"}, + fake_home, + tool_name="Bash", + ) + reason = json.loads(proc.stdout)["hookSpecificOutput"][ + "permissionDecisionReason" + ] + assert "credential file" in reason + assert "Read tool" in reason + + # A directory reference keeps the original reason. + proc = run_guard( + _bash_guard_command(), + {"command": "cat ~/.mureo/credentials.json"}, + fake_home, + tool_name="Bash", + ) + reason = json.loads(proc.stdout)["hookSpecificOutput"][ + "permissionDecisionReason" + ] + assert "~/.mureo" in reason + + # --------------------------------------------------------------------------- # The shell layer # ---------------------------------------------------------------------------