Skip to content

fix(sandbox): classify destructive commands from bash AST - #697

Open
gouhongshen wants to merge 3 commits into
matrixorigin:mainfrom
gouhongshen:codex/fix-bash-risk-ast-main
Open

fix(sandbox): classify destructive commands from bash AST#697
gouhongshen wants to merge 3 commits into
matrixorigin:mainfrom
gouhongshen:codex/fix-bash-risk-ast-main

Conversation

@gouhongshen

@gouhongshen gouhongshen commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • feat (new feature)
  • fix (bug fix)
  • docs (documentation)
  • style (formatting, no code change)
  • refactor (code change that neither fixes a bug nor adds a feature)
  • perf (performance improvement)
  • test (adding or updating tests)
  • chore (maintenance, tooling)
  • build / ci (build or CI changes)

Which issue(s) this PR fixes

N/A — main-branch port of the production MOI false-positive fix in #696.

What this PR does / why we need it

The Bash validator previously scanned the complete raw command text for destructive command names. That treated heredoc bodies and inline interpreter programs as shell commands, so an ordinary Python identifier such as dd could block the whole tool call and force the agent into repeated rewrites.

This port integrates the fix with main's stricter literal-argv parser and makes the Bash AST the canonical owner of destructive executable classification:

  • classify only executable positions, including path-qualified commands and transparent launchers such as sudo, env, and exec;
  • recursively analyze literal bash/sh/zsh -c programs so real nested destructive commands remain blocked;
  • parse shell long options, short-option clusters, and option values explicitly, and fail closed when the nested command boundary is ambiguous;
  • resolve commands dispatched through busybox/toybox, xargs, and find -exec/-execdir instead of treating only the outer executable as authoritative;
  • fail closed when an actual executable position is runtime-dependent, while keeping heredoc bodies and ordinary dynamic arguments as data;
  • preserve every previously configured destructive executable, process-control rule, fork-bomb syntax rule, and independent rm -rf guard;
  • remove duplicate raw-text command patterns from astra-tools, so heredoc and inline Python/Node data no longer trigger destructive-command false positives.

Architecture and complexity delta

  • Canonical owner changed or extended: astra-sandbox Bash AST risk analysis now owns destructive executable classification and reuses main's existing literal command parser.
  • Existing implementations/callers searched: parse_plain_bash_commands, analyze_command_risks, validate_execute_bash_command_in_workspace, wrapper handling, nested shell launchers, and existing command-risk tests.
  • Superseded code, states, tables, shims, and self-only tests removed: removed the legacy raw-text destructive-command tokenizer and duplicate executable substring patterns from astra-tools.
  • Net code/state/table delta: three existing Rust files; no protocol, persistent state, database schema, configuration, fallback, or compatibility layer.
  • If parallel implementations remain, the external boundary and retirement condition: shell-only syntax checks such as fork bombs and the dedicated catastrophic rm -rf guard remain in astra-tools; executable classification has one owner in astra-sandbox.

Production wiring and verification

  • Public product entrypoint exercised: the execute_bash validator used by Astra tool execution.
  • Unhappy paths exercised: every configured destructive executable, path-qualified invocations, transparent wrappers, nested shell programs, --norc, value-consuming --rcfile, short-option clusters, multi-call applets, xargs, find -exec/-execdir, runtime-dependent executable positions, ambiguous shell options, malformed input, heredoc Python data, inline interpreter source, and benign command arguments.
  • Database schema/query/transaction/migration verified against a real database, or N/A with reason: N/A; command validation is in-memory and this PR does not touch persistence.

Verification:

  • cargo fmt --all -- --check
  • cargo test -p astra-sandbox --lib — 160 passed
  • cargo test -p astra-tools --lib validate_execute_bash — 10 passed
  • cargo clippy -p astra-sandbox -p astra-tools --all-targets -- -D warnings
  • Full astra-tools lib run was attempted; unrelated process-detach and workspace-lease concurrency tests interfered and timed out under the parallel local run. The affected Bash validation suite above passes independently.

@gouhongshen gouhongshen self-assigned this Sep 4, 2026
@gouhongshen
gouhongshen marked this pull request as ready for review September 4, 2026 04:01

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

方向正确:用 Bash AST 区分命令与 heredoc/解释器数据,解决 dd 文本误报是合理的。但当前 head 引入了一个安全绕过,需修复后再合并。

[P1] 长选项中任意字符 c 被误认为 shell 的 -c

nested_shell_script 使用 argument[1..].chars().any(|flag| flag == 'c') 判断 command-string 选项:

