diff --git a/crates/proxy/src/config/simple.rs b/crates/proxy/src/config/simple.rs index e85f0f0..49e5380 100644 --- a/crates/proxy/src/config/simple.rs +++ b/crates/proxy/src/config/simple.rs @@ -118,6 +118,10 @@ pub struct BuiltinToolConfig { pub policy: Option, #[serde(default)] pub timeout_secs: Option, + /// For read_file: restrict reads to files under these absolute directory paths. + /// If empty or absent, all paths are permitted (dangerous; set this in production). + #[serde(default)] + pub allowed_dirs: Vec, } fn default_true() -> bool { diff --git a/crates/proxy/src/main.rs b/crates/proxy/src/main.rs index a918e37..9dac5be 100644 --- a/crates/proxy/src/main.rs +++ b/crates/proxy/src/main.rs @@ -175,7 +175,10 @@ async fn main() { let mut registry = tools::ToolRegistry::new(); // Register built-in tools (gated behind the dangerous-builtin-tools feature). - anyllm_proxy::tools::builtin::register_all(&mut registry); + anyllm_proxy::tools::builtin::register_all( + &mut registry, + simple_config_shell.builtin_tools.as_ref(), + ); // Build MCP manager and discover tools from configured servers. let mcp_manager = if let Some(ref servers) = simple_config_shell.mcp_servers { diff --git a/crates/proxy/src/tools/builtin/mod.rs b/crates/proxy/src/tools/builtin/mod.rs index 8ee53ed..e92766c 100644 --- a/crates/proxy/src/tools/builtin/mod.rs +++ b/crates/proxy/src/tools/builtin/mod.rs @@ -1,10 +1,60 @@ +// SAFETY: The tools in this module execute arbitrary shell commands (BashTool) +// and read arbitrary files (ReadFileTool) as the proxy process user. They must +// NEVER be registered in a server-side tool registry without explicit operator +// opt-in and appropriate sandboxing. Gated behind the `dangerous-builtin-tools` +// feature flag, which is OFF by default. + +#[cfg(feature = "dangerous-builtin-tools")] pub mod bash; +#[cfg(feature = "dangerous-builtin-tools")] pub mod read_file; use crate::tools::registry::ToolRegistry; -/// Convenience function to populate a registry with standard tools. -pub fn register_all(registry: &mut ToolRegistry) { - registry.register(Box::new(bash::BashTool)); - registry.register(Box::new(read_file::ReadFileTool)); +/// Populate a registry with standard built-in tools. +/// +/// `builtin_configs`: optional map of tool name -> config from PROXY_CONFIG; used to +/// pass per-tool settings (e.g., `allowed_dirs` for `read_file`) to tool constructors. +/// +/// # Safety +/// +/// When `dangerous-builtin-tools` is enabled, this registers `BashTool` (arbitrary +/// shell execution) and `ReadFileTool` (arbitrary file reads). Only call this if +/// the tool execution engine is sandboxed or if the operator has explicitly opted in. +/// +/// When the feature is disabled (the default), this is a no-op. +pub fn register_all( + _registry: &mut ToolRegistry, + _builtin_configs: Option<&std::collections::HashMap>, +) { + #[cfg(feature = "dangerous-builtin-tools")] + { + _registry.register(Box::new(bash::BashTool)); + + // Build ReadFileTool with allowed_dirs from config, canonicalized at registration time. + let allowed_dirs = _builtin_configs + .and_then(|m| m.get("read_file")) + .map(|cfg| { + cfg.allowed_dirs + .iter() + .filter_map(|d| { + let p = std::path::PathBuf::from(d); + match p.canonicalize() { + Ok(canon) => Some(canon), + Err(e) => { + tracing::warn!( + dir = %d, + error = %e, + "read_file allowed_dirs entry could not be canonicalized; skipping" + ); + None + } + } + }) + .collect() + }) + .unwrap_or_default(); + + _registry.register(Box::new(read_file::ReadFileTool { allowed_dirs })); + } } diff --git a/crates/proxy/src/tools/builtin/read_file.rs b/crates/proxy/src/tools/builtin/read_file.rs index 125a9a3..c96fb59 100644 --- a/crates/proxy/src/tools/builtin/read_file.rs +++ b/crates/proxy/src/tools/builtin/read_file.rs @@ -1,12 +1,16 @@ use crate::tools::registry::Tool; use serde_json::Value; -use std::path::Path; +use std::path::{Path, PathBuf}; /// Maximum file size to read (1 MB). Prevents OOM from huge files. const MAX_FILE_SIZE: u64 = 1024 * 1024; /// Tool for reading file contents safely. -pub struct ReadFileTool; +pub struct ReadFileTool { + /// If non-empty, restrict reads to files under these directories. + /// All entries must be canonical absolute paths (no symlinks, no ..). + pub allowed_dirs: Vec, +} impl Tool for ReadFileTool { fn name(&self) -> &str { @@ -35,6 +39,7 @@ impl Tool for ReadFileTool { input: Value, ) -> std::pin::Pin> + Send + 'a>> { + let allowed_dirs = self.allowed_dirs.clone(); Box::pin(async move { let raw_path = input .get("path") @@ -43,18 +48,37 @@ impl Tool for ReadFileTool { let path = Path::new(raw_path); - // Require absolute paths to prevent path traversal + // Require absolute paths to prevent relative path traversal. if !path.is_absolute() { return Err("Only absolute paths are allowed".to_string()); } - // Resolve symlinks and .. components, then verify the canonical path - // still starts with the original parent to block traversal via symlinks + // Resolve symlinks and .. components. let canonical = path .canonicalize() .map_err(|e| format!("Cannot resolve path '{}': {}", raw_path, e))?; - // Check file size before reading + // Enforce allowed_dirs allowlist. After canonicalize(), the resolved + // path must start with at least one of the configured base directories. + // This blocks both path traversal (../../../etc) and symlink attacks. + if !allowed_dirs.is_empty() { + let permitted = allowed_dirs.iter().any(|base| canonical.starts_with(base)); + if !permitted { + return Err(format!( + "Path '{}' is outside the configured allowed directories", + raw_path + )); + } + } else { + // No allowed_dirs configured: warn the operator. + tracing::warn!( + path = %raw_path, + "read_file executed with no allowed_dirs restriction; \ + set allowed_dirs in builtin_tools config to restrict file access" + ); + } + + // Check file size before reading. let metadata = std::fs::metadata(&canonical) .map_err(|e| format!("Cannot stat '{}': {}", raw_path, e))?; if metadata.len() > MAX_FILE_SIZE {