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
This commit is contained in:
@@ -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."""
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user