diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index db65cb36..a1640ba8 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -23,10 +23,30 @@ ``~/.mureo/credentials.json`` that is itself a symlink pointing OUT — its realpath escapes the dir, but the requested path is still under it). Both cover every file in the directory, not just ``credentials.json``. -* Bash guard: denies any command whose text references ``.mureo`` where a - path component could *start*. A substring check is all a command string - allows, but anchoring on the directory name (not ``credentials``) also - catches wildcard forms like ``cat ~/.mureo/cred*``. +* 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. + + The single reading is the load-bearing part, and it was learned the + expensive way. Earlier versions had one rule scanning the raw command + and another scanning the folded text; every obfuscation one of them + resolved was invisible to the other, so each new fold opened a new hole + on the axis the other rule owned. ``D=~/; cat $D.mu\\reo/…`` + reads the file: the continuation was folded away in the text the + pattern rule read, while the rule that knew about ``$D`` was still + looking at the raw command. Nothing here may reintroduce a second + reader. If a rule needs information the fold destroys, the fold has to + preserve it — which is what ``_COLLAPSE`` does for expansion + boundaries — rather than the rule reaching for a different string. + + 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 + that follows the name (``cat ~/.mureo/cred*``). Rule 2 below covers a + metacharacter placed *inside* the name. Because both read the + normalized text, a name that only becomes contiguous once the shell has + worked on it — ``.mure"o"``, ``.mur'e'o``, ``.mu\\reo``, + ``$D.mureo`` — is as visible to them as one written out. A bare substring test over-blocks badly, because case-folded ``.mureo`` is also a prefix of things that are emphatically not the directory: @@ -43,39 +63,321 @@ substring is at the start of the command or preceded by a non-identifier character. - That test alone would be too weak, because an identifier character can - also be the tail of a *substitution* that supplies the parent directory: - with ``D=~/``, the command ``cat $D.mureo/credentials.json`` resolves - into the protected directory while putting ``D`` immediately before the - name. Of all the ways a shell can splice text, only ``$NAME`` and - ``$1`` end in an identifier character — ``${...}``, ``$(...)``, - backticks and brace expansion all close with punctuation, which the - boundary test already catches. The same applies one level up, to - format specifiers consumed by a program (``printf '%s.mureo/...' ~/``). - So the guard additionally denies when the identifier run before the - substring is itself introduced by ``$`` or ``%``. - - Note ``$`` cannot appear literally in the payload (see the NOTE below), - hence ``chr(36)``. + That boundary test would be too weak on the raw command, because an + identifier character can also be the tail of a *substitution* that + supplies the parent directory: with ``D=~/``, the command ``cat + $D.mureo/credentials.json`` resolves into the protected directory while + putting ``D`` immediately before the name. The same applies one level + up, to a format specifier a program will fill in (``printf + '%s.mureo/...' ~/``). + + This is where an earlier design added a *second rule* over the raw text, + and where the split-brain bugs came from. Normalization handles it + instead: an expansion becomes ``*/`` and swallows the identifier run + that names it, so ``$D.mureo``, ``${D}.mureo``, ``$1.mureo`` and + ``%s.mureo`` all read as ``*/.mureo``. The dot then sits after a + non-identifier character, exactly as it does in ``~/.mureo``, and the + one boundary test sees every one of them — including when the name is + *also* broken up, which is what the two-rule version could not do. Nothing that names the directory in plain path syntax is admitted by this: sibling directories (``~/.mureoX``, ``~/.mureo_backup``) still deny, since only the text before the name is consulted. + Rule 2 (the name written as a pattern). A metacharacter inside the name + breaks rule 1's six-character literal while the shell still expands the + pattern onto the real directory: ``cat ~/.mure?/credentials.json`` + prints the credentials file, and so do ``.[m]ureo``, ``.mur*``, + ``.m?reo``, ``.?????``, ``.[!.]*`` and the brace form ``.mure{o,x}`` + (each run against bash 5.2 with a throwaway ``HOME``). No pattern over + the command text can decide this, because the string that reaches the + filesystem does not exist yet — so the guard asks the question the other + way round. It takes the path components of the normalized command, + keeps those beginning at a component boundary with a literal ``.`` and + containing a metacharacter, and denies when ``fnmatch`` says the pattern + matches ``.mureo``. + + Requiring the literal leading ``.`` is what makes that safe to do. A + shell will not let a wildcard match the leading period of a filename + unless ``dotglob`` is set, so ``ls *``, ``rm -rf build/*`` and + ``tests/*.py`` cannot reach ``.mureo`` and are never candidates. + Without that restriction the rule would have to deny every glob anyone + types, ``fnmatch('.mureo', '*')`` being true. + + 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 + to get quoting right. An earlier version stripped quoted spans with two + regex passes and had the defect that shape invites: in ``echo "it's" ; + cat ~/.mure?/x 'x'`` the single-quote pass read the apostrophe of + ``it's`` as an opening delimiter, paired it with the unrelated ``'x'`` + at the end of the line, and deleted the real pattern sitting between + them. The fold cannot make that mistake, and it also gets ``echo + it\\'s`` right, where an escaped quote is not a delimiter at all. + + The fold rewrites five things: + + - quote delimiters are dropped, so the text reads as the shell will read + it (this is what catches ``~/.mure"o"``); + - a line continuation — a backslash with a newline after it — is dropped + whole, both characters, because that is what a shell does with the + pair before it tokenises anything. ``cat ~/.mu\\reo/…`` + prints the credentials file, and so do ``.\\mureo``, + ``.mure\\?`` and the same spellings inside double quotes. + Keeping the newline was enough to stop the name ever being contiguous. + Inside *single* quotes a backslash is an ordinary character, so there + is no continuation there and none is normalized away; + - a *quoted* metacharacter becomes ``=``, because quoting makes it an + ordinary character and no ordinary character in ``.mureo`` is a + metacharacter. That is why ``sed 's/.*//'`` and ``find . -name '.*'`` + are a regex and a literal rather than globs, and why ``cat + "$HOME/.mure?/x"`` — which opens nothing — is allowed while the + unquoted spelling is denied. The placeholder has to be a character + that reads as a *boundary*: it was ``_`` once, and since ``_`` is an + identifier character, ``'{}.mureo'`` folded to ``__.mureo`` and the + boundary test saw one long name rather than the directory; + - the start of an expansion (``$``, backtick, ``%``) becomes ``*/`` and + swallows the identifier run naming it: ``*`` because its text is + unknown, ``/`` because its extent is unknown too, so what follows + cannot be assumed to continue the same path component, and swallowing + ``D`` in ``$D`` so the expansion reads as one unknown thing. ``%`` is + an expansion in every state, quoted or not, because the program that + fills it in is the next one along, not this shell; + - a quoted span containing ``%`` keeps its metacharacters live, because + such a span is a template rather than text: ``printf + '%s.mure?/x'`` builds a name whose ``?`` the shell then globs. The + flag resets at the end of the span, so a ``%`` in one argument cannot + animate the metacharacters of a later one — ``echo "100%" ; sed + 's/.*//'`` is still allowed. + + Brace groups are then *expanded*, not approximated: the normalized text + becomes the list of strings the shell would produce, and every rule runs + against all of them. ``~/.mure{o,x}`` and ``~/{.,z}mureo`` are caught + because ``.mureo`` is literally among the results. + + An earlier version folded each group to one placeholder and guessed + which — ``.*`` if the group held a dot anywhere, ``*`` otherwise — and + the guess is what broke. ``~/.{mureo,x.y}`` has a dot before the group + and a dot inside an alternative that has nothing to do with the + directory; the fold read them as one, produced ``..*``, which requires + two leading dots, and meanwhile the literal ``.mureo`` that rule 1 would + have matched had already been replaced. Both rules passed and the file + was read. Expanding removes the guess instead of refining it. + + Two groups are not lists of alternatives and cannot be enumerated this + way: a sequence (``.{l..n}ureo`` covers ``m`` without the letter + appearing anywhere) and one with absurdly many alternatives. Those fall + back to *both* coarse readings, ``*`` and ``.*``, which between them + cover "supplies a leading dot" and "does not" — the pair the single + guess was missing. A group with neither a comma nor a ``..`` is not + brace expansion at all; bash leaves ``{eo}`` literal, so the guard does + too, and ``~/.mur{eo}`` is allowed because it opens nothing. + + Expansion has a budget — eight passes, 400 strings — so a pathological + command cannot explode the hook. **Whatever the budget does not resolve + is refused.** Anything still holding an expandable group after the + passes denies on that ground alone, without being examined further. + + That rule replaced a fallback that collapsed leftovers coarsely, and it + is worth saying plainly why, because the docstring claimed the fallback + "over-approximates rather than dropping candidates" and that was false. + Past ten levels of nesting the collapse left literal ``{`` and ``}`` in + the candidates, which ``fnmatch`` reads as ordinary characters, so + *neither* rule fired: ``cat ~/.{z11,{z10,…{z1,mureo}}}/…`` — ninety + characters, no exotic syntax — was allowed while bash read the file. A + budget that shrugs is a bypass with a length requirement. The general + form of the rule is: when the guard cannot compute what the shell would + produce, it denies. + + What that refuses in practice is a command with more than eight brace + groups, or one whose expansion exceeds 400 strings. Of twenty-one + brace-using everyday commands — ``awk '{print $1}'``, ``find . -exec rm + {} ;``, ``mkdir -p build/{lib,bin,share}``, ``mv file{1..10}.txt``, + ``jq '{name: .name}'``, eight groups on one line — exactly one is + refused: nine groups on one line. Quoted braces never reach this step, + and a group with no comma and no ``..`` is literal to bash and to ``fe``. + + Deliberate over-blocks, all in the safe direction: + + - anything unquoted that really does glob dotfiles: ``ls .*``, ``ls -d + .??*``, ``rm -rf .[!.]*`` all reach ``~/.mureo`` from ``$HOME`` and + all deny; + - a component holding an expansion is unknown text, so ``ls .$X`` and + ``cat .$(cmd)`` deny. An arithmetic expansion is not treated any + differently, so ``echo .$((1+1))`` and ``cat ~/.mure$((0))?/x`` deny + too, though neither can reach the directory; + - a format string that builds ``.`` is the shape + of ``printf '%s.mureo/…' ~/``, and nothing in the text distinguishes + them, so ``printf '%s.%s' a b`` denies. Of twenty ``%``-heavy + everyday commands (``date +%Y-%m-%d``, ``git log --format=%h``, + ``awk '{printf "%.2f", $1}'``, ``grep '100%'``, a commit message + reading ``30% faster``) that is the only one that does; + - brace structure the expansion budget could not resolve: more than + eight groups in one command, or an expansion whose normalized text + exceeds 200 KB; + - a command longer than 64 KB, which is refused unread (see below); + - sequence syntax this does not recognise — a three-part ``{a..z..2}``, + an endpoint that is neither an integer nor a single letter — which is + refused rather than reasoned about. Bash expands a sequence only for + those two endpoint kinds, so ``{-..0}`` is not a sequence at all and + stays literal; the refusal costs nothing real. Sequences that *are* + recognised are read exactly, so ``echo {1..100}``, ``for i in + {1..5}``, ``printf '%s' {A..Z}`` and ``touch file{1..20}.log`` are + allowed — every one of them denied until the endpoints were consulted, + which is the kind of over-block that teaches people to turn a guard + off. + + Brace expansion itself used to be on this list — ``mv .{foo,bar}`` and + ``rm .{a,b,c}`` denied although neither can name the directory. + Expanding the alternatives exactly, rather than folding them to a + placeholder, removed those: each alternative is judged on its own, and + both are allowed. That is the shape of the right fix for the remaining + entries — compute what the shell would produce instead of approximating + it — and where that is impossible, refuse rather than approximate. + + The coarse approximations that are left: an expansion's *text* + (unknowable, so ``*``), an expansion's *extent* (unknowable, so ``/``), + and a ``%`` template's result. Two more — a sequence group and a group + with more than 64 alternatives — still take both coarse readings rather + than being enumerated; enumerating them is a contained change and the + place to start if this list is ever shortened again. + + What the guard does not cover — measured, not assumed, and pinned by + ``test_known_open_bypasses``: + + - the shell's own options. ``shopt -s dotglob; cat ~/*/x`` reads the + file; the command text says nothing about whether ``dotglob`` is set, + and it can have been set in an earlier call on the same persistent + shell or in the user's rc file. Denying every ``*`` instead is not an + option; + - anything whose text the command does not contain: a name or pattern + produced by another program (``cat ~/$(printf '.')mureo/x``), taken + from a variable set elsewhere (``cat ~/$P/x``), or written in a + notation that has to be decoded first (``cat ~/$'\\x2emureo'/x``). + Both rules can only read what is written down. Where the text *is* + written down the guard does see it, which is why ``P=.mure?; cat + ~/$P/x`` denies; + - extended globs (``.mure@(o|x)``), which bash parses only with + ``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``. + + The first two are not closable by inspecting command text, and no + further rule should be added pretending otherwise. + + What is actually checked, and where — every number below is produced by + committed code, not by a measurement someone once took: + + - ``tests/credential_guard_product.py`` builds a product of {how the + parent directory is supplied: literal, ``$HOME``, ``"$HOME"``, + ``$VAR``, ``"$VAR"``, ``${VAR}``, ``$VAR$EMPTY``, ``$1``, ``$(cmd)``, + backtick} x {how the name is broken: not at all, continuation, two + continuations, single-quote split, single-quoted character, + double-quote split, double-quoted character, escaped character, class, + wildcard, brace here, brace tail, brace whole, sequence, star} x {what + the breaking form contains: plain, an alternative with its own dot, + with two, a backup-looking name, a nested group, a metacharacter, a + leading dot} x {how deeply it nests: 0, 1, 2, 3, 5, 8, 9, 11, 14, 20} + x {where}. 2698 members. ``pytest -m slow`` runs all of them, + executing each in a throwaway ``HOME`` to confirm it really does read + the marker file and then asking the guard: all 2698 read it, all 2698 + deny. The default run checks an evenly-strided sample of 118, so + every commit defends the property even without the slow pass; + - the nesting cliff has its own table: every depth from 1 to 20 with + two, three and five alternatives per level, 60 cells, run by default. + Each asserts that the command really reads the marker file *and* that + the guard denies it. Against the commit before the refusal rule the + deeper cells were allowed while bash read the file, the cliff falling + at depth 11 for two alternatives per level and earlier for more; + - the resource bounds have their own tests: expansion bombs up to + multi-megabyte commands must still answer, and the 64 KB boundary must + refuse on one side and not the other. + + Older figures that once appeared here — a random single-character fuzz — + are gone rather than restated, because nothing in the repository + reproduces them. A number in a docstring with no committed artifact is + a claim about the past, not a property of the code; if a measurement is + worth quoting it is worth committing the thing that produces it. + + Each round of bugs here has been a product of axes the generator only + walked the margins of. It emitted continuations and it emitted + substitutions, but never a continuation *inside* a substituted parent. + Then it emitted brace groups, but every alternative was inert filler, so + a group holding an unrelated dot could not be produced. Then it had a + "nested group" filler at one fixed depth, so 1510 members all sat at + depth two or less and the cliff at eleven was invisible. Each time the + missing dimension was one level *inside* the last one added. + + Take the pattern rather than the instances: a form the generator cannot + produce is a form nothing here has checked, and that applies to the + insides of forms, to how deeply they nest, and to combinations of them, + not only to the list of features. Before trusting a number in this + docstring, look at whether the generator can express the shape it claims + to cover — and prefer a rule that fails closed on what it cannot resolve + over a measurement that says the gap is not reachable. + Both comparisons are case-folded: macOS and Windows filesystems are case-insensitive by default, so ``~/.MUREO/credentials.json`` opens the real file. On case-sensitive filesystems this can only over-block (a genuinely distinct ``~/.MUREO`` directory), never under-block — the right direction for a guard. -The guard remains defense-in-depth, not the primary control: shell -indirection and encoded forms can still evade the Bash guard. Real safety -comes from filesystem permissions on ``~/.mureo`` itself. +Both payloads fail closed. A hook that exits non-zero for any reason +other than the documented block is a *non-blocking* error and the tool +call proceeds, so an exception escaping the payload is a bypass, not a +crash: ``sys.excepthook`` is set to print the deny JSON and exit 0. This +was not academic — malformed stdin made both payloads exit 1 and let the +call through, as did a path with an embedded NUL, which makes +``os.path.realpath`` raise. + +Failing closed is about time as well as exceptions. ``sys.excepthook`` +catches what Python raises; it cannot catch the host killing a hook that +overruns, and that process exits non-zero *without* printing the deny +JSON — which is precisely the non-blocking case where the tool call +proceeds. A guard that is merely slow is a guard that is bypassed, and a +4 MB command of nested brace groups used to take it there: no answer in +45 seconds, 1.95 GB resident. Three bounds keep that shut, all of them +cheap: the command is refused unread above 64 KB, expansion is budgeted on +total normalized bytes rather than on how many candidates there are, and a +pass that has to revert stops the loop instead of letting the remaining +seven recompute and discard the same expansion. Multi-megabyte bombs now +answer in about a fifth of a second. Nothing legitimate comes near 64 KB; +if that ever stops being true, raise the bound deliberately rather than +letting the work grow to fit. + +The payloads run under whatever ``python3`` the host finds on PATH, which +need not be the interpreter mureo itself was installed with. The Bash +payload needs **Python 3.8 or newer** for ``itertools.accumulate(..., +initial=...)``; on anything older it raises, which fails closed — it +denies every Bash call rather than letting any through, so the symptom is +loud and safe rather than silent. Keep it that way: a rewrite of the fold +that avoids ``initial=`` is fine, one that swallows the error is not. + +WHAT THIS GUARD IS. It is a deterrent against an agent reading the +credentials by accident or on a careless instruction — the cases that +actually happen. It is not a security boundary and cannot be made into +one. The agent runs as the user who owns the file, so it can read it +through any construction the text does not reveal: a variable, a +substitution, an encoding, a helper script, a language runtime. The +earlier claim here that "real safety comes from filesystem permissions" +was wrong in the same direction: permissions do not stop a process running +as the owner either. What actually limits the damage is not keeping +long-lived credentials where an autonomous agent runs, scoping and +rotating them, and the audit trail — not this hook. Judge changes to it +by whether they make the common accident less likely without blocking real +work, and do not describe it as more than that. NOTE: the python payloads run inside double quotes on a shell command line (``python3 -c "..."``), so they must not contain double quotes, ``$``, -backticks, backslashes, or newlines. ``tests/test_credential_guard.py`` -enforces this along with the blocking behavior. +backticks, backslashes, newlines, or ``!`` — the last because a shell with +history expansion enabled rewrites ``!`` sequences inside double quotes. +Every one of those characters is also *data* the Bash guard needs, since +they are exactly the characters a shell treats as special, so each arrives +by ``chr()``: 33 ``!``, 34 ``"``, 36 ``$``, 39 ``'``, 92 backslash, 96 +backtick. ``tests/test_credential_guard.py`` enforces the prohibition, and +``TestGuardThroughARealShell`` runs the generated command through a real +bash so the wrapper's own quoting is exercised rather than assumed. """ from __future__ import annotations @@ -113,8 +415,15 @@ def _deny_expr(reason: str) -> str: ) +_PATH_REASON = "mureo credential guard: files under ~/.mureo are protected" + _PATH_GUARD_CODE = ( "import sys,json,os; " + # Fail closed: exit 1 is a non-blocking hook error in both hosts, so an + # escaping exception would let the call through. A path that makes + # realpath raise (an embedded NUL, say) must deny, not proceed. + "sys.excepthook=lambda *a: (" + _deny_expr(_PATH_REASON) + ", " + "sys.stdout.flush(), os._exit(0)); " "d=json.loads(sys.stdin.read() or '{}'); " "i=d.get('tool_input') or {}; " "p=str(i.get('file_path') or i.get('path') or i.get('notebook_path') or ''); " @@ -123,19 +432,214 @@ def _deny_expr(reason: str) -> str: "bl=os.path.abspath(os.path.expanduser('~/.mureo')).lower(); " "r=os.path.realpath(e).lower() if p else ''; " "lp=os.path.abspath(e).lower() if p else ''; " - + _deny_expr("mureo credential guard: files under ~/.mureo are protected") + + _deny_expr(_PATH_REASON) + " if p and (r==b or r.startswith(b+os.sep)" " or lp==bl or lp.startswith(bl+os.sep)) else None" ) +# Shell metacharacters, named once. Most may not appear literally in the +# payload (see the NOTE in the module docstring), so each arrives as a +# chr() call: q1 ', q2 ", bs backslash, dl $, tk backtick, nl newline, +# pc %. `ho` is the placeholder a quoted metacharacter collapses to — it +# must be none of: an identifier character (it has to read as a component +# boundary), a metacharacter, or a dot. +_CHARS = ( + "q1=chr(39); q2=chr(34); bs=chr(92); dl=chr(36); tk=chr(96); nl=chr(10); " + "pc=chr(37); ho='='; mt='*?[]{},'; " +) + +# The quoting automaton, as the step function of a left fold. The state is +# a pair. First, where we are: 0 unquoted, 1 single-quoted, 2 +# double-quoted, 3 escaped (from unquoted), 4 escaped (inside double +# quotes). Inside single quotes nothing is special, not even a backslash — +# the rule bash applies. +# +# Second, whether a `%` has appeared in the quoted span we are inside. A +# quoted string is ordinary text, unless a program is going to build a path +# out of it: ``printf '%s.mure?/x'`` is a template whose metacharacters +# survive into a filename, and the shell then globs the result. The flag +# resets on leaving the span, so the `%` in one argument cannot make the +# metacharacters of a later one live. +_QUOTE_STEP = ( + "lambda kv,x: (" + "(1 if x==q1 else 2 if x==q2 else 3 if x==bs else 0) if kv[0]==0" + " else (0 if x==q1 else 1) if kv[0]==1" + " else (0 if x==q2 else 4 if x==bs else 2) if kv[0]==2" + " else (0 if kv[0]==3 else 2)," + " 1 if x==pc else (kv[1] if kv[0] else 0))" +) + +# Rebuild the command with quoting resolved, one character at a time: drop +# the delimiters; drop the newline of a line continuation, since a shell +# removes the pair before it tokenises anything; turn a quoted +# metacharacter into the placeholder, because quoting makes it an ordinary +# character and no ordinary character in `.mureo` is a metacharacter; and +# turn the start of an expansion into `*/`. +# +# `*` because its text is unknown, and `/` because where it *ends* is +# unknown too: the characters after it in the command (`o` in `.mure$X`, +# `printf o` inside backticks) are not necessarily part of the same path +# component, so they must not extend the pattern being tested. +# +# `k>2` is the two escaped states: only there does a newline belong to a +# continuation. Inside single quotes a backslash is an ordinary character, +# so `.mu\\reo` in single quotes really is a name with a newline +# in it, and normalizing it away would over-block rather than protect. +# +# `%` becomes an expansion in *every* state, quoted or not, because it is +# the next program along that expands it, not this shell. +_NORMALIZE = ( + "''.join('' if (k==0 and x in q1+q2+bs) or (k==1 and x==q1)" + " or (k==2 and x in q2+bs) or (k>2 and x==nl)" + " else ('*/' if x in dl+tk+pc else (ho if k and not m and x in mt else x))" + " for x,(k,m) in zip(cc,st))" +) + +# 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 +# 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 +# identifier characters, and every form the guard looks for contains a dot. +_COLLAPSE = "re.sub('[*]/[a-z0-9_]*', '*/', t)" + +# Brace expansion, done properly: the command is turned into the *list* of +# strings the shell would produce, and every rule runs against all of them. +# +# The previous version folded a group to one placeholder and guessed which: +# `.*` if the group contained a dot anywhere, `*` otherwise. It never asked +# *where* the dot was, so `~/.{mureo,x.y}` — a dot before the group and a +# dot inside an unrelated alternative — folded to `..*`, which wants two +# leading dots, while the literal `.mureo` that rule 1 would have caught had +# already been replaced. It read the credentials file. Expanding removes the +# guess rather than refining it, and it also stops over-blocking +# `mv .{foo,bar}`, since each alternative is now judged on its own. +# +# `fe` finds the first *expandable* innermost group, skipping `{a}`, which +# bash leaves alone — a group is expandable only with a comma or a `..`. +# `al` gives its alternatives; a sequence (`{l..n}`) is not a list of +# alternatives this can enumerate, and neither is a group with absurdly many +# of them, so those are read coarsely — see `sq` below for which of the two +# coarse readings applies and why. +# +# `ga` excludes the newline from a group's contents, because an unquoted +# newline is a token separator: bash will not expand a brace group across +# one, so neither should this. Matching across newlines would also let two +# unrelated braces on different lines of a multi-line command pair up and +# swallow everything between them. +_BRACE_HELPERS = ( + "ga='[{][^{}' + nl + ']*[}]'; " + "fe=lambda s: next((w for w in re.finditer(ga, s)" + " if ',' in w.group() or '..' in w.group()), None); " + # A sequence bash recognises has endpoints that are integers or single + # *letters*, and neither can be a dot: an integer never contains one, + # and a letter range lies within ASCII 65..122, well clear of 46. So a + # recognised sequence cannot supply the leading dot of a dotfile and + # `*` alone reads it. Without that, `echo {1..100}` folded to `.*` and + # denied — a common idiom, and not an attempt at anything. + # + # The `.*` reading is kept for everything this does not recognise as a + # sequence: a three-part `{a..z..2}`, a range with an endpoint that is + # neither, anything malformed. Those are refused conservatively rather + # than reasoned about — bash leaves most of them literal (`{-..0}` is + # not a sequence at all and stays as written), so the cost is an + # over-block on text nobody types and the benefit is not having to be + # right about a syntax this does not parse. + "sq=lambda v: (lambda e: ['*'] if len(e)==2 and" + " ((e[0].lstrip(chr(45)).isdigit() and e[1].lstrip(chr(45)).isdigit())" + " or (len(e[0])==1 and len(e[1])==1 and not" + " (min(ord(e[0]),ord(e[1]))<=46<=max(ord(e[0]),ord(e[1])))))" + " else ['*','.*'])(v.split('..')); " + "al=lambda w: (lambda v: v if ',' in w.group() and len(v)<=64" + " else sq(w.group()[1:-1]))(w.group()[1:-1].split(',')); " + "ex=lambda s: (lambda w: [s[:w.start()] + a + s[w.end():] for a in al(w)]" + " if w else [s])(fe(s)); " +) + +# Eight passes expand eight groups, innermost first, so nesting resolves as +# the outer group becomes innermost, and a cap stops a pathological command +# from exploding the hook. +# +# Whatever is left when the budget runs out is *refused*, not approximated: +# `un` collects the candidates that still hold an expandable group, and a +# non-empty `un` denies on that ground alone. The budget used to end in a +# coarse fallback of two `re.sub` collapses, which past ten levels of +# nesting left literal braces in the candidates — text `fnmatch` reads as +# ordinary characters, so neither rule fired and +# `~/.{z11,{z10,...{z1,mureo}}}` was allowed while bash read the file. A +# budget that shrugs is a bypass with a length requirement. +# +# The rule is general: when the guard cannot compute what the shell would +# produce, it denies. Nothing legitimate is refused by it — a quoted +# `awk '{print $1}'` never reaches this step, and `find . -exec {} \\;` has +# neither a comma nor a `..`, so bash leaves it literal and so does `fe`. +# What is left is a command with more than eight brace groups, or one whose +# expansion exceeds 400 strings, and neither is a thing anyone types. +_EXPAND = ( + "rs=functools.reduce(lambda q,_: q if q[1] else" + " (lambda n: (q[0],True) if sum(map(len,n))>200000" + " else (n, n==q[0]))" + "([y for x in q[0] for y in ex(x)]), range(8), ([t],False)); " + "ls=rs[0]; un=[x for x in ls if fe(x)]; " +) + +# Source of a python expression yielding the regex for one path component +# written as a shell pattern: a literal dot plus the run of characters a +# pattern may contain. Neither ``/`` nor whitespace is in the set, so a run +# stops where the component does. The class needs no backslash escapes: +# ``]`` comes first and ``-`` last, and ``!`` arrives via ``chr(33)``. +_PATTERN_COMPONENT = "'[.][]a-z0-9_.*?[^{},' + chr(33) + '-]*'" + +_BASH_REASON = "mureo credential guard: commands that can reach ~/.mureo are blocked" + +# 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 +# happened — the command was too long to read, so nothing was concluded +# about it. +_OVERSIZE_REASON = ( + "mureo credential guard: command over 65536 bytes was refused unread, " + "not analysed; shorten it or run it in pieces" +) + _BASH_GUARD_CODE = ( - "import sys,json,re; " + "import sys,json,re,os,fnmatch,functools,itertools; " + # Fail closed: an escaping exception exits 1, which both hosts treat as a + # non-blocking hook error, so every exception must deny instead. + "sys.excepthook=lambda *a: (" + _deny_expr(_BASH_REASON) + ", " + "sys.stdout.flush(), os._exit(0)); " "d=json.loads(sys.stdin.read() or '{}'); " "c=str((d.get('tool_input') or {}).get('command') or '').lower(); " - "b=re.search('(^|[^a-z0-9_])[.]mureo', c) or " - "re.search('[' + chr(36) + '%][a-z0-9_]*[.]mureo', c); " - + _deny_expr("mureo credential guard: commands referencing .mureo are blocked") - + " if b else None" + # A guard that is merely slow is a guard that is bypassed: the host + # kills a hook that overruns and that process exits non-zero without + # printing the deny JSON, which is the non-blocking case. So an + # oversized command is refused before any of the work below, and every + # later step runs on the empty string instead. + "bg=len(c)>65536; cc='' if bg else c; " + + _CHARS + + "st=list(itertools.accumulate(cc, " + + _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. + "t=" + _NORMALIZE + "; " + "t=" + + _COLLAPSE + + "; " + + _BRACE_HELPERS + + _EXPAND + + "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)]; " + # `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; " + + _deny_expr(_OVERSIZE_REASON) + + " if bg else (" + + _deny_expr(_BASH_REASON) + + " if b else None)" ) diff --git a/pyproject.toml b/pyproject.toml index 7092b364..ce7c229f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -161,6 +161,7 @@ markers = [ "unit: Unit tests", "integration: Integration tests", "real_save: Opt out of the credentials-write stub and save for real (tmp_path)", + "slow: Exhaustive runs kept out of the default suite (pytest -m slow)", ] [tool.mypy] diff --git a/tests/credential_guard_product.py b/tests/credential_guard_product.py new file mode 100644 index 00000000..bab07ab0 --- /dev/null +++ b/tests/credential_guard_product.py @@ -0,0 +1,180 @@ +"""The differential product the Bash credential guard's numbers come from. + +``mureo/credential_guard.py`` quotes counts — "all N members read the +credentials file, and all N deny". This module is where those come from, so +the claim is a property that can be re-run rather than a number someone +once measured. ``tests/test_credential_guard_product.py`` runs it. + +The shape is a cartesian product of the axes that have actually produced +bypasses, because every one of them was a *combination* rather than a +feature: + +* how the parent directory is supplied — literal, ``$HOME``, ``"$HOME"``, + ``$VAR``, ``"$VAR"``, ``${VAR}``, ``$VAR$EMPTY``, ``$1``, ``$(cmd)``, + backtick; +* how the name is broken up — continuation, quote splits, escapes, + classes, wildcards, brace forms, sequences; +* what the breaking form *contains* — an alternative carrying its own dot + is what defeated the fold that replaced a group with one placeholder; +* how deeply the form nests — the expansion budget's cliff was invisible + while every member sat at depth two or less; +* where in the name it happens. + +Each member is checked two ways: run in a real bash against a throwaway +HOME holding a marker credentials file, and put through the real generated +hook command. A member that reads the marker and is not denied is a +bypass. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +# These tests execute the attack command in a real shell and check whether it +# reaches the file. That is a statement about POSIX path, glob and quoting +# semantics; under an emulated shell on Windows a "no" would mean "not +# reachable through this translation layer", which is not the property under +# test and is indistinguishable from the guard working. +POSIX_SHELL = os.name == "posix" + +Q1, Q2, BS, TICK, NL = chr(39), chr(34), chr(92), chr(96), chr(10) +CONT = BS + NL +NAME = ".mureo" +MARKER = "MUREO-PRODUCT-MARKER-8F31A7" + +# (label, prelude, text placed in front of the name) +PARENTS: list[tuple[str, str, str]] = [ + ("literal", "", "~/"), + ("$HOME", "", "$HOME/"), + ('"$HOME"', "", Q2 + "$HOME" + Q2 + "/"), + ("$VAR", "D2=~/; ", "$D2"), + ('"$VAR"', "D2=~/; ", Q2 + "$D2" + Q2), + ("${VAR}", "D2=~/; ", "${D2}"), + ("$VAR$EMPTY", "D2=~/; E2=; ", "$D2$E2"), + ("$1", "set -- ~/; ", "$1"), + ("$(cmd)", "", "$(printf " + Q1 + "%s" + Q1 + " ~/)"), + ("backtick", "", TICK + "printf " + Q1 + "%s" + Q1 + " ~/" + TICK), +] + +# (label, the other alternative of a brace group, extra members of a class) +FILLERS: list[tuple[str, str, str]] = [ + ("plain", "z", "z"), + ("dotted", "x.y", "x"), + ("dotted twice", "a.b.c", "a"), + ("backup-looking", "bashrc.bak", "b"), + ("nested group", "{a,b}.c", "c"), + ("metachar", "x*", "x"), + ("leading dot", ".hidden", "h"), +] + +NESTINGS = [0, 1, 2, 3, 5, 8, 9, 11, 14, 20] + + +def breaks(alt: str, cls: str) -> list[tuple[str, object]]: + """Ways to spell ``NAME`` so the six characters are not consecutive.""" + return [ + ("none", lambda n, i: n), + ("continuation", lambda n, i: n[:i] + CONT + n[i:]), + ("two continuations", lambda n, i: n[:i] + CONT + n[i:] + CONT), + ("single-quote split", lambda n, i: n[:i] + Q1 + Q1 + n[i:]), + ("single-quoted char", lambda n, i: n[:i] + Q1 + n[i] + Q1 + n[i + 1 :]), + ("double-quote split", lambda n, i: n[:i] + Q2 + Q2 + n[i:]), + ("double-quoted char", lambda n, i: n[:i] + Q2 + n[i] + Q2 + n[i + 1 :]), + ("escaped char", lambda n, i: n[:i] + BS + n[i] + n[i + 1 :]), + ("class", lambda n, i: n[:i] + "[" + n[i] + cls + "]" + n[i + 1 :]), + ("wildcard", lambda n, i: n[:i] + "?" + n[i + 1 :]), + ("brace here", lambda n, i: n[:i] + "{" + n[i] + "," + alt + "}" + n[i + 1 :]), + ("brace tail", lambda n, i: n[:i] + "{" + n[i:] + "," + alt + "}"), + ("brace whole", lambda n, i: "{" + n + "," + alt + "}"), + ("sequence", lambda n, i: n[:i] + "{" + n[i] + ".." + n[i] + "}" + n[i + 1 :]), + ("star", lambda n, i: n[:i] + "*" + n[i + 1 :]), + ] + + +def nest(spelled: str, depth: int) -> str: + """Bury a spelling inside ``depth`` further brace levels.""" + for level in range(depth): + spelled = "{z" + str(level) + "," + spelled + "}" + return spelled + + +def members() -> list[tuple[str, str]]: + """Every member of the product, as ``(label, command)``.""" + out: list[tuple[str, str]] = [] + seen: set[str] = set() + for plabel, prelude, prefix in PARENTS: + for flabel, alt, cls in FILLERS: + for blabel, spell in breaks(alt, cls): + # A filling only varies the forms that have one. + if flabel != "plain" and not ( + blabel.startswith("brace") or blabel == "class" + ): + continue + positions = [0] if blabel in ("none", "brace whole") else range(1, 6) + for i in positions: + try: + spelled = spell(NAME, i) # type: ignore[operator] + except IndexError: + continue + depths = ( + NESTINGS + if (flabel == "plain" and plabel in ("literal", "$VAR")) + else [0] + ) + for depth in depths: + body = nest(spelled, depth) + cmd = prelude + "cat " + prefix + body + "/credentials.json" + if cmd in seen: + continue + seen.add(cmd) + out.append( + (f"{plabel} | {blabel} | {flabel} | @{i} | d{depth}", cmd) + ) + return out + + +def build_home(root: str | os.PathLike[str]) -> Path: + """A throwaway HOME with a marker credentials file.""" + home = Path(root) / "home" + shutil.rmtree(home, ignore_errors=True) + (home / ".mureo").mkdir(parents=True) + (home / ".mureo" / "credentials.json").write_text( + json.dumps({"access_token": MARKER}), encoding="utf-8" + ) + (home / "project").mkdir(exist_ok=True) + return home + + +def reads_marker(command: str, home: str | os.PathLike[str], bash: str) -> bool: + """Does a real shell actually print the credentials file for this? + + ``home`` is coerced here rather than at the call sites. It arrives as a + ``Path`` from every caller, and ``subprocess`` on POSIX accepts one in + ``env`` while Windows raises ``TypeError: environment can only contain + strings`` — so an annotation that disagreed with the argument was not + cosmetic, it was a failure waiting for the first platform that enforces + the contract. Coercing at the boundary means the signature and the + reality agree without every caller having to remember. + + The environment is ``os.environ`` with ``HOME`` overridden, not a + hand-built pair: a minimal env drops variables a shell needs to start + at all on some platforms, which fails as "the command did not read the + file" — indistinguishable from the guard working. + """ + env = dict(os.environ, HOME=str(home), USERPROFILE=str(home)) + try: + proc = subprocess.run( + [bash, "-c", command], + capture_output=True, + text=True, + env=env, + cwd=str(home), + timeout=15, + ) + except subprocess.TimeoutExpired: + return False + return MARKER in proc.stdout diff --git a/tests/hook_guard_runner.py b/tests/hook_guard_runner.py index 2f2c60f0..89e50b87 100644 --- a/tests/hook_guard_runner.py +++ b/tests/hook_guard_runner.py @@ -9,8 +9,14 @@ portable (no bash dependency on the Windows CI job) while exercising the exact code a shell would hand to ``python3 -c``. ``extract_python_code`` also asserts the payload is shell-safe: because the code sits inside double -quotes on the command line, any ``"``, ``$``, backtick, or backslash would -change meaning under a POSIX shell. +quotes on the command line, any ``"``, ``$``, backtick, backslash, or ``!`` +(history expansion) would change meaning under a POSIX shell. + +That is not the whole story, though: lifting the payload out skips the +wrapper, so a quoting mistake in the wrapper itself would go unnoticed. +``run_guard_in_shell`` closes that by handing the whole command to a real +bash. Anything whose answer depends on quoting — the guard's own, or the +command's — belongs there. """ from __future__ import annotations @@ -18,6 +24,7 @@ import json import os import re +import shutil import subprocess import sys from pathlib import Path @@ -25,7 +32,10 @@ _COMMAND_RE = re.compile(r'^python3 -c "(?P[^"]*)" # \[mureo-credential-guard\]$') -_SHELL_HAZARDS = ("$", "`", "\\", "\n") +_SHELL_HAZARDS = ("$", "`", "\\", "\n", "!") + +BASH = shutil.which("bash") +PYTHON3 = shutil.which("python3") def extract_python_code(command: str) -> str: @@ -66,6 +76,42 @@ def run_guard( ) +def run_guard_in_shell( + command: str, + tool_input: dict[str, Any] | None, + home: Path, + tool_name: str = "Bash", + raw_stdin: str | None = None, +) -> subprocess.CompletedProcess[str]: + """Run the guard command *as a shell runs it*: ``bash -c ``. + + ``run_guard`` above lifts the python payload out of the command and runs + it directly, which never exercises the ``python3 -c "..."`` wrapper. The + quoting of that wrapper is a layer of its own: it is where a stray ``"``, + ``$``, backtick or ``!`` in the payload would change the program the + shell actually runs. Tests that care about a quoting question must go + through here. + + ``raw_stdin`` sends bytes verbatim instead of a tool-call JSON, for the + malformed-input cases. + """ + assert BASH is not None and PYTHON3 is not None, "needs bash and python3" + payload = ( + raw_stdin + if raw_stdin is not None + else json.dumps({"tool_name": tool_name, "tool_input": tool_input or {}}) + ) + env = dict(os.environ, HOME=str(home), USERPROFILE=str(home)) + return subprocess.run( + [BASH, "-c", command], + input=payload, + capture_output=True, + text=True, + env=env, + timeout=30, + ) + + def deny_decision(proc: subprocess.CompletedProcess[str]) -> str | None: """Return the ``permissionDecision`` emitted by a guard run, if any.""" if not proc.stdout.strip(): diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index dec5b75e..fae68d3b 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -8,7 +8,9 @@ never be mistaken for an intentional block. These tests execute the generated hook payloads in a subprocess with a fake -``$HOME``, mirroring how the agent harness invokes them. +``$HOME``, mirroring how the agent harness invokes them. The cases whose +answer depends on quoting run the whole command through a real bash instead +— see ``TestGuardThroughARealShell``. """ from __future__ import annotations @@ -20,7 +22,18 @@ import pytest -from tests.hook_guard_runner import deny_decision, run_guard +from tests.hook_guard_runner import ( + BASH, + PYTHON3, + deny_decision, + run_guard, + run_guard_in_shell, +) + +needs_shell = pytest.mark.skipif( + BASH is None or PYTHON3 is None, + reason="the shell layer needs both bash and python3 on PATH", +) _PROTECTED_FILES = ( "credentials.json", @@ -44,13 +57,13 @@ def fake_home(tmp_path: Path) -> Path: def _path_guard_command() -> str: from mureo.credential_guard import path_guard_entry - return path_guard_entry()["hooks"][0]["command"] + return str(path_guard_entry()["hooks"][0]["command"]) def _bash_guard_command() -> str: from mureo.credential_guard import bash_guard_entry - return bash_guard_entry()["hooks"][0]["command"] + return str(bash_guard_entry()["hooks"][0]["command"]) # --------------------------------------------------------------------------- @@ -261,6 +274,95 @@ def test_denies_every_spelling_of_the_mureo_dir( ) assert deny_decision(proc) == "deny", command + @pytest.mark.parametrize( + "command", + [ + # Every one of these was verified against a throwaway $HOME with a + # real bash 5.2: each prints the contents of the credentials file. + "cat ~/.mure?/credentials.json", + "cat ~/.[m]ureo/credentials.json", + "cat ~/.mur*/credentials.json", + "cat ~/.m?reo/credentials.json", + "cat ~/.?????/credentials.json", + "cat ~/.[!.]*/credentials.json", + "cat ~/.mure[o]/credentials.json", + # Brace expansion runs before pathname expansion, so it produces + # the real directory name without any wildcard at all. + "cat ~/.mure{o,x}/credentials.json", + "cat ~/.mur{eo,ex}/credentials.json", + # Same patterns, other spellings of the parent directory. + "ls -la ~/.mure?", + "cp -r ~/.m?reo /tmp/exfil", + "cat $HOME/.mure?/credentials.json", + "cat ${HOME}/.mur*/credentials.json", + 'cat "$HOME"/.mure?/credentials.json', + "cat /Users/x/.mur*/credentials.json", + # Case-folded, as everywhere else in the guard. + "cat ~/.MURE?/credentials.json", + "cat ~/.[M]UREO/credentials.json", + # A substitution supplies the parent, so the pattern does not + # start at a path boundary — the same shapes rule 1 covers for + # the literal name. + "D=~/; cat $D.mure?/credentials.json", + "cat $(printf '%s.mure?/credentials.json' ~/)", + "python3 -c \"print(open('%s.mure?/credentials.json' % h).read())\"", + ], + ) + def test_denies_glob_patterns_matching_the_mureo_dir( + self, fake_home: Path, command: str + ) -> None: + """A wildcard inside the directory name still reaches the real files. + + The literal-substring rule looks for six consecutive characters, so + any metacharacter placed *inside* ``.mureo`` breaks the match while + the shell still expands the pattern onto the protected directory. + """ + proc = run_guard( + _bash_guard_command(), {"command": command}, fake_home, tool_name="Bash" + ) + assert deny_decision(proc) == "deny", command + + @pytest.mark.parametrize( + "command", + [ + # Wildcards that cannot reach a dotfile at all: the shell requires + # a leading period to be matched explicitly. + "ls *", + "rm -rf build/*", + "cp dist/* /tmp/", + "node --test tests/js/*.test.js", + "pytest tests/test_*.py", + "ls -d */", + "git add -- mureo/*.py", + # Dot-leading patterns that cannot spell the directory name. + "rm -f .coverage*", + "ls -d .git*", + "cat .env.*", + "rm -rf .pytest_cache .ruff_cache", + # Quoted metacharacters never reach pathname expansion — regexes + # and format strings must not be read as globs. + "sed 's/.*//' notes.txt", + "grep -rn '.*TODO' mureo/", + "find . -name '*.py' -newer setup.py", + "find . -name '.*' -maxdepth 1", + "git log --grep '.*fix'", + # ...including a fully quoted path: quoting suppresses globbing, + # so `?` here is a literal character and opens nothing. + 'cat "$HOME/.mure?/credentials.json"', + # Ordinary commands with no pattern at all. + "ruff check .", + "git diff -- .", + "black --check .", + ], + ) + def test_allows_everyday_glob_commands(self, fake_home: Path, command: str) -> None: + """The pattern rule must not fire on day-to-day shell usage.""" + proc = run_guard( + _bash_guard_command(), {"command": command}, fake_home, tool_name="Bash" + ) + assert proc.returncode == 0 + assert proc.stdout.strip() == "", command + @pytest.mark.parametrize("command", ["echo hello", "ls -la", "git status"]) def test_allows_unrelated_commands(self, fake_home: Path, command: str) -> None: proc = run_guard( @@ -302,6 +404,312 @@ def test_allows_mureo_own_identifiers(self, fake_home: Path, command: str) -> No assert proc.stdout.strip() == "", command +# --------------------------------------------------------------------------- +# The shell layer +# --------------------------------------------------------------------------- + + +@needs_shell +@pytest.mark.unit +class TestGuardThroughARealShell: + """Run the generated command the way a host runs it: ``bash -c``. + + Every case below was checked outside the suite against a throwaway + ``HOME`` holding a marker credentials file: the deny cases print the + marker when the guard is removed, and the allow cases print nothing. + """ + + @pytest.mark.parametrize( + "command", + [ + # Quoting splits the name, and quote removal puts it back + # together. None of these contains `.mureo` as six consecutive + # characters, and the first four hide the metacharacter too. + "cat ~/'.'mure?/credentials.json", + 'cat ~/".mure"?/credentials.json', + "cat ~/.mure''?/credentials.json", + 'cat ~/.mure"o"/credentials.json', + 'cat "$HOME"/.mure"o"/credentials.json', + "cat ~/.mur'e'o/credentials.json", + # An apostrophe inside a double-quoted word is an ordinary + # character. Reading it as a delimiter pairs it with the next + # quote and swallows the real pattern in between. + "echo \"it's\" ; cat ~/.mure?/credentials.json 'x'", + "echo \"don't\" && cat ~/.mure?/credentials.json 'y'", + # A backslash-escaped quote is not a delimiter either. + "echo it\\'s ; cat ~/.mure?/credentials.json", + # An escaped metacharacter is literal, but an escaped letter is + # still the letter. + "cat ~/\\.mureo/credentials.json", + "cat ~/.mur\\eo/credentials.json", + # A substitution inside the name makes the rest of it unknown. + "cat ~/.mure$(printf '?')/credentials.json", + "cat ~/.mure`printf o`/credentials.json", + # A brace group can supply any character, including the dot. + "cat ~/{.,z}mureo/credentials.json", + "cat ~/.mure{o,x}/credentials.json", + "cat ~/.mur{e{o,z},y}/credentials.json", + # An alternative carrying its own dot, which is unrelated to the + # dot of the dotfile. Folding the group to a single placeholder + # had to guess which of the two the dot belonged to, and chose + # wrong: these read the credentials file. + "cat ~/.{mureo,x.y}/credentials.json", + "cp -r ~/.{mureo,bashrc.bak} /tmp/dest/", + "ls -la ~/.{mureo,x.y}", + "cat ~/.mure{o,x.y}/credentials.json", + "cat ~/.m{ureo,x.y}/credentials.json", + "cat ~/.{MUREO,x.y}/credentials.json", + "cat $HOME/.{mureo,x.y}/credentials.json", + "cat ~/.{mure?,x.y}/credentials.json", + "cat ~/.{a.b,c.d,mureo}/credentials.json", + "cat ~/.{mureo,{a,b}.c}/credentials.json", + # A sequence expression is not a list of alternatives at all, + # and `{l..n}` covers `m` without the letter appearing anywhere. + "cat ~/.{l..n}ureo/credentials.json", + "cat ~/.mure{n..p}/credentials.json", + # Nested past the expansion budget. The budget used to end in a + # coarse fallback that left literal braces in the candidates — + # ordinary characters to fnmatch — so neither rule fired and the + # file was read. Unresolved structure now denies on its own. + "cat ~/.{z2,{z1,mureo}}/credentials.json", + "cat ~/.{z9,{z8,{z7,{z6,{z5,{z4,{z3,{z2,{z1,mureo}}}}}}}}}/creds", + "cat ~/.{z11,{z10,{z9,{z8,{z7,{z6,{z5,{z4,{z3,{z2,{z1,mureo}}}" + "}}}}}}}}/credentials.json", + "cp -r ~/.{z11,{z10,{z9,{z8,{z7,{z6,{z5,{z4,{z3,{z2,{z1,mureo}}}" + "}}}}}}}}/ /tmp/dest/", + # Three and five alternatives per level reach the budget sooner. + "cat ~/.{a,b,{c,d,{e,f,{g,h,{i,j,{k,l,{m,n,mureo}}}}}}}/creds", + # A letter range covering `m`. This one really does expand onto + # the directory; `{-..0}` below does not, and is a row in the + # conservative-refusal test instead. + "cat ~/.{l..n}ureo/credentials.json", + # A line continuation is deleted, backslash and newline both, + # before the shell tokenises anything — so the name is spelled + # across two lines and is contiguous by the time it is used. + "cat ~/.mu\\\nreo/credentials.json", + "cat ~/.\\\nmureo/credentials.json", + "cat ~/.m\\\nu\\\nr\\\ne\\\no/credentials.json", + "cat ~/.mure\\\n?/credentials.json", + "cat ~/.m\\\nure?/credentials.json", + # ...including inside double quotes, where it is still a + # continuation (and where nothing globs, so the name itself is + # what has to be seen). + 'cat "$HOME/.mu\\\nreo/credentials.json"', + # `$"..."` is a translated string: the `$` is not an expansion + # of anything the guard cannot see. + 'cat ~/$".mureo"/credentials.json', + # The parent directory comes from a substitution *and* the name + # is broken by something only normalization resolves. This is + # the product of the two axes, and it is the category that + # shipped twice: while the two rules read different strings, + # whatever one of them folded away the other could not see. + "D2=~/; cat $D2.mu\\\nreo/credentials.json", + 'D2=~/; cat "$D2".mu\\\nreo/credentials.json', + "set -- ~/; cat $1.mu\\\nreo/credentials.json", + "D2=~/; E2=; cat $D2$E2.mu\\\nreo/credentials.json", + "D2=~/; cat ${D2}.mu\\\nreo/credentials.json", + "D2=~/; cat $D2.\\\nmureo/credentials.json", + "D2=~/; cat $D2.m\\\nur\\\neo/credentials.json", + 'D2=~/; cat $D2.mure"o"/credentials.json', + "D2=~/; cat $D2.mur'e'o/credentials.json", + "D2=~/; cat $D2.mure?/credentials.json", + 'D2=~/; cat "$D2".mure[o]/credentials.json', + "set -- ~/; cat $1.mure{o,x}/credentials.json", + "cat $(printf '%s' ~/).mu\\\nreo/credentials.json", + "cat `printf '%s' ~/`.mure?/credentials.json", + # ...and the plain forms still deny through the shell layer. + "cat ~/.mureo/credentials.json", + "cat ~/.mure?/credentials.json", + "D=~/; cat $D.mure?/credentials.json", + "cat $(printf '%s.mure?/credentials.json' ~/)", + ], + ) + def test_denies_through_the_shell(self, fake_home: Path, command: str) -> None: + proc = run_guard_in_shell( + _bash_guard_command(), {"command": command}, fake_home + ) + assert proc.returncode == 0, proc.stderr + assert deny_decision(proc) == "deny", command + + @pytest.mark.parametrize( + "command", + [ + # Quoted metacharacters are ordinary characters: a regex, a + # literal argument, a path that opens nothing. + "sed 's/.*//' notes.txt", + "sed -e 's|.*/||' paths.txt", + "find . -name '.*' -maxdepth 1", + "grep -rn '.*TODO' mureo/", + 'cat "$HOME/.mure?/credentials.json"', + "cat '~/.mure?/credentials.json'", + # Everyday globbing, which cannot reach a dotfile. + "ls *", + "rm -rf build/*", + "node --test tests/js/*.test.js", + "ls -d .git*", + # Inside single quotes a backslash is an ordinary character, so + # this is a name with a newline in it, not a continuation, and + # it opens nothing. + "cat '~/.mu\\\nreo/credentials.json'", + # Brace usage that is not an attempt at the directory. The + # refusal on unresolved structure must not reach these: the + # awk/jq bodies are quoted, `{}` has no comma, and eight groups + # sit inside the expansion budget. + "awk '{print $1}' data.txt", + "find . -name '*.pyc' -exec rm {} ;", + "mkdir -p build/{lib,bin,share}", + "mv report.{txt,md}", + "mv file{1..10}.txt archive/", + # A bare sequence group, which is a common idiom and denied + # until the range was consulted: integers hold no dot, and a + # character range holds one only if it spans ASCII 46. + "echo {1..100}", + "for i in {1..5}; do touch file$i.txt; done", + "printf '%s\\n' {A..Z}", + "touch file{1..20}.log", + "mkdir -p test{1..3}/{a..c}", + "echo {a..z}{0..9}", + "mkdir -p a/{1,2}/b/{3,4}/c/{5,6}/d/{7,8}", + "kubectl get pods -o jsonpath='{.items[0].metadata.name}'", + # Braces on separate lines of a multi-line command must not pair + # up across the newline and swallow what lies between them. + "echo '{' > a.json\necho '}' >> a.json", + # A continuation that only wraps a long line. + "ls -la \\\n ~/project", + # mureo's own identifiers, including inside quotes. + "gh release create v0.10.44 --notes 'adds window.MUREO_REPORTS_FORMAT'", + "pip install --index-url https://pkgs.mureo.jp/simple/ mureo-agency", + "echo user@pkgs.mureo.jp", + ], + ) + def test_allows_through_the_shell(self, fake_home: Path, command: str) -> None: + proc = run_guard_in_shell( + _bash_guard_command(), {"command": command}, fake_home + ) + assert proc.returncode == 0, proc.stderr + assert proc.stdout.strip() == "", command + + @pytest.mark.parametrize( + "raw_stdin", + [ + "{not json", + "[]", + '{"tool_input": "not a dict"}', + "\x00\x01\x02", + ], + ) + def test_malformed_input_denies_rather_than_escapes( + self, fake_home: Path, raw_stdin: str + ) -> None: + """An exception in the payload is a bypass, not a crash. + + Exit 1 is a *non-blocking* hook error in both hosts, so a payload + that raises lets the tool call proceed. Anything the guard cannot + parse must therefore deny. + """ + proc = run_guard_in_shell( + _bash_guard_command(), None, fake_home, raw_stdin=raw_stdin + ) + assert deny_decision(proc) == "deny", raw_stdin + assert proc.returncode == 0, proc.stderr + + def test_path_guard_malformed_input_denies(self, fake_home: Path) -> None: + proc = run_guard_in_shell( + _path_guard_command(), None, fake_home, raw_stdin="{not json" + ) + assert deny_decision(proc) == "deny" + assert proc.returncode == 0 + + @pytest.mark.skipif( + sys.platform == "win32", + reason=( + "the trigger is POSIX-specific: os.path.realpath raises on an " + "embedded NUL there, while on Windows it returns and the path " + "simply does not resolve under ~/.mureo, so there is no " + "exception to escape. The property this demonstrates — an " + "exception denies rather than exits 1 — is asserted on every " + "platform by the malformed-stdin tests above, for both guards" + ), + ) + def test_path_guard_unusable_path_denies(self, fake_home: Path) -> None: + """``realpath`` raises on an embedded NUL — that must not fail open.""" + proc = run_guard_in_shell( + _path_guard_command(), + {"file_path": "\x00/x/.mureo/credentials.json"}, + fake_home, + tool_name="Read", + ) + assert deny_decision(proc) == "deny" + assert proc.returncode == 0 + + @pytest.mark.parametrize( + "command", + [ + "cat ~/{-..0}mureo/credentials.json", + "echo {-..0}", + "echo {a..z..2}", + ], + ) + def test_denies_sequence_syntax_it_does_not_recognise( + self, fake_home: Path, command: str + ) -> None: + """Unrecognised sequence syntax is refused rather than reasoned about. + + These are over-blocks, recorded as such. Bash expands a sequence + only when both endpoints are integers or single letters, so + ``{-..0}`` is not a sequence at all and stays literal — none of + these reaches the directory, and none of them is something a person + types. The guard does not parse the syntax and does not try to: it + reads what it cannot resolve conservatively, which is the same rule + that closed the nesting cliff. + """ + proc = run_guard_in_shell( + _bash_guard_command(), {"command": command}, fake_home + ) + assert proc.returncode == 0, proc.stderr + assert deny_decision(proc) == "deny", command + + @pytest.mark.parametrize( + "command", + [ + # The shell's own glob options are not in the command text. With + # dotglob set — here, or in an earlier call on the persistent + # shell, or in the user's rc file — `*` reaches dotfiles. + "shopt -s dotglob; cat ~/*/credentials.json", + # The leading dot is produced at runtime, so the text never + # contains a dot-anchored component to test. + "cat ~/$(printf '.')mureo/credentials.json", + "cat ~/$(printf '.')mure?/credentials.json", + # The name is assembled by a previous command. + "cat ~/$P/credentials.json", + # Another notation has to be decoded first. + "cat ~/$'\\x2emureo'/credentials.json", + ], + ) + def test_known_open_bypasses(self, fake_home: Path, command: str) -> None: + """The bypasses this guard does not close, pinned so they cannot grow. + + All but one were run against a throwaway HOME and printed the + credentials file; the exception is ``cat ~/$P/x``, which reads it + only once an earlier call has set ``P`` — the point of the row is + that the name is nowhere in the text. They are here so the open + surface is a list someone has to edit, rather than something a + reviewer discovers: closing one means deleting its row and saying + so in the module docstring. + + What they have in common is that the text handed to the guard does + not contain the thing that reaches the filesystem — it is produced + later, by the shell's options, by another program, or by decoding + another notation. No inspection of the command text can decide them; + see the module docstring in ``mureo/credential_guard.py``. + """ + proc = run_guard_in_shell( + _bash_guard_command(), {"command": command}, fake_home + ) + assert proc.returncode == 0, proc.stderr + assert proc.stdout.strip() == "", f"now denied, update the docstring: {command}" + + # --------------------------------------------------------------------------- # Template structure # --------------------------------------------------------------------------- diff --git a/tests/test_credential_guard_product.py b/tests/test_credential_guard_product.py new file mode 100644 index 00000000..c2758c77 --- /dev/null +++ b/tests/test_credential_guard_product.py @@ -0,0 +1,294 @@ +"""Differential tests for the Bash guard: what the shell does vs what the +guard decides. + +The parametrised rows in ``test_credential_guard.py`` pin roughly ninety +spellings someone thought of. These check a whole product, and they check +it against a real bash rather than against a re-implementation of the rule +— which is the only way the earlier bypasses were ever found. + +Two speeds: + +* the default run takes an evenly-strided sample of the product and asks + the guard about each. It is deterministic, needs a couple of hundred + subprocesses, and is what CI defends on every commit; +* ``-m slow`` runs the whole product, and additionally executes every + member in a throwaway HOME to confirm it really does read the marker + file. This is where the counts quoted in + ``mureo/credential_guard.py`` come from:: + + pytest tests/test_credential_guard_product.py -m slow + +Keep the numbers in that docstring and the output of the slow run in step. +If you add an axis here, update them; if a claim there has no counterpart +here, delete the claim. +""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path + +import pytest + +from tests.credential_guard_product import ( + POSIX_SHELL, + build_home, + members, + reads_marker, +) +from tests.hook_guard_runner import BASH, PYTHON3, deny_decision, run_guard_in_shell + +needs_shell = pytest.mark.skipif( + BASH is None or PYTHON3 is None, + reason="the differential product needs both bash and python3 on PATH", +) + +# Asking the guard about a command is platform-independent — it is string +# analysis in Python. *Executing* the command and seeing whether it reaches +# the file is not, so the two claims are separate tests rather than one test +# with a conditional assertion. Everything the guard decides is checked +# everywhere; only the premise about what a shell does is POSIX-only. +needs_posix_shell = pytest.mark.skipif( + not POSIX_SHELL, + reason=( + "executes the attack command and asserts a real shell reaches the " + "file: a claim about POSIX path and glob semantics, not about the " + "guard, which is checked on every platform by the deny tests" + ), +) + +# The sample the default run checks. Strided rather than random so a +# failure names the same member on every machine. +_SAMPLE_STRIDE = 23 + + +def _bash_guard_command() -> str: + from mureo.credential_guard import bash_guard_entry + + return str(bash_guard_entry()["hooks"][0]["command"]) + + +def _denies(command: str, home: Path) -> bool: + proc = run_guard_in_shell(_bash_guard_command(), {"command": command}, home) + assert proc.returncode == 0, proc.stderr + return deny_decision(proc) == "deny" + + +@needs_shell +@pytest.mark.unit +class TestProductSample: + def test_sample_of_the_product_is_denied(self, tmp_path: Path) -> None: + """Every strided member of the product must deny. + + Each is a spelling of ``~/.mureo/credentials.json`` assembled from + a parent form, a way of breaking the name, a filling for that form, + a nesting depth and a position. None of them contains the six + characters of the directory name consecutively unless the ``none`` + break was chosen. + """ + home = build_home(tmp_path) + sample = members()[::_SAMPLE_STRIDE] + assert len(sample) > 100, "the product shrank; check the axes" + missed = [label for label, cmd in sample if not _denies(cmd, home)] + assert not missed, f"{len(missed)} of {len(sample)} allowed: {missed[:5]}" + + def test_the_axes_are_all_represented(self) -> None: + """A guard against an axis quietly dropping out of the product.""" + labels = [label for label, _ in members()] + for axis in ("$VAR", "backtick", "continuation", "brace here", "sequence"): + assert any(axis in label for label in labels), axis + for depth in ("d0", "d3", "d11", "d20"): + assert any(label.endswith(depth) for label in labels), depth + + +@pytest.mark.unit +class TestSubprocessContracts: + """The environment handed to a subprocess must be strings, everywhere. + + POSIX accepts a ``Path`` in ``env`` and Windows raises ``TypeError: + environment can only contain strings``. That difference turned an + annotation nobody enforced into 61 failures on the first Windows run, + so the contract is pinned here rather than left to the platform to + discover. These run on every platform precisely because the lenient + one is where the mistake gets made. + """ + + def test_reads_marker_passes_only_strings_to_the_environment( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from tests import credential_guard_product as product + + seen: dict[str, object] = {} + + def fake_run( + *args: object, **kwargs: object + ) -> subprocess.CompletedProcess[str]: + seen.update(kwargs) + return subprocess.CompletedProcess( + args=[], returncode=0, stdout="", stderr="" + ) + + # Patch the shared module object both helpers import. + monkeypatch.setattr(subprocess, "run", fake_run) + product.reads_marker("echo hi", build_home(tmp_path), "bash") + + env = seen["env"] + assert isinstance(env, dict) + bad = {k: v for k, v in env.items() if not isinstance(v, str)} + assert not bad, f"non-string environment values: {bad}" + assert isinstance(seen["cwd"], str) + # Inherited rather than hand-built: a minimal env drops variables a + # shell needs to start, which looks like "the command read nothing". + assert len(env) > 2, "the environment should extend os.environ" + + def test_the_runner_passes_only_strings_to_the_environment( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from tests import hook_guard_runner as runner + + seen: dict[str, object] = {} + + def fake_run( + *args: object, **kwargs: object + ) -> subprocess.CompletedProcess[str]: + seen.update(kwargs) + return subprocess.CompletedProcess( + args=[], returncode=0, stdout="", stderr="" + ) + + monkeypatch.setattr(subprocess, "run", fake_run) + runner.run_guard(_bash_guard_command(), {"command": "echo hi"}, tmp_path) + + env = seen["env"] + assert isinstance(env, dict) + bad = {k: v for k, v in env.items() if not isinstance(v, str)} + assert not bad, f"non-string environment values: {bad}" + + +@pytest.fixture +def only_when_asked_for(request: pytest.FixtureRequest) -> None: + """Run only when ``slow`` was selected, so a plain ``pytest`` skips it. + + Expressed here rather than as a global ``addopts`` filter: a marker that + silently disappears from the default run is how a suite ends up with + checks nobody has executed in months. + """ + if "slow" not in str(request.config.getoption("markexpr")): + pytest.skip("exhaustive; run with: pytest -m slow") + + +@needs_shell +@needs_posix_shell +@pytest.mark.slow +class TestWholeProduct: + @pytest.mark.usefixtures("only_when_asked_for") + def test_every_member_reads_the_file_and_is_denied(self, tmp_path: Path) -> None: + """The claim the module docstring makes, in full. + + Both halves matter. That every member is denied is the guarantee; + that every member really reads the marker is what stops the product + quietly filling up with commands that prove nothing. + """ + home = build_home(tmp_path) + all_members = members() + inert = [ + label + for label, cmd in all_members + if not reads_marker(cmd, home, BASH or "bash") + ] + missed = [label for label, cmd in all_members if not _denies(cmd, home)] + assert not inert, f"{len(inert)} members do not reach the file: {inert[:5]}" + assert not missed, f"{len(missed)} allowed: {missed[:5]}" + + +@needs_shell +@pytest.mark.unit +class TestNestingDepth: + """The expansion budget's cliff, which cost a round on its own. + + Past the budget the old code left literal braces in the candidates — + ordinary characters to fnmatch — so neither rule fired. Unresolved + structure now denies on its own, and this is the table that says so. + """ + + @staticmethod + def _nested(depth: int, alts: int) -> str: + inner = "mureo" + for level in range(1, depth + 1): + filler = ",".join(f"z{level}x{k}" for k in range(alts - 1)) + inner = "{" + filler + "," + inner + "}" + return "cat ~/." + inner + "/credentials.json" + + @pytest.mark.parametrize("alts", [2, 3, 5]) + @pytest.mark.parametrize("depth", list(range(1, 21))) + def test_denies_at_every_depth(self, tmp_path: Path, depth: int, alts: int) -> None: + """The guarantee: every cell denies. Runs everywhere.""" + home = build_home(tmp_path) + assert _denies(self._nested(depth, alts), home) + + @needs_posix_shell + @pytest.mark.parametrize("alts", [2, 3, 5]) + @pytest.mark.parametrize("depth", list(range(1, 21))) + def test_every_depth_really_reaches_the_file( + self, tmp_path: Path, depth: int, alts: int + ) -> None: + """The premise: every cell is a command that reads the file. + + Without this the table above could fill up with commands that deny + because they are nonsense rather than because the guard works. + """ + home = build_home(tmp_path) + command = self._nested(depth, alts) + assert reads_marker(command, home, BASH or "bash"), command + + +@needs_shell +@pytest.mark.unit +class TestResourceBounds: + """A guard that is merely slow is a guard that is bypassed. + + ``sys.excepthook`` cannot catch the host killing an overrunning hook, + and that process exits non-zero without printing the deny JSON — the + non-blocking case, where the tool call proceeds. Oversized input is + therefore refused before any of the analysis runs. + """ + + @staticmethod + def _decide(command: str, seconds: float) -> str: + payload = json.dumps({"tool_name": "Bash", "tool_input": {"command": command}}) + proc = subprocess.run( + [BASH or "bash", "-c", _bash_guard_command()], + input=payload, + capture_output=True, + text=True, + timeout=seconds, + ) + assert proc.returncode == 0, proc.stderr[:400] + return "deny" if proc.stdout.strip() else "allow" + + @pytest.mark.parametrize( + ("label", "alt_len", "alts", "groups"), + [ + ("modest", 100, 4, 2), + ("wide alternatives", 8000, 64, 4), + ("wide and repeated", 8000, 64, 8), + ("multi-megabyte", 16000, 64, 8), + ], + ) + def test_expansion_bombs_are_answered_quickly( + self, label: str, alt_len: int, alts: int, groups: int + ) -> None: + body = ",".join(["z" * alt_len] * (alts - 1) + ["mureo"]) + command = ( + "cat ~/." + "".join("{" + body + "}" for _ in range(groups)) + "/creds" + ) + # A generous ceiling: the point is that it answers at all, in time + # the host will not kill it. Locally these run in ~0.2s. + assert self._decide(command, seconds=15) == "deny", label + + def test_oversized_commands_are_refused_at_the_boundary(self) -> None: + """Nothing legitimate approaches 64 KB, and past it the guard has + not read the command, so it cannot say the command is safe.""" + assert self._decide("echo " + "z" * (65536 - 5), seconds=15) == "allow" + assert self._decide("echo " + "z" * (65537 - 5), seconds=15) == "deny"