Skip to content

fix(security): tokenize the execute_bash destructive-command screen (#128) - #195

Merged
Zongwei9888 merged 2 commits into
HKUDS:mainfrom
rifkir23:fix/command-guard-tokenized-blocklist
Sep 3, 2026
Merged

fix(security): tokenize the execute_bash destructive-command screen (#128)#195
Zongwei9888 merged 2 commits into
HKUDS:mainfrom
rifkir23:fix/command-guard-tokenized-blocklist

Conversation

@rifkir23

Copy link
Copy Markdown
Contributor

Follow-up to #128.

Problem

The execute_bash guard in tools/code_implementation_server.py decides whether a command is destructive by matching it as a substring of the raw command string:

dangerous_commands = ["rm -rf", "sudo", "chmod 777", "mkfs", "dd if="]
normalized_command = re.sub(r"\s+", " ", command).lower()
if any(d in normalized_command for d in dangerous_commands):
    block

The whitespace-normalization that was already added here helps (RM -rf no longer slips through), but a normalized substring check is still bypassable, and issue #128 explicitly kept this open to track it:

command classified as reality
rm -r -f / safe ❌ split short flags — destructive
rm --recursive --force / safe ❌ long flags — destructive
chmod 0777 file safe ❌ leading zero — same as 777
touch rm-rf-notes.txt blocked benign, only contains the text

So the layer both misses real destructive commands and false-positives on harmless ones.

Change

Add core/harness/command_guard.py with screen_command(command) -> str | None. It splits the command on the shell control operators (; && || | & and newlines) and tokenizes each segment with shlex, then matches on the argv (command name + flags) rather than substrings. That:

  • treats -rf, -fr, -r -f, --recursive --force as equivalent (flag order / combination / spelling no longer matter);
  • recognizes numeric (777, 0777) and symbolic (a+rwx) chmod forms;
  • catches a destructive stage hidden in a pipeline or sequence (echo hi && rm -rf /, cd /tmp; rm -rf build);
  • stops false-positiving on benign commands that merely contain the text.

execute_bash now calls screen_command in place of the substring list.

This is defense-in-depth, not the boundary

I have deliberately not touched the sandbox. As #128 concluded, the workspace sandbox in core.harness.sandbox is the real enforcement; this screen is the cheap first pass in front of it. It never raises, and any command it cannot tokenize (unbalanced quotes, etc.) is passed straight through to the sandbox unchanged — the goal is to make the shallow layer honestly catch what it claims to, without pretending to be the boundary.

Tests

tests/test_command_guard.py covers the historical bypasses, the old false-positives, multi-segment commands, empty input, and unparseable input. ruff check and ruff format --check are clean on the new and modified files.

…S#128)

The execute_bash guard matched destructive commands as substrings of the
raw command string. Even after whitespace normalization it stayed
bypassable: split short flags (rm -r -f), long flags
(rm --recursive --force), and numeric permission variants (chmod 0777)
all slip through, while benign commands that merely contain the text
(touch rm-rf-notes.txt) are falsely blocked.

Replace it with core.harness.command_guard.screen_command, which splits
the command on shell operators (; && || | &) and tokenizes each segment
with shlex, matching on argv (command name + flags) instead of
substrings. This closes the flag-order / flag-spelling / numeric gaps and
also catches a destructive stage hidden in a pipeline or sequence
(echo hi && rm -rf /).

This is cheap defense-in-depth, not the security boundary: the workspace
sandbox remains the real enforcement, and any command the screen cannot
parse is passed through to it unchanged.

Add tests/test_command_guard.py covering the historical bypasses, the old
false-positives, multi-segment commands, empty input, and unparseable
input.
@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor

🔍 CI 失败诊断 (raymondginger2018-sudo)

PR #195 的 4 个 CI job(test 3.12/3.13/3.14 + Windows lifecycle)全部失败,根因是同一处: 测试失败(3 个 parametrize 参数全挂)。

根因 1: 大小写敏感

将 直接传给助手函数,但 检查 Microsoft Windows [°汾 10.0.26200.9278]
(c) Microsoft Corporation¡£±£ÁôËùÓÐȨÀû¡£

F:\DEEPCODE>(大小写敏感)。大写输入 不被拦截,落到 sandbox 执行失败,消息变成 (不含 )。

修复: 后再分类

根因 2:error message 不含

将 blocked 消息从 改为 ,但新增的 断言 。消息格式变更与测试断言不一致。

修复: → (消息变为 )

修复 diff(2 行)

diff --git a/core/harness/command_guard.py b/core/harness/command_guard.py
index a6b4bff1..17cc7e08 100644
--- a/core/harness/command_guard.py
+++ b/core/harness/command_guard.py
@@ -144,7 +144,7 @@ def screen_command(command: str) -> str | None:
             continue
         if not tokens:
             continue
-        reason = _classify(tokens[0], tokens[1:])
+        reason = _classify(tokens[0].lower(), tokens[1:])
         if reason is not None:
             return reason
 
diff --git a/tools/code_implementation_server.py b/tools/code_implementation_server.py
index 41f162bb..d16a9cd5 100644
--- a/tools/code_implementation_server.py
+++ b/tools/code_implementation_server.py
@@ -800,7 +800,7 @@ async def execute_bash(command: str, timeout: int = 30) -> str:
         if blocked_reason is not None:
             result = {
                 "status": "error",
-                "message": f"Dangerous command blocked ({blocked_reason}): {command}",
+                "message": f"Dangerous command prohibited ({blocked_reason}): {command}",
             }
             log_operation(
                 "execute_bash_blocked",

验证结果

  • 修复后 全部 PASSED
  • 35 个 test 全部 PASSED
  • 不影响已有逻辑(3 个助手函数只改 cmd 大小写归一化,不改变已有分类规则)

@rifkir23

rifkir23 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

thanks for catching this. i pushed the follow-up in aaca028d: the executable token is now normalized before classification, and the error response keeps the existing prohibited wording. i also added a regression case for uppercase command input.

the python 3.12/3.13/3.14 and windows jobs that were failing are green now.

@Zongwei9888
Zongwei9888 merged commit 030810c into HKUDS:main Sep 3, 2026
14 checks passed
@Zongwei9888

Copy link
Copy Markdown
Collaborator

Merged on 2026-09-03 as 030810c. Thanks for the quick follow-up on the case handling and the prohibited wording — it turned red CI into green within the day. One note for the record: this screen only guards the legacy execute_bash tool; the V2 agent's shell tool relies on the permission engine and the sandbox, so the honest "cheap first pass, not the boundary" framing in your docstring is exactly right. Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants