fix(approval): a quoted $(...)/backtick body keeps its command boundaries when newlines are masked; a backtick operand is not a closer
Independent review of the first fix found two defects. Bounding the grep lexer removed the old "malformed payload" floor, and that floor had been the ONLY thing blocking a hardline command hidden after a newline inside a double-quoted $(grep ...) substitution: _mask_quoted_newlines treated the newline as quoted DATA and replaced it with a space, so the later command-start pass saw `/dev/null reboot` as an operand and the public guards approved a hardline command with no callback. A substitution body inside double quotes is executable; the masker now recurses into $(...) and backtick bodies with a fresh quote state so a newline there stays a command boundary. The witness now blocks as "system shutdown/reboot" (the correct verdict), as do the ; / && / | / nested / backtick variants. Second: the lexer stopped at ANY backtick as if it closed an enclosing substitution, so a grep with a backtick OPERAND lost its operands and was reported malformed (a new false positive). A backtick opened after the grep is an operand substitution; only an unmatched one closes the enclosing command. Tests: the six hardline-inside-substitution shapes block as themselves; the public guard blocks with zero approval callbacks; the backtick operand, a data pattern and a quoted-newline commit message stay allowed. Approval/hardline suites: 637 passed; the one failure (real-binary sort payload probe) fails identically on main.
This commit is contained in:
@@ -39,3 +39,37 @@ def test_genuinely_unbalanced_quoting_still_fails_closed():
|
||||
def test_hardline_patterns_unchanged():
|
||||
for command in ("rm -rf /", 'echo "$(rm -rf /)"', "sudo shutdown -h now"):
|
||||
assert detect_hardline_command(command)[0] is True, command
|
||||
|
||||
|
||||
class TestExecutableSubstitutionBodiesStayExecutable:
|
||||
"""Bounding the grep lexer removed the old 'malformed' floor; the newline masker must then treat a
|
||||
substitution body inside double quotes as CODE, or a newline-separated hardline command hides as an
|
||||
operand (independent review witness: blocked on main as 'malformed', approved on the first fix)."""
|
||||
|
||||
@pytest.mark.parametrize("cmd", [
|
||||
'echo "$(grep -P \'safe\' /dev/null\nreboot)"',
|
||||
'echo "$(grep x f; reboot)"',
|
||||
'echo "$(grep x f && reboot)"',
|
||||
'echo "$(grep x f | shutdown -h now)"',
|
||||
'echo "$(echo $(grep x f)\nreboot)"',
|
||||
'echo "`grep x f\nreboot`"',
|
||||
])
|
||||
def test_hardline_command_inside_a_quoted_substitution_blocks_as_itself(self, cmd):
|
||||
blocked, reason = detect_hardline_command(cmd)
|
||||
assert blocked and reason == "system shutdown/reboot"
|
||||
|
||||
def test_public_guard_blocks_without_offering_approval(self, tmp_path, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hh"))
|
||||
from tools.approval import check_dangerous_command
|
||||
calls = []
|
||||
result = check_dangerous_command('echo "$(grep -P \'safe\' /dev/null\nreboot)"', "local",
|
||||
approval_callback=lambda *a, **k: calls.append(a) or False)
|
||||
assert result["approved"] is False and "hardline" in result["message"].lower() and calls == []
|
||||
|
||||
@pytest.mark.parametrize("cmd", [
|
||||
"grep -e `echo needle` file", # backtick OPERAND (opened after the grep) is not a closer
|
||||
"grep -F 'sudo reboot' notes.md", # data pattern
|
||||
'git commit -m "fix\nsudo reboot handling"', # quoted data newline
|
||||
])
|
||||
def test_benign_shapes_stay_allowed(self, cmd):
|
||||
assert detect_hardline_command(cmd) == (False, None)
|
||||
|
||||
@@ -578,6 +578,14 @@ def _command_parser_limit_exceeded(command: str) -> bool:
|
||||
return sum(command.count(char) for char in ";&|\n") >= _MAX_DETECTION_SEGMENTS
|
||||
|
||||
|
||||
def _backtick_end_from(segment: str, i: int) -> int | None:
|
||||
"""Index of the backtick closing the one opened at ``i``, or None when none follows."""
|
||||
j = segment.find("`", i + 1)
|
||||
while j != -1 and segment[j - 1] == "\\":
|
||||
j = segment.find("`", j + 1)
|
||||
return None if j == -1 else j
|
||||
|
||||
|
||||
def _shell_tokens_with_spans(segment: str, start: int):
|
||||
"""Return shell words as ``(value, start, end, quoted)`` or ``None`` on malformed quoting.
|
||||
Deliberately small lexer that never expands shell syntax; it exists to keep source spans (which
|
||||
@@ -591,6 +599,9 @@ def _shell_tokens_with_spans(segment: str, start: int):
|
||||
is the canonical shape)."""
|
||||
tokens, value, token_start, quote = [], [], None, None
|
||||
depth = 0 # $(...) nesting opened AFTER start; a closer at depth 0 ends the enclosing substitution
|
||||
# A backtick opened AFTER start is an operand substitution (``grep -e `cmd` f``); the matching
|
||||
# closer belongs to it, not to an enclosing backtick the command might sit inside.
|
||||
in_backtick = False
|
||||
|
||||
def flush(end: int) -> None:
|
||||
raw = segment[token_start:end]
|
||||
@@ -610,7 +621,15 @@ def _shell_tokens_with_spans(segment: str, start: int):
|
||||
continue
|
||||
if segment.startswith("$(", i):
|
||||
depth += 1
|
||||
elif ch == ")" or ch == "`":
|
||||
elif ch == "`":
|
||||
if in_backtick:
|
||||
in_backtick = False
|
||||
elif depth == 0 and _backtick_end_from(segment, i) is None:
|
||||
end_at = i # unmatched: it closes the substitution this command sits inside
|
||||
break
|
||||
else:
|
||||
in_backtick = True
|
||||
elif ch == ")":
|
||||
if depth == 0:
|
||||
end_at = i
|
||||
break
|
||||
@@ -1042,10 +1061,29 @@ def _mask_quoted_newlines(command: str) -> str:
|
||||
exactly as the shell would, so masking them cannot hide a runnable command."""
|
||||
if "\n" not in command:
|
||||
return command
|
||||
return "".join(
|
||||
" " if quote and kind == "char" and command[i] == "\n" else command[i:j]
|
||||
for kind, i, j, quote in _scan_shell(command)
|
||||
)
|
||||
return _mask_quoted_newlines_span(command, 0, len(command))
|
||||
|
||||
|
||||
def _mask_quoted_newlines_span(command: str, start: int, end: int) -> str:
|
||||
"""``_mask_quoted_newlines`` over ``command[start:end]``. A ``$(...)`` / backtick substitution
|
||||
inside double quotes is EXECUTABLE, not data: its body is re-scanned with a fresh quote state so a
|
||||
newline that separates commands inside it survives as a command boundary. Masking it as quoted
|
||||
data turned ``"$(grep x f\nreboot)"`` into ``... f reboot)``, which no later stage can tell from an
|
||||
operand; the pre-fix scanner only caught it by refusing the whole command as malformed."""
|
||||
out: list[str] = []
|
||||
for kind, i, j, quote in _scan_shell(command, start, end, subst="q"):
|
||||
if kind == "subst":
|
||||
# j is the index just past the closer; keep the opener and closer, recurse into the body.
|
||||
body_start = i + (2 if command.startswith("$(", i) else 1)
|
||||
body_end = j - 1
|
||||
out.append(command[i:body_start])
|
||||
out.append(_mask_quoted_newlines_span(command, body_start, body_end))
|
||||
out.append(command[body_end:j])
|
||||
elif quote and kind == "char" and command[i] == "\n":
|
||||
out.append(" ")
|
||||
else:
|
||||
out.append(command[i:j])
|
||||
return "".join(out)
|
||||
|
||||
|
||||
def _iter_shell_command_word_spans(command: str):
|
||||
|
||||
Reference in New Issue
Block a user