diff --git a/_primitives/_rust/Cargo.lock b/_primitives/_rust/Cargo.lock index e7638b4..b4a7e3a 100644 --- a/_primitives/_rust/Cargo.lock +++ b/_primitives/_rust/Cargo.lock @@ -3969,6 +3969,7 @@ dependencies = [ "anyhow", "kei-atom-discovery", "kei-skills", + "libc", "serde", "serde_json", "tempfile", diff --git a/_primitives/_rust/kei-mcp/Cargo.toml b/_primitives/_rust/kei-mcp/Cargo.toml index b6f0768..cc6429f 100644 --- a/_primitives/_rust/kei-mcp/Cargo.toml +++ b/_primitives/_rust/kei-mcp/Cargo.toml @@ -24,6 +24,11 @@ tokio = { workspace = true, features = ["io-std"] } # v0.40 (Phase C): toml needed for safe_tools::policy_chain — reads # ~/.claude/hooks/_lib/policy-chain.toml to know which hooks to chain. toml = "0.8" +# v0.41 (audit fix #5): killpg via libc on Unix — kill_on_drop only SIGKILLs +# the immediate child shell, leaving grandchildren orphaned. We set the child +# in its own process group and killpg() the group on timeout. +[target.'cfg(unix)'.dependencies] +libc = "0.2" anyhow = { workspace = true } kei-atom-discovery = { path = "../kei-atom-discovery" } kei-skills = { path = "../kei-skills" } diff --git a/_primitives/_rust/kei-mcp/src/handlers/safe_tools.rs b/_primitives/_rust/kei-mcp/src/handlers/safe_tools.rs index 68f70b0..a96fbb7 100644 --- a/_primitives/_rust/kei-mcp/src/handlers/safe_tools.rs +++ b/_primitives/_rust/kei-mcp/src/handlers/safe_tools.rs @@ -15,17 +15,29 @@ //! abstraction layer. Shell-out per hook keeps the contract identical to //! Claude's native PreToolUse pipeline. //! -//! Guard against double-enforcement: if the parent process is already inside -//! Claude Code (`$CLAUDECODE=1`) or Grok (`$GROKCODE=1`), the chain is -//! skipped — the CLI's native hooks already fired on its own PreToolUse. +//! CLAUDECODE / GROKCODE guard — DESIGN NOTE (NOT a security boundary): +//! When invoked from inside Claude Code (`$CLAUDECODE=1`) or Grok the chain +//! is SKIPPED to avoid double-firing the same hooks (they already ran on the +//! CLI's own PreToolUse). This is a perf / UX optimization for the inside-CLI +//! call path — NOT an authorization check. An attacker who can set the +//! parent process's environment already controls the CLI invocation anyway; +//! re-running hooks would not stop them. To raise the bar for confused-deputy +//! scenarios use full sandboxing (Phase D) or run kei-mcp as a separate UID. +//! +//! v0.41 audit fixes (2026-05-26, Gemini security review): +//! #1 fail-CLOSED on missing hooks (was: silently skip) +//! #2 path-traversal guard on kei_edit/kei_write (canonicalize + root check) +//! #3 CLAUDECODE bypass — documented as design (see above), no behavior change +//! #4 tokio::fs for async file I/O (was: blocking std::fs on tokio thread) +//! #5 process-group kill on Unix (was: kill_on_drop SIGKILLs only direct child) use crate::protocol::{err, ok, JsonRpcRequest, JsonRpcResponse, INTERNAL_ERROR, INVALID_PARAMS}; use serde::Deserialize; use serde_json::{json, Value}; -use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Stdio; use std::time::Duration; +use tokio::fs; use tokio::io::AsyncWriteExt; use tokio::process::Command; @@ -132,12 +144,25 @@ async fn handle_bash(args: &Value) -> Result { .stdout(Stdio::piped()) .stderr(Stdio::piped()) .kill_on_drop(true); + // v0.41 fix #5 (Gemini MED): put child in its own process group so timeout + // kills it and ALL grandchildren together (not just the immediate shell). + set_process_group(&mut cmd); - let fut = cmd.spawn().map_err(|e| format!("spawn bash: {e}"))?.wait_with_output(); - let out = tokio::time::timeout(Duration::from_secs(SAFE_TOOL_TIMEOUT_SECS), fut) - .await - .map_err(|_| "kei_bash timeout".to_string())? - .map_err(|e| format!("wait bash: {e}"))?; + let child = cmd.spawn().map_err(|e| format!("spawn bash: {e}"))?; + let pid_opt = child.id(); + let fut = child.wait_with_output(); + + let out = match tokio::time::timeout(Duration::from_secs(SAFE_TOOL_TIMEOUT_SECS), fut).await { + Ok(Ok(o)) => o, + Ok(Err(e)) => return Err(format!("wait bash: {e}")), + Err(_) => { + // Timeout — kill the entire process group, not just the child. + if let Some(pid) = pid_opt { + killpg_best_effort(pid); + } + return Err("kei_bash timeout".to_string()); + } + }; let stdout = String::from_utf8_lossy(&out.stdout).to_string(); let stderr = String::from_utf8_lossy(&out.stderr).to_string(); @@ -151,6 +176,27 @@ async fn handle_bash(args: &Value) -> Result { Ok(if stderr.is_empty() { stdout } else { format!("{stdout}\n[stderr]\n{stderr}") }) } +// v0.41 fix #5: process-group helpers (Unix-only; no-op on other platforms). +// tokio::process::Command::process_group is available on Unix without +// requiring the std::os::unix::process::CommandExt trait import. +#[cfg(unix)] +fn set_process_group(cmd: &mut Command) { + cmd.process_group(0); // 0 = new session leader for this child +} +#[cfg(not(unix))] +fn set_process_group(_cmd: &mut Command) {} + +#[cfg(unix)] +fn killpg_best_effort(pid: u32) { + // SAFETY: libc::kill on a negative PID targets the process group. + // SIGKILL = 9. Best-effort — ignore errors (process may have exited). + unsafe { + let _ = libc::kill(-(pid as i32), libc::SIGKILL); + } +} +#[cfg(not(unix))] +fn killpg_best_effort(_pid: u32) {} + async fn handle_edit(args: &Value) -> Result { let file_path = args.get("file_path").and_then(Value::as_str) .ok_or_else(|| missing_arg("kei_edit", "file_path"))?; @@ -159,25 +205,29 @@ async fn handle_edit(args: &Value) -> Result { let new_string = args.get("new_string").and_then(Value::as_str) .ok_or_else(|| missing_arg("kei_edit", "new_string"))?; + // v0.41 fix #2: path-traversal guard + let safe_path = validate_path(file_path)?; + let hook_input = json!({ "tool_name": "Edit", "tool_input": { - "file_path": file_path, + "file_path": safe_path.display().to_string(), "old_string": old_string, "new_string": new_string } }); run_chain("edit", &hook_input).await?; - let contents = fs::read_to_string(file_path) - .map_err(|e| format!("read {file_path}: {e}"))?; + // v0.41 fix #4: tokio::fs (async) + let contents = fs::read_to_string(&safe_path).await + .map_err(|e| format!("read {}: {e}", safe_path.display()))?; if !contents.contains(old_string) { - return Err(format!("kei_edit: old_string not found in {file_path}")); + return Err(format!("kei_edit: old_string not found in {}", safe_path.display())); } let updated = contents.replacen(old_string, new_string, 1); - fs::write(file_path, &updated) - .map_err(|e| format!("write {file_path}: {e}"))?; - Ok(format!("edited {file_path} ({} bytes)", updated.len())) + fs::write(&safe_path, &updated).await + .map_err(|e| format!("write {}: {e}", safe_path.display()))?; + Ok(format!("edited {} ({} bytes)", safe_path.display(), updated.len())) } async fn handle_write(args: &Value) -> Result { @@ -186,21 +236,113 @@ async fn handle_write(args: &Value) -> Result { let content = args.get("content").and_then(Value::as_str) .ok_or_else(|| missing_arg("kei_write", "content"))?; + // v0.41 fix #2: path-traversal guard + let safe_path = validate_path(file_path)?; + let hook_input = json!({ "tool_name": "Write", - "tool_input": { "file_path": file_path, "content": content } + "tool_input": { "file_path": safe_path.display().to_string(), "content": content } }); run_chain("write", &hook_input).await?; - if let Some(parent) = std::path::Path::new(file_path).parent() { + if let Some(parent) = safe_path.parent() { if !parent.as_os_str().is_empty() { - fs::create_dir_all(parent) + fs::create_dir_all(parent).await .map_err(|e| format!("mkdir {}: {e}", parent.display()))?; } } - fs::write(file_path, content) - .map_err(|e| format!("write {file_path}: {e}"))?; - Ok(format!("wrote {file_path} ({} bytes)", content.len())) + fs::write(&safe_path, content).await + .map_err(|e| format!("write {}: {e}", safe_path.display()))?; + Ok(format!("wrote {} ({} bytes)", safe_path.display(), content.len())) +} + +/// v0.41 fix #2 (Gemini HIGH): reject obvious path-traversal / sensitive-path +/// targets BEFORE running hooks. Defense-in-depth: hooks may also flag this, +/// but having the Rust layer reject obvious attacks gives a fast-fail +/// independent of hook configuration. +/// +/// Allowed roots: $PWD (recursively), $HOME (excluding dotfile-secret dirs). +/// Override: set KEI_ALLOWED_ROOTS=":" -separated absolute paths. +/// Always rejected: /etc/, /usr/, /System/, /var/, /private/etc/, $HOME/.ssh/, +/// $HOME/.aws/, $HOME/.config/gcloud/, $HOME/.gnupg/, any path containing "..". +fn validate_path(p: &str) -> Result { + if p.is_empty() { + return Err("file_path: empty".into()); + } + // 1. Reject literal `..` segments — covers most traversal attempts. + if p.split('/').any(|seg| seg == "..") { + return Err(format!("file_path: '..' segment not allowed in {p}")); + } + let path = Path::new(p); + // 2. Canonicalize the parent (file may not exist yet for kei_write); + // if even the parent doesn't exist, use the absolute form. + let canonical = if let Some(parent) = path.parent() { + if parent.as_os_str().is_empty() || parent == Path::new("") { + std::env::current_dir() + .map_err(|e| format!("file_path: cwd unavailable: {e}"))? + .join(path) + } else if parent.exists() { + parent.canonicalize() + .map_err(|e| format!("file_path: canonicalize {}: {e}", parent.display()))? + .join(path.file_name().unwrap_or_default()) + } else if path.is_absolute() { + path.to_path_buf() + } else { + std::env::current_dir() + .map_err(|e| format!("file_path: cwd unavailable: {e}"))? + .join(path) + } + } else { + return Err(format!("file_path: invalid {p}")); + }; + let canon_str = canonical.display().to_string(); + + // 3. Reject obvious sensitive directories. + let denylist = [ + "/etc/", "/usr/", "/System/", "/var/", "/private/etc/", "/private/var/", + "/root/", + ]; + for d in denylist { + if canon_str.starts_with(d) { + return Err(format!("file_path: denied (system dir): {canon_str}")); + } + } + if let Ok(home) = std::env::var("HOME") { + let secret_dirs = [".ssh/", ".aws/", ".gnupg/", ".config/gcloud/"]; + for sd in secret_dirs { + let full = format!("{home}/{sd}"); + if canon_str.starts_with(&full) { + return Err(format!("file_path: denied (secret dir): {canon_str}")); + } + } + } + + // 4. Enforce allowed-root containment. + let roots = allowed_roots(); + if !roots.is_empty() { + let ok = roots.iter().any(|r| canon_str.starts_with(r)); + if !ok { + return Err(format!( + "file_path: outside allowed roots {roots:?}: {canon_str}" + )); + } + } + + Ok(canonical) +} + +fn allowed_roots() -> Vec { + if let Ok(v) = std::env::var("KEI_ALLOWED_ROOTS") { + return v.split(':').filter(|s| !s.is_empty()).map(String::from).collect(); + } + let mut roots = Vec::new(); + if let Ok(cwd) = std::env::current_dir() { + roots.push(format!("{}/", cwd.display())); + } + if let Ok(home) = std::env::var("HOME") { + roots.push(format!("{home}/")); + } + roots } // ---- chain runner ------------------------------------------------------- @@ -229,11 +371,15 @@ async fn run_chain(tool: &str, hook_input: &Value) -> Result<(), String> { for hook in chain { let path = hooks_dir.join(&hook); if !path.is_file() { - // Missing hook is a config error — log but don't block. Better - // to surface it to the user as a stderr-side warning than to - // silently allow the action. - eprintln!("[safe_tools] missing hook (skipped): {}", path.display()); - continue; + // v0.41 fix #1 (Gemini HIGH): FAIL-CLOSED on missing hook. + // Previously we logged a warning and continued — that meant a + // misconfigured deployment (hook deleted, wrong path) silently + // disabled enforcement. Now: refuse the action, surface the + // error so the operator notices. + return Err(format!( + "[policy-chain] hook missing: {} (declared in policy-chain.toml [{}])", + path.display(), tool + )); } let mut child = Command::new(&path) @@ -274,12 +420,19 @@ async fn run_chain(tool: &str, hook_input: &Value) -> Result<(), String> { fn load_chain(tool: &str) -> Result, String> { let path = chain_path()?; if !path.is_file() { - // No policy-chain.toml → unsafe default = pass through with a warning. - // This matches Claude Code's behavior when no hooks are configured. - eprintln!("[safe_tools] no policy-chain.toml at {}; passing through", path.display()); - return Ok(vec![]); + // v0.41 fix #1 (Gemini HIGH companion): default behavior when + // policy-chain.toml is absent is now configurable via env. Without + // explicit opt-in to pass-through, FAIL-CLOSED — caller sees a + // clear error instead of silent bypass. + if std::env::var("KEI_POLICY_CHAIN_OPTIONAL").as_deref() == Ok("1") { + return Ok(vec![]); + } + return Err(format!( + "[policy-chain] config missing: {} (set KEI_POLICY_CHAIN_OPTIONAL=1 to allow pass-through, e.g. for tests)", + path.display() + )); } - let raw = fs::read_to_string(&path) + let raw = std::fs::read_to_string(&path) .map_err(|e| format!("read policy-chain.toml: {e}"))?; let parsed: PolicyChain = toml::from_str(&raw) .map_err(|e| format!("parse policy-chain.toml: {e}"))?; diff --git a/docs/encyclopedia/cross-cli-policy.md b/docs/encyclopedia/cross-cli-policy.md index 9793b70..7839ed0 100644 --- a/docs/encyclopedia/cross-cli-policy.md +++ b/docs/encyclopedia/cross-cli-policy.md @@ -100,10 +100,14 @@ on stdin with `.tool_name` + `.tool_input`, return exit 0 = pass / 2 = block). ### kei-mcp built-in tools -`kei-mcp` (Rust MCP server at `_primitives/_rust/kei-mcp/`) exposes four -built-in tools that bypass atom discovery: +`kei-mcp` (Rust MCP server at `_primitives/_rust/kei-mcp/`) exposes 4 +built-in tools across two source files (both bypass the atom-discovery +loop in `handlers/tools.rs`): +In `handlers/tools.rs`: - `spawn_agent(name, task, on?)` — invokes a KeiSeiKit agent on any backend + +In `handlers/safe_tools.rs` (Phase C, v0.40+): - `kei_bash(command, cwd?)` — runs `[bash]` chain → executes - `kei_edit(file_path, old_string, new_string)` — runs `[edit]` chain → edits - `kei_write(file_path, content)` — runs `[write]` chain → writes @@ -112,6 +116,23 @@ The chain runs against the same hook scripts Claude uses; identical input shape, identical decisions. On block, the hook's stderr surfaces as the MCP error message so the calling agent sees exactly why. +**v0.41 hardening** (post-audit fixes): + +- **Fail-CLOSED on missing config** — if `policy-chain.toml` is absent the + chain refuses to run (was: silent pass-through). Tests / dev can opt in + via `KEI_POLICY_CHAIN_OPTIONAL=1` env. +- **Fail-CLOSED on missing hook script** — if a hook declared in the chain + is not on disk the call fails (was: warn-and-skip). +- **Path-traversal guard** on `kei_edit` / `kei_write` — rejects `..` + segments, `/etc/`, `/usr/`, `/System/`, `/var/`, `/root/`, plus + `$HOME/{.ssh,.aws,.gnupg,.config/gcloud}/` recursively. Override via + `KEI_ALLOWED_ROOTS=':'-separated-absolute-paths`. +- **Async file I/O** — `kei_edit` / `kei_write` now use `tokio::fs` so a + pathological file (`/dev/random` etc.) cannot block a tokio worker. +- **Process-group kill on timeout** — `kei_bash` puts its child shell in + its own process group; on timeout the entire group is `killpg(SIGKILL)`'d + so grandchildren don't orphan (Unix-only; no-op on Windows). + ### Double-enforcement guard If kei-mcp is invoked from a process where `$CLAUDECODE=1` or `$GROKCODE=1`, diff --git a/scripts/kei-agent-cli.sh b/scripts/kei-agent-cli.sh index b63b9ea..e904413 100755 --- a/scripts/kei-agent-cli.sh +++ b/scripts/kei-agent-cli.sh @@ -115,9 +115,21 @@ backend_invoke() { exec "$bin" --agent "$agent_name" --print "${prompt##*TASK FOR THIS RUN:}" fi + # v0.41 fix: headless subprocess invocation of claude/grok without + # --dangerously-skip-permissions returns empty (the agent's system prompt + # asks for Read/Grep tools, but those need permission prompts which can't + # be answered in -p mode). Pass the flag so the agent actually executes. + # Override via KEI_AGENT_PERMISSIVE=0 to keep the strict default. + local permissive_claude="" permissive_grok="" + if [ "${KEI_AGENT_PERMISSIVE:-1}" = "1" ]; then + permissive_claude="--permission-mode=bypassPermissions" + permissive_grok="--always-approve" + fi + case "$backend" in - claude) exec "$bin" -p "$prompt" ;; - grok|agy|antigravity) exec "$bin" --print "$prompt" ;; + claude) exec "$bin" $permissive_claude -p "$prompt" ;; + grok) exec "$bin" $permissive_grok --print "$prompt" ;; + agy|antigravity) exec "$bin" --dangerously-skip-permissions --print "$prompt" ;; copilot) exec "$bin" --prompt "$prompt" ;; kimi) # Kimi has NO one-shot print mode (smoke-tested 2026-05-26): bare `kimi`