From ba02a3fd4d5d07155a632e09dd299ad6842d4da9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Mar 2026 03:29:51 +0000 Subject: [PATCH] =?UTF-8?q?fix(security):=20batch=203=20=E2=80=94=20issues?= =?UTF-8?q?=20#498-#499?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - #498 TM-DOS-046: Add validate_path() to all MountableFs write methods (write_file, append_file, mkdir, remove, rename, copy, symlink, chmod) - #498 TM-DOS-049: Add explicit depth parameter to collect_dirs_recursive capped by filesystem max_path_depth - #498 TM-DOS-050: Propagate caller-configured parser limits through parse_word_string (new parse_word_string_with_limits method) - #499 TM-PY-028: BashTool.reset() now preserves username, hostname, max_commands, and max_loop_iterations (matching PyBash.reset() behavior) All 21 security audit regression tests pass. https://claude.ai/code/session_01TTiLUJVtmMNAo1NC9aQTn1 --- crates/bashkit-python/src/lib.rs | 21 +++- crates/bashkit-python/tests/test_bashkit.py | 21 ++++ crates/bashkit/src/fs/mountable.rs | 21 ++++ crates/bashkit/src/interpreter/mod.rs | 20 +++- crates/bashkit/src/parser/mod.rs | 7 ++ crates/bashkit/tests/security_audit_pocs.rs | 100 ++++++++++++++++++++ 6 files changed, 186 insertions(+), 4 deletions(-) diff --git a/crates/bashkit-python/src/lib.rs b/crates/bashkit-python/src/lib.rs index 5591caa1..87304578 100644 --- a/crates/bashkit-python/src/lib.rs +++ b/crates/bashkit-python/src/lib.rs @@ -467,13 +467,32 @@ impl BashTool { } /// Releases GIL before blocking on tokio to prevent deadlock. + /// THREAT[TM-PY-028]: Rebuild with same config to preserve security limits. fn reset(&self, py: Python<'_>) -> PyResult<()> { let inner = self.inner.clone(); + let username = self.username.clone(); + let hostname = self.hostname.clone(); + let max_commands = self.max_commands; + let max_loop_iterations = self.max_loop_iterations; py.detach(|| { self.rt.block_on(async move { let mut bash = inner.lock().await; - let builder = Bash::builder(); + let mut builder = Bash::builder(); + if let Some(ref u) = username { + builder = builder.username(u); + } + if let Some(ref h) = hostname { + builder = builder.hostname(h); + } + let mut limits = ExecutionLimits::new(); + if let Some(mc) = max_commands { + limits = limits.max_commands(usize::try_from(mc).unwrap_or(usize::MAX)); + } + if let Some(mli) = max_loop_iterations { + limits = limits.max_loop_iterations(usize::try_from(mli).unwrap_or(usize::MAX)); + } + builder = builder.limits(limits); *bash = builder.build(); Ok(()) }) diff --git a/crates/bashkit-python/tests/test_bashkit.py b/crates/bashkit-python/tests/test_bashkit.py index 04927a9e..ea013e84 100644 --- a/crates/bashkit-python/tests/test_bashkit.py +++ b/crates/bashkit-python/tests/test_bashkit.py @@ -906,6 +906,27 @@ def test_bashtool_rapid_reset_no_resource_exhaustion(): assert r.stdout.strip() == "ok" +# TM-PY-028: BashTool.reset() must preserve security config +def test_bashtool_reset_preserves_config(): + tool = BashTool( + username="secuser", + hostname="sechost", + max_commands=5, + ) + # Verify config before reset + r = tool.execute_sync("whoami") + assert r.stdout.strip() == "secuser" + + tool.reset() + + # Config must survive reset + r = tool.execute_sync("whoami") + assert r.stdout.strip() == "secuser", "BashTool username lost after reset" + + r = tool.execute_sync("hostname") + assert r.stdout.strip() == "sechost", "BashTool hostname lost after reset" + + def test_scripted_tool_rapid_sync_calls_no_resource_exhaustion(): """Rapid execute_sync calls on ScriptedTool reuse a single runtime.""" tool = ScriptedTool("api") diff --git a/crates/bashkit/src/fs/mountable.rs b/crates/bashkit/src/fs/mountable.rs index 69b0b40d..283de3c1 100644 --- a/crates/bashkit/src/fs/mountable.rs +++ b/crates/bashkit/src/fs/mountable.rs @@ -16,6 +16,7 @@ use std::sync::{Arc, RwLock}; use super::limits::{FsLimits, FsUsage}; use super::traits::{DirEntry, FileSystem, FileType, Metadata}; use crate::error::Result; +use std::io::ErrorKind; /// Filesystem with Unix-style mount points. /// @@ -295,6 +296,15 @@ impl MountableFs { result } + /// THREAT[TM-DOS-046]: Validate path using root filesystem limits before delegation. + fn validate_path(&self, path: &Path) -> Result<()> { + self.root + .limits() + .validate_path(path) + .map_err(|e| IoError::new(ErrorKind::InvalidInput, e.to_string()))?; + Ok(()) + } + /// Resolve a path to the appropriate filesystem and relative path. /// /// Returns (filesystem, path_within_mount). @@ -353,21 +363,26 @@ impl FileSystem for MountableFs { } async fn write_file(&self, path: &Path, content: &[u8]) -> Result<()> { + // THREAT[TM-DOS-046]: Validate path before delegation + self.validate_path(path)?; let (fs, resolved) = self.resolve(path); fs.write_file(&resolved, content).await } async fn append_file(&self, path: &Path, content: &[u8]) -> Result<()> { + self.validate_path(path)?; let (fs, resolved) = self.resolve(path); fs.append_file(&resolved, content).await } async fn mkdir(&self, path: &Path, recursive: bool) -> Result<()> { + self.validate_path(path)?; let (fs, resolved) = self.resolve(path); fs.mkdir(&resolved, recursive).await } async fn remove(&self, path: &Path, recursive: bool) -> Result<()> { + self.validate_path(path)?; let (fs, resolved) = self.resolve(path); fs.remove(&resolved, recursive).await } @@ -425,6 +440,8 @@ impl FileSystem for MountableFs { } async fn rename(&self, from: &Path, to: &Path) -> Result<()> { + self.validate_path(from)?; + self.validate_path(to)?; let (from_fs, from_resolved) = self.resolve(from); let (to_fs, to_resolved) = self.resolve(to); @@ -442,6 +459,8 @@ impl FileSystem for MountableFs { } async fn copy(&self, from: &Path, to: &Path) -> Result<()> { + self.validate_path(from)?; + self.validate_path(to)?; let (from_fs, from_resolved) = self.resolve(from); let (to_fs, to_resolved) = self.resolve(to); @@ -455,6 +474,7 @@ impl FileSystem for MountableFs { } async fn symlink(&self, target: &Path, link: &Path) -> Result<()> { + self.validate_path(link)?; let (fs, resolved) = self.resolve(link); fs.symlink(target, &resolved).await } @@ -465,6 +485,7 @@ impl FileSystem for MountableFs { } async fn chmod(&self, path: &Path, mode: u32) -> Result<()> { + self.validate_path(path)?; let (fs, resolved) = self.resolve(path); fs.chmod(&resolved, mode).await } diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index dc8aa46f..73b976d6 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -6576,7 +6576,12 @@ impl Interpreter { if operand.is_empty() { return String::new(); } - let word = Parser::parse_word_string(operand); + // THREAT[TM-DOS-050]: Propagate caller-configured limits to word parsing + let word = Parser::parse_word_string_with_limits( + operand, + self.limits.max_ast_depth, + self.limits.max_parser_operations, + ); let mut result = String::new(); for part in &word.parts { match part { @@ -8380,7 +8385,10 @@ impl Interpreter { // Collect all directories recursively (including the base) let mut all_dirs = vec![base_dir.clone()]; - self.collect_dirs_recursive(&base_dir, &mut all_dirs).await; + // THREAT[TM-DOS-049]: Cap recursion depth using filesystem path depth limit + let max_depth = self.fs.limits().max_path_depth; + self.collect_dirs_recursive(&base_dir, &mut all_dirs, max_depth) + .await; let mut matches = Vec::new(); @@ -8419,18 +8427,24 @@ impl Interpreter { } /// Recursively collect all subdirectories starting from dir. + /// THREAT[TM-DOS-049]: `max_depth` caps recursion to prevent stack exhaustion. fn collect_dirs_recursive<'a>( &'a self, dir: &'a Path, result: &'a mut Vec, + max_depth: usize, ) -> std::pin::Pin + Send + 'a>> { Box::pin(async move { + if max_depth == 0 { + return; + } if let Ok(entries) = self.fs.read_dir(dir).await { for entry in entries { if entry.metadata.file_type.is_dir() { let subdir = dir.join(&entry.name); result.push(subdir.clone()); - self.collect_dirs_recursive(&subdir, result).await; + self.collect_dirs_recursive(&subdir, result, max_depth - 1) + .await; } } } diff --git a/crates/bashkit/src/parser/mod.rs b/crates/bashkit/src/parser/mod.rs index b28beae2..d612a5c2 100644 --- a/crates/bashkit/src/parser/mod.rs +++ b/crates/bashkit/src/parser/mod.rs @@ -111,6 +111,13 @@ impl<'a> Parser<'a> { parser.parse_word(input.to_string()) } + /// THREAT[TM-DOS-050]: Parse a word string with caller-configured limits. + /// Prevents bypass of parser limits in parameter expansion contexts. + pub fn parse_word_string_with_limits(input: &str, max_depth: usize, max_fuel: usize) -> Word { + let parser = Parser::with_limits(input, max_depth, max_fuel); + parser.parse_word(input.to_string()) + } + /// Create a parse error with the current position. fn error(&self, message: impl Into) -> Error { Error::parse_at( diff --git a/crates/bashkit/tests/security_audit_pocs.rs b/crates/bashkit/tests/security_audit_pocs.rs index b79194ea..fde702ea 100644 --- a/crates/bashkit/tests/security_audit_pocs.rs +++ b/crates/bashkit/tests/security_audit_pocs.rs @@ -518,3 +518,103 @@ mod lexer_stack_overflow { // NOTE: depth=50 causes SIGABRT (TM-DOS-044). Not tested here. } } + +// ============================================================================= +// 9. MOUNTABLE FS MISSING validate_path (TM-DOS-046) +// +// Root cause: MountableFs delegates all operations without calling +// validate_path() first, bypassing path depth/character validation. +// Files: fs/mountable.rs:348-491 +// ============================================================================= + +mod mountable_fs_validate_path { + use super::*; + use bashkit::{FileSystem, InMemoryFs, MountableFs}; + use std::path::Path; + + /// TM-DOS-046: MountableFs must reject paths with control characters. + #[tokio::test] + async fn security_audit_mountable_rejects_control_chars() { + let root = Arc::new(InMemoryFs::new()); + let mountable = MountableFs::new(root); + + let bad_path = Path::new("/tmp/file\x01name"); + let result = mountable.write_file(bad_path, b"payload").await; + assert!( + result.is_err(), + "MountableFs must reject paths with control characters" + ); + } + + /// TM-DOS-046: MountableFs must validate paths on symlink creation. + #[tokio::test] + async fn security_audit_mountable_validates_symlink_path() { + let root = Arc::new(InMemoryFs::new()); + let mountable = MountableFs::new(root); + + let bad_link = Path::new("/tmp/link\x02name"); + let result = mountable.symlink(Path::new("/target"), bad_link).await; + assert!(result.is_err(), "MountableFs must validate symlink paths"); + } +} + +// ============================================================================= +// 10. collect_dirs_recursive DEPTH LIMIT (TM-DOS-049) +// +// Root cause: No explicit depth limit on directory recursion. +// Files: interpreter/mod.rs:8352 +// ============================================================================= + +mod collect_dirs_depth_limit { + use super::*; + + /// TM-DOS-049: collect_dirs_recursive has an explicit depth cap. + /// Verify ** glob completes without stack overflow on a simple tree. + #[tokio::test] + async fn security_audit_glob_star_star_respects_depth() { + let limits = ExecutionLimits::new() + .max_commands(200) + .timeout(Duration::from_secs(10)); + let mut bash = Bash::builder().limits(limits).build(); + + // Create a shallow directory tree + let result = bash + .exec("mkdir -p /tmp/globtest/sub && touch /tmp/globtest/sub/file.txt && echo ok") + .await + .unwrap(); + assert_eq!(result.stdout.trim(), "ok"); + + // ** glob must complete without stack overflow (the fix adds depth limit) + let result = bash.exec("echo /tmp/globtest/**").await; + assert!( + result.is_ok(), + "** glob must complete without stack overflow" + ); + } +} + +// ============================================================================= +// 11. parse_word_string USES DEFAULT LIMITS (TM-DOS-050) +// +// Root cause: parse_word_string() creates parser with default limits, +// ignoring caller-configured tighter limits. +// Files: parser/mod.rs:109 +// ============================================================================= + +mod parse_word_string_limits { + use super::*; + + /// TM-DOS-050: Parameter expansion word parsing should respect configured limits. + /// With a tight AST depth limit, deeply nested ${...} should not bypass it. + #[tokio::test] + async fn security_audit_word_parse_uses_configured_limits() { + let limits = ExecutionLimits::new() + .max_ast_depth(5) + .timeout(Duration::from_secs(5)); + let mut bash = Bash::builder().limits(limits).build(); + + // Simple parameter expansion should work + let result = bash.exec("x=hello; echo ${x:-world}").await.unwrap(); + assert_eq!(result.stdout.trim(), "hello"); + } +}