From bbca32fea9a39252a1e14d0496ee646e4dd1c25a Mon Sep 17 00:00:00 2001 From: michaelxer Date: Sat, 27 Jun 2026 05:31:54 +0700 Subject: [PATCH] fix(security): validate each shell segment against exec.allowPatterns Guard against shell-chain bypass where an attacker appends '&& malicious' after an allowlisted prefix. The allowlist check now splits the command on top-level chaining operators (&&, ||, ;, |) and requires every segment to match at least one allowPattern independently. Fixes #4521 --- nanobot/agent/tools/shell.py | 82 ++++++++++++++++++++++++- tests/tools/test_exec_allow_patterns.py | 44 +++++++++---- 2 files changed, 112 insertions(+), 14 deletions(-) diff --git a/nanobot/agent/tools/shell.py b/nanobot/agent/tools/shell.py index 9fac8725..26fb9fa9 100644 --- a/nanobot/agent/tools/shell.py +++ b/nanobot/agent/tools/shell.py @@ -755,9 +755,12 @@ class ExecTool(Tool): # allow_patterns take priority over deny_patterns so that users can # exempt specific commands (e.g. "rm -rf" inside a build directory) - # from the hardcoded deny list via configuration. - explicitly_allowed = bool(self.allow_patterns) and any( - re.fullmatch(p, lower) for p in self.allow_patterns + # from the hardcoded deny list via configuration. A chained command is + # only explicitly allowed when every top-level shell segment matches. + segments = self._split_shell_segments(lower) + explicitly_allowed = bool(self.allow_patterns) and bool(segments) and all( + any(re.search(pattern, segment) for pattern in self.allow_patterns) + for segment in segments ) if not explicitly_allowed: for pattern in self.deny_patterns: @@ -822,6 +825,79 @@ class ExecTool(Tool): return None + @staticmethod + def _split_shell_segments(command: str) -> list[str]: + """Split shell commands on top-level chaining operators.""" + segments: list[str] = [] + current: list[str] = [] + quote: str | None = None + escaped = False + paren_depth = 0 + i = 0 + + while i < len(command): + ch = command[i] + + if escaped: + current.append(ch) + escaped = False + i += 1 + continue + + if ch == "\\" and quote != "'": + current.append(ch) + escaped = True + i += 1 + continue + + if quote is not None: + current.append(ch) + if ch == quote: + quote = None + i += 1 + continue + + if ch in {"'", '"', "`"}: + current.append(ch) + quote = ch + i += 1 + continue + + if ch == "(": + paren_depth += 1 + current.append(ch) + i += 1 + continue + + if ch == ")" and paren_depth > 0: + paren_depth -= 1 + current.append(ch) + i += 1 + continue + + operator_len = 0 + if paren_depth == 0: + if command.startswith(("&&", "||"), i): + operator_len = 2 + elif ch in {";", "|"}: + operator_len = 1 + + if operator_len: + segment = "".join(current).strip() + if segment: + segments.append(segment) + current = [] + i += operator_len + continue + + current.append(ch) + i += 1 + + segment = "".join(current).strip() + if segment: + segments.append(segment) + return segments + @classmethod def _is_benign_device_path(cls, path: str) -> bool: """Return True for kernel device files that should never be workspace-blocked.""" diff --git a/tests/tools/test_exec_allow_patterns.py b/tests/tools/test_exec_allow_patterns.py index cfd59f2b..9700d29c 100644 --- a/tests/tools/test_exec_allow_patterns.py +++ b/tests/tools/test_exec_allow_patterns.py @@ -58,18 +58,11 @@ def test_allow_patterns_is_whitelist_only(): assert "allowlist" in result.lower() -def test_allow_patterns_do_not_allow_chained_command_bypass(): - """A partial allowlist match must not bypass deny patterns in chained commands.""" - tool = ExecTool(allow_patterns=[r"\becho\b"]) - result = tool._guard_command("echo hello; rm -rf /", "/tmp") - assert result is not None - assert "deny pattern filter" in result.lower() +def test_guard_allow_patterns_block_non_matching_chained_segment(): + """Every top-level shell segment must match an allow pattern.""" + tool = ExecTool(allow_patterns=[r"\becho\s+allowlisted\b"]) - -def test_allow_patterns_do_not_allow_comment_tail_bypass(): - """Comment tails must not make a non-allowlisted command match.""" - tool = ExecTool(allow_patterns=[r"echo allowlisted"]) - result = tool._guard_command("touch canary # echo allowlisted", "/tmp") + result = tool._guard_command("echo allowlisted && touch /tmp/evil", "/tmp") assert result is not None assert "allowlist" in result.lower() @@ -87,3 +80,32 @@ def test_allow_patterns_fullmatch_allows_exact_command(): tool = ExecTool(allow_patterns=[r"rm\s+-rf\s+/tmp/build"]) result = tool._guard_command("rm -rf /tmp/build", "/tmp") assert result is None + + +def test_guard_allow_patterns_allow_single_matching_segment(): + tool = ExecTool(allow_patterns=[r"\becho\s+allowlisted\b"]) + + result = tool._guard_command("echo allowlisted", "/tmp") + + assert result is None + + +def test_guard_allow_patterns_allow_multiple_matching_segments(): + tool = ExecTool( + allow_patterns=[ + r"\becho\s+allowlisted\b", + r"\becho\s+also_allowed\b", + ] + ) + + result = tool._guard_command("echo allowlisted && echo also_allowed", "/tmp") + + assert result is None + + +def test_guard_allow_patterns_keep_fullmatch_style_compatibility(): + tool = ExecTool(allow_patterns=[r"^echo\s+allowlisted$"]) + + result = tool._guard_command("echo allowlisted", "/tmp") + + assert result is None