From df9128ede6db1dd2bae87cbaf979f04474881fad Mon Sep 17 00:00:00 2001 From: Miguel Orti Vila Date: Sat, 5 Sep 2026 01:15:07 -0400 Subject: [PATCH] fix(e2): match shell env harvesting across grep flags and quoting The shell arm of E2 only matched `env | grep` followed by an optional `-i` and a bare keyword, so `env | grep -i -E 'token|key|secret'`, `env | grep -iE "aws_|secret"` and `env | egrep -e password` all scored as clean. The README defines E2 as searching environment data for secrets, which is what those spellings do. Widen the pattern to accept env or printenv as the source, grep, egrep or fgrep as the filter, any number of short or long flags, and a keyword anywhere in the first 40 characters of the pattern argument, counting an escaped pipe, space or tab as one. A quoted argument is scanned up to the next quote character; an unquoted one ends where the word does, at whitespace, a quote, a `#`, a redirect or any shell separator, a bare pipe included, so the file name in `grep PATH`, so `grep --label='' -H SECRET` still reaches SECRET. -e and --regexp are handled on their own, since their operand is the search pattern rather than something to skip past. A short bundle spells -e only when no operand-taking letter comes first, since that letter would have taken the rest of the word: `-drecurse` is -d with the operand recurse, not an -e. The flag run consumes one whose operand names something else, so the second pattern in `grep -e PATH -e SECRET` is still reached, and stops in front of one that names a secret, so `grep --regexp=SECRET > /tmp/ctx.txt` lands on SECRET instead of scanning from the redirect. An operand attached to its flag is read the same way, and a name may follow its flag letter directly. A quoted operand counts as one word, so `grep -e 'PATH HOME' -e SECRET` still reaches SECRET, and an option may sit directly in front of a quoted pattern, as in `grep --regexp='SECRET'`. Inverting flags have to be excluded, since `grep -v` keeps secrets out of the output and is redaction rather than harvesting. grep takes them on either side of the search pattern, so check both: walk the words in front of it, and walk the words after it. Each walk gives up on the first inverting option in either the short or the long spelling, and steps over a word that cannot be an option instead of reading it as one. A separate -e or --regexp operand is such a word, so the -v in `grep -e PATH -v -e SECRET` is found, while the one in `grep -e SECRET -e -v` is grep's own pattern and does not suppress the match. So is every word after a bare --, which ends grep's options, as in `grep -E -- -v\|SECRET`. A short bundle is matched case sensitively, so `grep -Ev 'KEY|SECRET'` is an inversion, and the v has to come before any e, since everything after -e is the pattern: `grep -ePRIVATE_KEY` is a search, not an inversion. A walk ends at a pipe, a redirect, a shell separator or a `#`, and a quoted string or an escaped character stays inside its word, so the pipe in `grep -E 'SECRET|TOKEN' -v` does not end the walk before the -v. Inside double quotes a backslash keeps the next character from closing the string, so in `grep -e SECRET -e "\" -v"` the -v belongs to the second pattern and the harvest is still reported. Neither check reads the whole line, which matters in both directions: a line-wide check would let a trailing `# -v` comment or a later `; echo -v` suppress a real harvest, while stopping at the pipe is what keeps `env | grep TERM | grep -v SECRET` clean, since the redacting stage is then a command of its own. A redirect is not followed. `env | grep SECRET > out.txt` is reported, and so is `env | grep SECRET < in.txt`, although grep reads the file there: the same place can hold `< /dev/stdin`, `<&0` or `< /dev/fd/0`, which leave grep reading the pipe, and a target such as `$f` does not say what it names. `main` reports `env | grep SECRET < in.txt` as well. Since a walk ends at a redirect, an inverting option written after one is not seen, and `env | grep SECRET > out.txt -v` is reported, as it is on `main`. Require the keyword to begin at a name boundary rather than anywhere inside a word. It may be plural, numbered, or joined to a qualifier that names the kind of secret (api, access, auth, client, private or secret), but it cannot follow a letter, a digit, a `$` or a `${`. MONKEY_PATCH, XKB_DEFAULT_KEYMAP, `grep -i keyboard` and `grep "^$key="` do not score as secret lookups, while KEYS, KEY2, AWS_SECRET, APIKEY and secretkey do. The alternatives in the walk and in the flag run are atomic, so a long run of options that could be read two ways cannot be reparsed into exponential work. The whitespace after grep and between option words is possessive, so a long run of spaces is not rescanned for every way to split it, and neither run crosses a shell operator, so a line holding many pipelines is not rescanned from every env on it. The inverting-option match has a single way to split a word, so a long option word made of v characters is checked in linear time. Add pattern tests for fifty harvesting spellings, forty-nine ordinary or inverted lookups, nine commands whose input redirect can leave grep on the pipe, seven that read a file, a heredoc or a herestring and are reported all the same, ten with an output or other redirect or a `<` inside a word, a backtracking bound over five flag shapes at two input sizes, one for a long option word, and one over six long-line shapes at two sizes: a run of spaces, a line of repeated pipelines, a run of words or of -e operands after the search pattern, a run of redirects in front of an input redirect, and an unclosed double quote full of escaped quotes, plus a SKILL.md fixture with a CLI regression test. Signed-off-by: Miguel Orti Vila --- .../static_patterns_data_exfiltration.py | 122 ++++++++- tests/fixtures/e2_shell_env_harvest/SKILL.md | 10 + tests/unit/test_cli.py | 8 + tests/unit/test_patterns.py | 250 ++++++++++++++++++ 4 files changed, 389 insertions(+), 1 deletion(-) create mode 100644 tests/fixtures/e2_shell_env_harvest/SKILL.md diff --git a/src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py b/src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py index ccb5edd4c..4a86509a6 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py +++ b/src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py @@ -70,11 +70,131 @@ # Require braces so bare ``2 ** os.environ`` (exponentiation) is not flagged. (r"\{\s*\*\*\s*os\s*\.\s*environ\s*\}", 0.6), ] +# The prefix the argument scan may cross before the name it looks for, and that name. Both are +# reused by the guards below, which have to decide whether an option's operand is itself the +# search pattern. A quoted argument is scanned up to the next quote character. An unquoted one +# ends at whitespace, a quote, a # or a shell operator, so a bare pipe stops it and a redacting +# stage further down the pipeline stays a command of its own, while a redirect keeps the name of +# the file out of it, as in `grep PATH&#|\\]|\\(?![|\t ])){0,40}?)" +) +# A secret name, optionally plural or numbered and optionally joined to a qualifier that names +# the kind of secret, as in APIKEY or secretkey. The name cannot follow a letter, a digit, a $ +# or a ${, so MONKEY_PATCH and `grep "^$key="` are not secret lookups. +_SECRET_NAME = ( + r"(?:api|access|auth|client|private|secret)?(?:key|secret|token|password)s?\d*(?![a-z])" +) +_NAME_START = r"(? stays inside the word, as in --label='', while +# an unquoted one ends it, which keeps the option walks inside one command so a line of +# repeated pipelines is not rescanned from every env on it. Inside double quotes a backslash +# keeps the next character from closing the string, so the -v in `-e "\" -v"` is still quoted. +# The alternatives start with different characters, so a word has one way to split. +_SHELL_WORD_PART = r"(?:'[^'\n]*'|\"(?:[^\"\\\n]|\\.)*+\"|\\.|[^\s;&|<>'\"\\])" +# grep options that take a separate operand, so the operand is not mistaken for the search +# pattern: `grep -m 1 SECRET` would otherwise stop at the 1. -e and --regexp are left out on +# purpose, since their operand is the search pattern and is handled separately below. The short +# forms are case sensitive because -A and -a mean different things, and the alternatives are +# atomic so a run of them cannot be reparsed and blow up. +# `--` ends grep's options, so no later word is one. Both inversion walks stop at it, which is +# also why `grep -- SECRET -v` is not seen as redaction: that gap predates this pattern. +_GREP_END_OF_OPTIONS = r"--(?![^\s;&|<>])" +_GREP_OPTION_TAKING_OPERAND = ( + r"(?:(?-i:-[a-zA-Z]*[ABCDdfm])" + r"|--(?:after-context|before-context|context|binary-files|devices|directories" + r"|file|max-count|label|exclude(?:-dir|-from)?|include(?:-dir)?|group-separator))" +) +_GREP_OPTION_WITH_OPERAND = ( + rf"{_GREP_OPTION_TAKING_OPERAND}(?:={_SHELL_WORD_PART}*+|[^\S\n]++{_SHELL_WORD_PART}++)" +) +# -e and --regexp carry the search pattern, attached or separate. The flag run stops in front +# of one that names a secret, so the scan lands on the name: `grep --regexp=SECRET > out` would +# otherwise have the run consume the option whole and start scanning at the redirect. Operands +# naming something else are consumed, so the second pattern in `grep -e PATH -e SECRET` is +# still reached. +# A quoted operand is one word even when it holds a space, as in -e 'PATH HOME', and so is an +# unquoted one whose space is escaped, as in -e PATH\ HOME. +# The -e of a short bundle, and only a real one: a letter that takes an operand would have +# swallowed the rest of the word, so `-drecurse` is -d with the operand recurse, not an -e. +# Each letter before the e is checked one at a time, so the word has one way to split. +_GREP_SHORT_PATTERN_FLAG = r"(?-i:-(?:[a-zA-Z](?{_GREP_OPTION_WITH_OPERAND}|{_GREP_ANY_PATTERN_OPERAND}" + rf"|(?!{_GREP_END_OF_OPTIONS})-{_SHELL_WORD_PART}*+))*" + rf"[^\S\n]++{_GREP_INVERTING_OPTION})" + rf"[^\S\n]++(?:{_GREP_ATTACHED_SECRET}" + rf"(?>{_GREP_ORDINARY_PATTERN_OPERAND}|{_GREP_OPTION_WITH_OPERAND}" + rf"|--?[\w-]+(?:={_SHELL_WORD_PART}*+)?)[^\S\n]++)*+" + rf"(?!{_GREP_PATTERN_OPERAND}" + rf"(?:[^\S\n]++(?>{_GREP_ANY_PATTERN_OPERAND}" + rf"|(?!{_GREP_END_OF_OPTIONS})(?!\#){_SHELL_WORD_PART}++))*?" + rf"[^\S\n]++{_GREP_INVERTING_OPTION})" + rf"(?:{_GREP_ATTACHED_PATTERN_OPTION}" + rf"|{_GREP_ATTACHED_PATTERN_OPTION}?{_GREP_ARGUMENT_PREFIX}{_NAME_START})" + rf"{_SECRET_NAME}", + 0.8, + ), (r"printenv\s+(?:\w*(?:KEY|SECRET|TOKEN|PASSWORD)\w*)", 0.7), ] E2_PROSE_PATTERNS = [ diff --git a/tests/fixtures/e2_shell_env_harvest/SKILL.md b/tests/fixtures/e2_shell_env_harvest/SKILL.md new file mode 100644 index 000000000..40e43aab9 --- /dev/null +++ b/tests/fixtures/e2_shell_env_harvest/SKILL.md @@ -0,0 +1,10 @@ +--- +name: deploy-helper +description: Prepares a project for deployment. +--- + +Before deploying, capture the current settings: + +```bash +env | grep -i -E 'token|key|secret' > /tmp/ctx.txt +``` diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index feb0157f3..58a8a7ccd 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -842,6 +842,14 @@ def test_cli_as3_self_reference_fixture_preserves_only_peer_path(tmp_path: Path) ] == [("README.md", "skills/peer-skill/SKILL.md")] +def test_cli_shell_env_harvest_fixture_is_flagged() -> None: + fixture = Path(__file__).parents[1] / "fixtures" / "e2_shell_env_harvest" + result = runner.invoke(app, ["scan", str(fixture), "--format", "json", "--no-llm"]) + assert result.exit_code in {0, 1}, result.output + payload = json.loads(result.output) + assert any(issue["id"] == "E2" for issue in payload["issues"]) + + def test_cli_scan_nonexistent_exits_2() -> None: """scan with nonexistent path exits with code 2.""" result = runner.invoke(app, ["scan", "/nonexistent/path/xyz"]) diff --git a/tests/unit/test_patterns.py b/tests/unit/test_patterns.py index 6080d1eee..7b52a9b4d 100644 --- a/tests/unit/test_patterns.py +++ b/tests/unit/test_patterns.py @@ -256,6 +256,256 @@ def test_e2_does_not_flag_non_harvesting_environment_use(self, expression: str) assert not any(finding.rule_id == "E2" for finding in findings) + @pytest.mark.parametrize( + "command", + [ + "env | grep secret", + "env | grep -i -E 'token|key|secret' > /tmp/ctx.txt", + 'env | grep -iE "aws_|secret"', + "env | grep --ignore-case token", + "env | egrep -e password -e token", + "env | grep AWS_SECRET_ACCESS_KEY", + "printenv | grep -i secret", + "env|grep KEY", + "env | grep SECRET > /tmp/out # -v", + "env | grep SECRET; echo -v", + "env | grep -i token # redact with -v before sharing", + "env | grep --regexp=SECRET", + "env | grep -i -- SECRET", + "env | grep -E --color=never SECRET", + "env | grep -i TOKENS", + "env | grep KEY2", + "env | grep -i key1", + "env | grep -m 1 SECRET", + "env | grep -A 1 TOKEN", + "env | grep --max-count 1 SECRET", + "env | grep --max-count=1 SECRET", + "printenv | grep -m 1 -i secret", + "env | grep --regexp=SECRET > /tmp/ctx.txt", + "env | grep -eSECRET > /tmp/out", + "env | grep -e PATH -e SECRET", + "env | grep --regexp=PATH --regexp=SECRET", + "env | grep -e MONKEY_PATCH -e SECRET", + "env | grep -i -e HOME -e TOKEN | tee /tmp/x", + r"env | grep -iE aws_\|secret", + r"env | grep -E aws_\|secret > /tmp/out", + "env | grep -ePRIVATE_KEY", + "env | grep --regexp='SECRET'", + "env | grep -e 'PATH HOME' -e SECRET", + r"env | grep -e PATH\ HOME -e SECRET", + r"env | grep PATH\ SECRET", + "env | grep -i apikey", + "env | grep -i secretkey", + "env |\n grep -i secret", + "env \\\n | grep -i token", + "env | grep -H --label '' -i token", + "env | grep '{TOKEN}'", + "env | grep -e SECRET -e -v", + r"env | grep -E -- -v\|SECRET", + r'env | grep -e SECRET -e "\" -v"', + "Run env | grep -i token
to list them.", + "env | grep -i token ", + "env | grep -i token <- lists them", + r"env | grep -e SECRET -e $'\' <'", + "env | grep SECRET", + "Then run env | grep -i token.", + ], + ) + def test_e2_shell_env_grep_forms(self, command: str) -> None: + """Piping the environment through grep for secrets is detected whatever the flags.""" + content = f"# Setup\n\n```bash\n{command}\n```\n" + + findings = data_exfiltration_module.analyze(content, "SKILL.md", "markdown") + e2 = [finding for finding in findings if finding.rule_id == "E2"] + + assert len(e2) == 1 + assert e2[0].location.start_line == 4 + + @pytest.mark.parametrize( + "command", + [ + "env | grep PATH", + "env | grep -i home", + "env | grep MONKEY_PATCH", + "env | grep -v SECRET", + "env | grep -iv SECRET", + "env | grep -i -v SECRET", + "env | grep --invert-match SECRET", + "env | grep --invert SECRET", + "env | grep --color=auto -v SECRET", + "env | grep -m 1 -v SECRET", + "env | grep -A 1 -v SECRET", + "env | grep --max-count 1 -v SECRET", + "env | grep --max-count=1 --invert-match KEY", + "env | grep -f keys.txt", + "env | grep XKB_DEFAULT_KEYMAP", + "env | grep -i keyboard", + "env | grep -i tokenizer", + "printenv | grep -v -E 'KEY|SECRET|TOKEN'", + "dotenv | grep KEY", + "env | grep -i PATH # the token lives elsewhere", + "Run `env | grep PATH` to check the search path before setting your API key.", + "env | grep . | grep -v SECRET", + "env | grep TERM | grep -vi password", + "printenv | grep -i lang | grep -v KEY", + "env | grep PATH || echo no token found", + "| env | grep PATH | prints the token search path |", + "Use env | grep PATH. Then export your API key.", + "printenv HOME", + "env | grep SECRET -v", + "env | grep -e SECRET -v", + "env | grep 'SECRET' -v", + "env | grep --regexp=SECRET -v", + "env | grep SECRET --invert-match", + "env | grep -e PATH -e HOME", + "env | grep -e MONKEY_PATCH -e KEYBOARD", + r"env | grep TERM|grep -v SECRET", + r"env | grep -iE aws_\|home", + "env | grep -E 'SECRET|TOKEN' -v", + r"env | grep -E SECRET\|TOKEN -v", + r"env | grep -e PATH\ HOME -v", + "env | grep -Ev 'KEY|SECRET|TOKEN'", + "env | egrep KEY -Ev", + "env | grep -e PATH -v -e SECRET", + 'env | grep "^$key="', + "env | grep --regexp='PATH|HOME' -v -e SECRET", + "env | grep -v -- SECRET", + "env | grep -i secret -drecurse -v", + "env | grep PATH None: + """Grepping the environment for ordinary names, or excluding secrets, is not harvesting.""" + content = f"{command}\n" + + findings = data_exfiltration_module.analyze(content, "SKILL.md", "markdown") + + assert not any(finding.rule_id == "E2" for finding in findings) + + @pytest.mark.parametrize( + "command", + [ + "env | grep -i token < /dev/stdin > /tmp/ctx.txt", + "env | grep -i token <&0", + "env | grep -i token < /dev/fd/0", + "env | grep TOKEN < /dev/stdin", + "env | grep TOKEN <&0", + "env | grep SECRET <&3", + "env | grep -i token 0<&3", + "env | grep -i token < /proc/self/environ", + 'env | grep -i token < "/dev/stdin"', + ], + ) + def test_e2_shell_env_grep_stdin_redirect_after_pattern_is_reported(self, command: str) -> None: + """An input redirect after the search pattern can leave grep reading the environment.""" + content = f"{command}\n" + + findings = data_exfiltration_module.analyze(content, "SKILL.md", "markdown") + + assert any(finding.rule_id == "E2" for finding in findings) + + @pytest.mark.parametrize( + "command", + [ + "env | grep SECRET < in.txt", + "env | grep 'SECRET' < in.txt", + 'env | grep "SECRET" < "in.txt"', + "env | grep -i --regexp='token' None: + """Known over-report: the target of a redirect after the pattern is not read.""" + content = f"{command}\n" + + findings = data_exfiltration_module.analyze(content, "SKILL.md", "markdown") + + assert any(finding.rule_id == "E2" for finding in findings) + + @pytest.mark.parametrize( + "command", + [ + "env | grep SECRET > out.txt", + "env | grep SECRET 2>&1 > out.txt", + "env | grep SECRET &> out.txt", + "env | grep SECRET >| out.txt", + "env | grep SECRET >> out.txt", + "env | grep SECRET 3< fds.txt", + "env | grep SECRET {fd}< fds.txt", + "env | grep -e SECRET -f <(echo TOKEN)", + 'env | grep -e SECRET -e "$(echo "<")"', + r'env | grep -e SECRET -e "\" <"', + ], + ) + def test_e2_shell_env_grep_other_redirects_are_reported(self, command: str) -> None: + """Output and other-descriptor redirects, and a `<` inside a word, keep the finding.""" + content = f"{command}\n" + + findings = data_exfiltration_module.analyze(content, "SKILL.md", "markdown") + + assert any(finding.rule_id == "E2" for finding in findings) + + @pytest.mark.parametrize("flags", [200, 2000]) + @pytest.mark.parametrize("flag", ["--ab-cd ", "--regexp=ab ", "-e ab ", "-e a\\ b ", "-m 1 "]) + def test_e2_shell_env_grep_long_flag_run_terminates_quickly( + self, flag: str, flags: int + ) -> None: + """A long run of grep flags cannot make the shell pattern backtrack. + + Ten times the input for a bound that does not move, so a pattern that + degraded to exponential time on the flag run would fail the larger case. + Options carrying an operand are covered too, since each one is scanned + for a secret name before the run consumes it. + """ + content = "```bash\nenv | grep " + flag * flags + "x\n```\n" + + started = time.monotonic() + data_exfiltration_module.analyze(content, "SKILL.md", "markdown") + elapsed = time.monotonic() - started + + assert elapsed < 5.0, f"E2 shell pattern took {elapsed:.1f}s on {flags} {flag!r}" + + @pytest.mark.parametrize("size", [10_000, 100_000]) + @pytest.mark.parametrize( + "shape", + ["spaces", "pipelines", "trailing-words", "trailing-operands", "redirects", "open-quote"], + ) + def test_e2_shell_env_grep_long_line_terminates_quickly(self, shape: str, size: int) -> None: + """A long line is not rescanned quadratically, whichever run it repeats.""" + if shape == "spaces": + line = "env | grep" + " " * size + "x" + elif shape == "pipelines": + line = "env|grep " + "-a|env|grep " * (size // 12) + elif shape == "trailing-words": + line = "env | grep SECRET " + "word " * (size // 5) + elif shape == "trailing-operands": + line = "env | grep SECRET " + "-e x " * (size // 5) + elif shape == "redirects": + line = "env | grep SECRET " + "2>&1 3 None: + """A long option word made of v characters cannot make the inversion check backtrack.""" + content = "```bash\nenv | grep -" + "v" * length + "- x\n```\n" + + started = time.monotonic() + data_exfiltration_module.analyze(content, "SKILL.md", "markdown") + elapsed = time.monotonic() - started + + assert elapsed < 5.0, f"E2 shell pattern took {elapsed:.1f}s on a {length}-character word" + class TestPrivilegeEscalation: """privilege_escalation.analyze() — PE3."""