fn nested_shell_script(words: &[String]) -> Option<&str> {
let index = effective_command_index(words)?;
let executable = command_basename(words.get(index)?);
if !matches!(executable.as_str(), "bash" | "sh" | "dash" | "zsh" | "ksh") {
return None;
}
let mut argument_index = index + 1;
while let Some(raw) = words.get(argument_index) {
let argument = unquote_shell_word(raw);
if argument == "--" || !argument.starts_with('-') || argument == "-" {
return None;
}
if argument[1..].chars().any(|flag| flag == 'c') {
return words
.get(argument_index + 1)
.map(|script| unquote_shell_word(script));
}
argument_index += 1;
}
None

因此常见命令 bash --norc -c 'dd if=/dev/zero of=/dev/sda' 会把 --norc 当作 -c,把下一个参数字面量 -c 当作脚本递归解析,真正的 destructive script 则完全跳过。bash --rcfile file -c ... 也有同类问题。由于本 PR 同时移除了 destructive substring fallback,这些命令会从 DestructiveCommand 降为未识别,形成策略绕过。

请区分精确长选项与合法的短选项簇,并正确跳过会消费参数的 shell 选项;至少补 bash --norc -c ...bash --rcfile file -c ...(以及等价 wrapper)回归测试。解析无法可靠解析 executable/-c 边界时,安全策略应 fail closed,同时继续保证 Python heredoc 中普通 dd 数据不误报。

@gouhongshen

Copy link
Copy Markdown
Collaborator Author

已在 bdb5681b 修复,这条 P1 成立。

  • 长选项改为精确匹配,不再把 --norc 中的 c 当成 -c
  • --rcfile / --init-file 会正确消费参数;
  • 短选项簇按真实参数消费顺序处理,包括 -lc-oc pipefail-oO pipefail extglob -c
  • 无法可靠确定 shell command-string 边界时标记为 RemoteCodeExecution 并 fail closed;没有恢复原始命令全文扫描,因此 Python heredoc 中的 dd 仍不会误报。

新增了 bash --norc -cbash --rcfile ... -csudo / env wrapper、短选项簇和歧义参数的回归覆盖。astra-sandbox 159 个单测、execute_bash 相关 10 个单测及两个 crate 的 all-target clippy 均通过。等价修复也已同步到 moi-dev 的 #6967db624c4)。请重新 review。

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

重新审查了最新 head bdb5681b--norc/--rcfile 被误判为 -c 的问题已经修复,短选项簇、消费参数的选项、未知选项 fail-closed 及对应 validator 测试都合理。

仍有一个安全阻断项:

[P1] executable dispatcher 和动态 executable 仍可绕过 destructive 检测

当前 destructive_command_name 只检查 effective_command_index 指向的 executable:

const DESTRUCTIVE_COMMANDS: &[&str] = &[
"dd",
"mkswap",
"truncate",
"shred",
"wipefs",
"blkdiscard",
"fdisk",
"sfdisk",
"parted",
"cryptsetup",
"pvremove",
"vgremove",
"lvremove",
"zpool",
"zfs",
"shutdown",
"reboot",
"poweroff",
"halt",
"telinit",
];
fn destructive_command_name(words: &[String]) -> Option<&'static str> {
let index = effective_command_index(words)?;
let executable = command_basename(words.get(index)?);
if executable == "mkfs" || executable.starts_with("mkfs.") {
return Some("mkfs");
}
DESTRUCTIVE_COMMANDS
.iter()
.copied()
.find(|candidate| executable.eq_ignore_ascii_case(candidate))
}

因此 busybox dd if=/dev/zero of=/dev/sda 的 executable 是 busybox,真实执行的 applet dd 完全不会被识别;tool=dd; "$tool" if=/dev/zero of=/dev/sda 也因 command name 无法静态还原而直接漏过。两者在本 PR 移除 token fallback 前都会被识别,而现在不会产生 DestructiveCommand/fail-closed risk。仓库现有 rm validator 已经显式承认 busybox/toybox 这类 multi-call binary,因此该执行形态不是假设场景。

请把 command resolution 建模为“直接 executable / transparent launcher / multi-call dispatcher / unresolved dynamic”而不是继续堆字符串特例:至少覆盖 busybox/toybox applet;对位于 command position 且无法证明安全的动态 executable fail closed。也请评估并测试 xargsfind -exec/-execdir 这类从 argv 调度命令的边界,同时保留 Python/Node/heredoc 数据不被当成 shell command 的目标。

@gouhongshen

Copy link
Copy Markdown
Collaborator Author

已按该 P1 的执行语义处理,修复在 7e730079

  • 用统一的 command resolver 区分直接 executable、transparent launcher、multi-call dispatcher 和无法静态确定的 executable;没有恢复全文/token 子串扫描。
  • busybox/toybox 会继续解析真实 applet;xargsfind -exec/-execdir 会继续解析被调度命令,嵌套 sh -c 也递归检查。
  • 仅真实 executable 位置无法静态确定时 fail closed;Python/Node/heredoc 内容和普通参数仍作为数据,不会因出现 dd 被误拦。
  • 增加了上述危险、动态和 benign 对照用例,并保留 malformed input 的原有行为。

验证通过:astra-sandbox 全部 160 个 lib tests、execute_bash 目标测试 10 个、受影响 crates 的 clippy -D warnings。同一修复已同步到 #696df3bd3b1)。已更新 PR 描述。

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.

2 participants