diff --git a/crates/skilllite-sandbox/src/bash_validator.rs b/crates/skilllite-sandbox/src/bash_validator.rs index 2a4dca01..a9e44603 100644 --- a/crates/skilllite-sandbox/src/bash_validator.rs +++ b/crates/skilllite-sandbox/src/bash_validator.rs @@ -6,8 +6,9 @@ //! //! ## Security Layers //! -//! 1. **Chain operator detection** — blocks `;`, `&&`, `||`, `|`, backticks, -//! `$(...)`, `${...}`, newlines, and other injection vectors. +//! 1. **Chain operator detection** — blocks `;`, `&&`, `||`, `|`, `&`, +//! redirects (`>`, `<`, `>(`), backticks, `$(...)`, `${...}`, newlines, +//! and other injection vectors. //! 2. **Allowed prefix matching** — command must start with one of the //! `allowed-tools: Bash(prefix:*)` patterns declared in SKILL.md. //! 3. **Blocked prefix check** — dangerous commands (rm, sudo, sh, curl, etc.) @@ -46,11 +47,15 @@ pub enum BashValidationError { EmptyCommand, } -/// Operators that could chain multiple commands together. +/// Operators that could chain multiple commands together or redirect I/O. /// We treat their presence anywhere in the command string as an injection attempt. +/// +/// Note: bare `&` / `>` / `<` must be blocked even though `&&` / `||` / `>(` +/// are listed separately — an unsandboxed `sh -c` will otherwise run a +/// backgrounded second command or write arbitrary host files. const CHAIN_OPERATORS: &[&str] = &[ - ";", "&&", "||", "|", "`", "$(", "${", "\n", "\r", // Redirect-based attacks - ">(", + ";", "&&", "||", "|", "&", "`", "$(", "${", "\n", "\r", // Redirect-based attacks + ">(", ">", "<", ]; /// Command prefixes that are always blocked, regardless of `allowed-tools`. @@ -248,6 +253,47 @@ mod tests { assert!(matches!(result, Err(BashValidationError::ChainOperator(_)))); } + #[test] + fn test_reject_background_ampersand() { + let patterns = agent_browser_patterns(); + let result = + validate_bash_command("agent-browser open x.com & touch /tmp/pwned", &patterns); + assert!(matches!(result, Err(BashValidationError::ChainOperator(_)))); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("chain operator"), + "error should mention chain operator: {err}" + ); + } + + #[test] + fn test_reject_tight_background_ampersand() { + let patterns = agent_browser_patterns(); + let result = validate_bash_command("agent-browser open x&touch /tmp/pwned", &patterns); + assert!(matches!(result, Err(BashValidationError::ChainOperator(_)))); + } + + #[test] + fn test_reject_stdout_redirect() { + let patterns = agent_browser_patterns(); + let result = validate_bash_command("agent-browser open x.com > /tmp/pwned", &patterns); + assert!(matches!(result, Err(BashValidationError::ChainOperator(_)))); + } + + #[test] + fn test_reject_stdin_redirect() { + let patterns = agent_browser_patterns(); + let result = validate_bash_command("agent-browser open x.com < /etc/passwd", &patterns); + assert!(matches!(result, Err(BashValidationError::ChainOperator(_)))); + } + + #[test] + fn test_reject_append_redirect() { + let patterns = agent_browser_patterns(); + let result = validate_bash_command("agent-browser open x.com >> /tmp/pwned", &patterns); + assert!(matches!(result, Err(BashValidationError::ChainOperator(_)))); + } + // ---- Blocked prefixes ---- #[test] diff --git a/docs/en/ARCHITECTURE.md b/docs/en/ARCHITECTURE.md index 9e8aac60..562244c6 100644 --- a/docs/en/ARCHITECTURE.md +++ b/docs/en/ARCHITECTURE.md @@ -852,7 +852,7 @@ Scans Skill dependencies for known vulnerabilities using OSV (Open Source Vulner ### 6. Additional Protections -- **Bash Validator** (`bash_validator.rs`): Detects dangerous bash commands +- **Bash Validator** (`bash_validator.rs`): Detects dangerous bash commands, including chain operators (`;`, `&&`, `||`, `|`, `&`), redirects (`>`, `<`, `>(`), substitutions, and blocked prefixes before unsandboxed `sh -c` execution - **File Move Protection** (`move_protection.rs`): Prevents malicious file overwrites of critical paths - **User Authorization**: Level 3 runs a unified precheck (`SKILL.md` + entry script); if the precheck produces a review report (including medium script findings, SKILL.md alerts, or scan errors), the runner (CLI) or host (agent/MCP) requires explicit consent before execution. `SKILLLITE_AUTO_APPROVE` applies to the same gate for TTY/CLI runs. diff --git a/docs/zh/ARCHITECTURE.md b/docs/zh/ARCHITECTURE.md index e6750680..175e4e06 100644 --- a/docs/zh/ARCHITECTURE.md +++ b/docs/zh/ARCHITECTURE.md @@ -847,7 +847,7 @@ SKILLLITE_NO_SANDBOX=false # 禁用沙箱 ### 6. 其他保护 -- **Bash 验证器** (`bash_validator.rs`):检测危险 bash 命令 +- **Bash 验证器** (`bash_validator.rs`):检测危险 bash 命令,包括链式运算符(`;`、`&&`、`||`、`|`、`&`)、重定向(`>`、`<`、`>(`)、替换写法以及被禁止的命令前缀;在无沙箱的 `sh -c` 执行前拦截 - **文件移动保护** (`move_protection.rs`):防止恶意文件覆盖关键路径 - **用户授权**:Level 3 先做统一预检(`SKILL.md` + 入口脚本);若预检生成需审阅的报告(含中等脚本告警、SKILL.md 告警或扫描失败提示),CLI 由 runner 在 TTY 上征求同意,Agent/MCP 由宿主侧确认;`SKILLLITE_AUTO_APPROVE` 作用于 CLI 侧同一门控。 diff --git a/tasks/TASK-2026-085-bash-validator-background-redirect/CONTEXT.md b/tasks/TASK-2026-085-bash-validator-background-redirect/CONTEXT.md new file mode 100644 index 00000000..070560d9 --- /dev/null +++ b/tasks/TASK-2026-085-bash-validator-background-redirect/CONTEXT.md @@ -0,0 +1,23 @@ +# CONTEXT + +## Technical boundaries + +- Change is confined to `crates/skilllite-sandbox/src/bash_validator.rs` plus EN/ZH architecture notes. +- Call sites in `skilllite-agent` / `skilllite-commands` already call `validate_bash_command` before `sh -c`; no call-site API change. + +## Constraints + +- Keep fail-closed substring policy consistent with existing operators. +- Do not relax blocked-prefix or allowed-pattern checks. +- Avoid broad refactors in the same PR. + +## Compatibility notes + +- Stricter than `main@12010e8`: previously accepted commands with `&` / `>` / `<` (including URL query strings containing `&`) will now fail validation. +- This is intentional security hardening for an unsandboxed execution path. + +## Near-misses deferred + +- `SilentEventSink` auto-approves `ConfirmRequired` during memory flush / swarm single-task. +- `ChatSession` uses `chat_root()` and ignores `config.workspace` when `SKILLLITE_WORKSPACE` is unset. +- Concurrent `sessions.json` RMW last-writer-wins (medium metadata loss). diff --git a/tasks/TASK-2026-085-bash-validator-background-redirect/PRD.md b/tasks/TASK-2026-085-bash-validator-background-redirect/PRD.md new file mode 100644 index 00000000..736f42b5 --- /dev/null +++ b/tasks/TASK-2026-085-bash-validator-background-redirect/PRD.md @@ -0,0 +1,21 @@ +# PRD + +## Summary + +Harden bash-tool command validation so backgrounding and I/O redirection cannot bypass the existing chain-operator gate before unsandboxed `sh -c` execution. + +## Why + +Bash-tool skills are an intentional exception path: after Rust-side validation they run on the host shell without bubblewrap. The validator is therefore the primary injection boundary. Omitting bare `&` / `>` / `<` leaves a concrete host command-execution and arbitrary-file-write hole. + +## Requirements + +1. Reject commands containing bare `&` (background / and-redirect forms). +2. Reject commands containing `>` or `<` (stdout/stdin/append redirects). +3. Keep existing protections for `;`, `&&`, `||`, `|`, backticks, `$()`, `${}`, newlines, and `>(`. +4. Document the operator set in EN/ZH architecture security notes. + +## Non-goals + +- Quote-aware or AST-based shell parsing in this change. +- Moving bash-tool execution into the sandbox. diff --git a/tasks/TASK-2026-085-bash-validator-background-redirect/REVIEW.md b/tasks/TASK-2026-085-bash-validator-background-redirect/REVIEW.md new file mode 100644 index 00000000..85dcea79 --- /dev/null +++ b/tasks/TASK-2026-085-bash-validator-background-redirect/REVIEW.md @@ -0,0 +1,17 @@ +# REVIEW + +## Findings + +- Root cause confirmed: `CHAIN_OPERATORS` omitted bare `&`, `>`, `<` while execution uses unsandboxed `sh -c`. +- Fix is minimal and fail-closed; no API surface change. +- False-positive risk for URL query `&` is accepted and documented (same substring policy as `;` / `|`). +- Falsifiability: without the new operators, the five injection payloads remain ACCEPT; with them they BLOCK; valid `agent-browser open https://example.com` stays ACCEPT. + +## Security review notes + +- [x] What security policy changed, and why is it needed? — Expanded chain/redirect operator deny list to close host RCE/file-write via bash-tool skills. +- [x] Is default behavior more permissive? — No; stricter only. +- [x] Does this affect `SKILLLITE_*` config semantics or backward compatibility? — No env semantics; previously-accepted unsafe command strings are now rejected. +- [x] Were tests and EN/ZH docs updated? — Yes. + +## Merge readiness: ready to merge after PR CI diff --git a/tasks/TASK-2026-085-bash-validator-background-redirect/STATUS.md b/tasks/TASK-2026-085-bash-validator-background-redirect/STATUS.md new file mode 100644 index 00000000..61cf3cc8 --- /dev/null +++ b/tasks/TASK-2026-085-bash-validator-background-redirect/STATUS.md @@ -0,0 +1,50 @@ +# STATUS + +## Current status + +`done` — fix validated; preparing PR. + +## Timeline + +- 2026-08-09: Critical bug sweep on `main@12010e8` reproduced bash-tool injection via bare `&` / `>` / `<`. +- 2026-08-09: Extended `CHAIN_OPERATORS`, added regression tests, updated EN/ZH architecture notes. +- 2026-08-09: Validation passed (`cargo test -p skilllite-sandbox bash_validator` 25 ok; clippy sandbox clean; `validate_tasks.py` 71 folders). + +## Checkpoints + +- [x] Concrete PoC: validator accepted injection forms; `sh -c` created `/tmp/pwned_*` +- [x] Code fix in `bash_validator.rs` +- [x] Unit tests added +- [x] Docs EN/ZH updated +- [x] `cargo test -p skilllite-sandbox bash_validator` — 25 passed +- [x] `cargo clippy -p skilllite-sandbox --all-targets -- -D warnings` — clean +- [x] `cargo fmt --check` — clean for changed files / workspace +- [x] `python3 scripts/validate_tasks.py` — 71 task folders passed +- [x] PR opened: https://github.com/EXboys/skilllite/pull/137 + +## Blockers + +- None. + +## Validation evidence + +```text +$ cargo test -p skilllite-sandbox bash_validator +running 25 tests +... +test result: ok. 25 passed; 0 failed; 0 ignored; 0 measured; 61 filtered out + +$ cargo clippy -p skilllite-sandbox --all-targets -- -D warnings +Finished `dev` profile ... + +$ python3 scripts/validate_tasks.py +Task validation passed (71 task directories checked). +``` + +Pre-fix PoC (host): + +```text +ACCEPT agent-browser open ... & touch /tmp/pwned_bg -> file created +ACCEPT agent-browser open x&touch /tmp/pwned_tight -> file created +ACCEPT agent-browser open x > /tmp/pwned_redir -> file created +``` diff --git a/tasks/TASK-2026-085-bash-validator-background-redirect/TASK.md b/tasks/TASK-2026-085-bash-validator-background-redirect/TASK.md new file mode 100644 index 00000000..1cc02d01 --- /dev/null +++ b/tasks/TASK-2026-085-bash-validator-background-redirect/TASK.md @@ -0,0 +1,72 @@ +# TASK Card + +## Metadata + +- Task ID: TASK-2026-085-bash-validator-background-redirect +- Title: Block bash background and redirect injection +- Status: `done` +- Priority: `P0` +- Owner: automation +- Contributors: automation +- Created: 2026-08-09 +- Target milestone: security hotfix + +## Problem + +`validate_bash_command` blocks `;`, `&&`, `||`, `|`, and process substitution `>(`, but accepts bare `&`, `>`, and `<`. Bash-tool skills then run the accepted string via unsandboxed `sh -c`, so an LLM (or compromised skill args) can background a second host command or redirect I/O to arbitrary paths. + +Concrete trigger: + +```text +agent-browser open https://example.com & touch /tmp/pwned +agent-browser open x > /tmp/pwned +``` + +Both are accepted by the validator on `main@12010e8` and execute the side effect even when `agent-browser` is missing. + +## Scope + +- In scope: + - Extend `CHAIN_OPERATORS` in `skilllite-sandbox` bash validator + - Regression tests for `&`, tight `&`, `>`, `<`, `>>` + - EN/ZH architecture note for the hardened operator set +- Out of scope: + - Full shell AST parsing / quote-aware validation + - Sandboxing bash-tool execution itself + - SilentEventSink auto-approve and chat-root workspace split (tracked separately) + +## Acceptance Criteria + +- [x] Bare `&`, `>`, and `<` are rejected by `validate_bash_command` +- [x] Existing chain-operator / blocked-prefix tests still pass +- [x] New regression tests cover background and redirect forms +- [x] EN/ZH architecture docs mention the operator set +- [x] `cargo test -p skilllite-sandbox` passes for validator coverage + +## Risks + +- Risk: Strict substring blocking of `&` / `>` / `<` rejects URLs or args that contain those characters (including quoted query strings). + - Impact: Some previously-accepted bash-tool commands fail validation. + - Mitigation: Matches existing substring policy for `;` / `|`; safer fail-closed default for unsandboxed `sh -c`. + +## Validation Plan + +- Required tests: sandbox bash_validator unit tests (existing + new) +- Commands to run: + - `cargo test -p skilllite-sandbox bash_validator` + - `cargo clippy -p skilllite-sandbox --all-targets -- -D warnings` + - `cargo fmt --check` + - `python3 scripts/validate_tasks.py` +- Manual checks: + - Confirm pre-fix acceptance / post-fix rejection matrix for `&` / `>` / `<` + +## Regression Scope + +- Areas likely affected: bash-tool skill execution (`skilllite-agent` / `skilllite-commands` execute paths) +- Explicit non-goals: changing sandbox level defaults; rewriting bash execution to use bwrap + +## Links + +- Source TODO section: N/A (critical bug automation sweep 2026-08-09) +- Related PRs/issues: open security backlog #89 / #112 / #123–#136 (different themes) +- Related docs: `docs/en/ARCHITECTURE.md`, `docs/zh/ARCHITECTURE.md` diff --git a/tasks/board.md b/tasks/board.md index 47f785f3..42f2ff06 100644 --- a/tasks/board.md +++ b/tasks/board.md @@ -1,6 +1,6 @@ # Task Board -Last updated: 2026-07-06 (TASK-2026-070 critical bug sweep done) +Last updated: 2026-08-09 (TASK-2026-085 bash validator background/redirect injection done) ## In Progress @@ -17,6 +17,7 @@ Last updated: 2026-07-06 (TASK-2026-070 critical bug sweep done) ## Done +- `TASK-2026-085-bash-validator-background-redirect` - Status: `done` - Owner: `automation` - `TASK-2026-070-critical-bug-sweep-2026-07-06` - Status: `done` - Owner: `agent` - `TASK-2026-069-evolution-workspace-run-scope` - Status: `done` - Owner: `agent` - `TASK-2026-068-evolution-workspace-db-scope` - Status: `done` - Owner: `agent`