diff --git a/Docs/04-advanced-features/subprocess-execution.md b/Docs/04-advanced-features/subprocess-execution.md index e542702f..6648f81d 100644 --- a/Docs/04-advanced-features/subprocess-execution.md +++ b/Docs/04-advanced-features/subprocess-execution.md @@ -23,6 +23,13 @@ shell_execution_mode = sanitized - Policy applies to **both** the shell form and the `with arguments` form. Passing arguments is safer against injection *after* a program is allowed; it is not a bypass of the policy. +- `allowlist_only` permits direct execution only. Shell chaining, pipes, + redirects, expansion, and other shell features are blocked even when the + first command is listed. +- Name-only allowlist entries do not authorize explicit paths with the same + basename. Allow an executable path explicitly when a script must use one. +- Avoid allowlisting shells and interpreters such as `sh`, `cmd.exe`, + PowerShell, or Python: their ordinary arguments can execute additional code. See [Configuration Reference](../reference/configuration-reference.md#security-settings) for full option details. diff --git a/Docs/reference/configuration-reference.md b/Docs/reference/configuration-reference.md index 0798d33b..ac1e6097 100644 --- a/Docs/reference/configuration-reference.md +++ b/Docs/reference/configuration-reference.md @@ -192,7 +192,7 @@ All keys currently loaded from config files, with defaults. |---|---|---|---| | `allow_shell_execution` | bool | `false` | Master switch for all process launches | | `shell_execution_mode` | string | `forbidden` | `forbidden` / `allowlist_only` / `sanitized` / `unrestricted` | -| `allowed_shell_commands` | comma-list | *(empty)* | Program basenames allowed in `allowlist_only` mode | +| `allowed_shell_commands` | comma-list | *(empty)* | Program names or explicit paths allowed in `allowlist_only` mode | | `warn_on_shell_execution` | bool | `true` | Warn whenever a shell command runs | ### Subprocess resources @@ -389,7 +389,7 @@ Applied to every launch, not only shell metacharacter forms. - **Default:** `forbidden` - **Options:** - `forbidden` — no process execution allowed (most secure) - - `allowlist_only` — only programs whose basename is in `allowed_shell_commands` may run + - `allowlist_only` — only direct-exec programs in `allowed_shell_commands` may run; shell features are rejected - `sanitized` — any program may run; shell features produce warnings - `unrestricted` — any program may run with shell; not recommended for production - **Example:** `shell_execution_mode = allowlist_only` @@ -411,9 +411,19 @@ allowed_shell_commands = echo, ls, git #### `allowed_shell_commands` -Comma-separated list of allowed **program basenames** when using -`allowlist_only` mode. Matching uses the basename of the program path -(`/bin/echo` matches `echo`). On Windows, comparison is case-insensitive. +Comma-separated list of allowed program names or explicit executable paths when +using `allowlist_only` mode. A name such as `echo` authorizes only a name-only +invocation resolved through the host process's `PATH`; it does not authorize +`./echo`, `/tmp/echo`, or another caller-selected path with the same basename. +Path-bearing commands require a path-bearing allowlist entry resolving to the +same executable. On Windows, comparison is case-insensitive. + +`allowlist_only` never invokes a shell. Commands containing pipes, redirects, +expansion, command chaining, or other shell features are rejected even when +their first program is allowlisted. Pass data through `with arguments`; opt in +to `sanitized` or `unrestricted` only when shell syntax is genuinely required. +Do not allowlist a shell or interpreter (`sh`, `cmd.exe`, PowerShell, Python, +and similar) unless you intend its arguments to be able to execute code. - **Type:** Comma-separated strings - **Default:** *(empty)* diff --git a/src/interpreter/command_sanitizer.rs b/src/interpreter/command_sanitizer.rs index 55c6db27..17e3f714 100644 --- a/src/interpreter/command_sanitizer.rs +++ b/src/interpreter/command_sanitizer.rs @@ -184,8 +184,6 @@ impl CommandSanitizer { }); } - let program_base = Self::program_basename(program); - match self.config.shell_execution_mode { ShellExecutionMode::Forbidden => Ok(ValidationResult::Blocked { reason: "Subprocess execution is disabled by security policy \ @@ -193,20 +191,28 @@ impl CommandSanitizer { .to_string(), }), ShellExecutionMode::AllowlistOnly => { - if self.is_program_allowlisted(&program_base) { - if needs_shell { - Ok(ValidationResult::RequiresShell { - reason: "Command is allowlisted".to_string(), - warnings: vec!["Using shell execution (allowlisted)".to_string()], - }) - } else { - Ok(ValidationResult::Safe) - } + // An allowlist can authorize one executable, but it cannot + // safely authorize an entire shell command line. If shell + // parsing is allowed here, a command such as + // `echo safe; unlisted-program` passes the `echo` check and the + // shell executes both commands. Require the argv/direct-exec + // form in this mode; callers that intentionally need pipes, + // redirects, expansion, or chaining must opt into `sanitized` + // or `unrestricted` explicitly. + if needs_shell { + return Ok(ValidationResult::Blocked { + reason: "Shell features are not permitted in allowlist_only mode; use the direct-exec form with an explicit argument list" + .to_string(), + }); + } + + if self.is_program_allowlisted(program) { + Ok(ValidationResult::Safe) } else { Ok(ValidationResult::Blocked { reason: format!( "Program '{}' is not in the allowlist (allowed_shell_commands)", - program_base + program ), }) } @@ -251,16 +257,51 @@ impl CommandSanitizer { /// Check if a program (or command string) is in the allowlist pub fn is_allowlisted(&self, command: &str) -> bool { - let base_command = Self::program_basename(&self.get_command_base(command)); - self.is_program_allowlisted(&base_command) + if Self::contains_shell_metacharacters(command) { + return false; + } + let program = Self::parse_command(command) + .map(|(program, _)| program) + .unwrap_or_else(|_| self.get_command_base(command)); + self.is_program_allowlisted(&program) } - fn is_program_allowlisted(&self, program_base: &str) -> bool { + fn is_program_allowlisted(&self, program: &str) -> bool { + let program_has_path = Self::has_path_syntax(program); self.config.allowed_shell_commands.iter().any(|allowed| { + let allowed_has_path = Self::has_path_syntax(allowed); + + // A name-only entry delegates resolution to the trusted process + // environment's PATH. It must not also authorize a caller-selected + // executable at `./name`, `/tmp/name`, or another explicit path. + if program_has_path != allowed_has_path { + return false; + } + if program_has_path { + let Ok(program_path) = std::fs::canonicalize(program) else { + return false; + }; + let Ok(allowed_path) = std::fs::canonicalize(allowed) else { + return false; + }; + + #[cfg(windows)] + { + return program_path + .to_string_lossy() + .eq_ignore_ascii_case(&allowed_path.to_string_lossy()); + } + #[cfg(not(windows))] + { + return program_path == allowed_path; + } + } + + let program_base = Self::program_basename(program); let allowed_base = Self::program_basename(allowed); #[cfg(windows)] { - allowed_base.eq_ignore_ascii_case(program_base) + allowed_base.eq_ignore_ascii_case(&program_base) } #[cfg(not(windows))] { @@ -269,6 +310,11 @@ impl CommandSanitizer { }) } + fn has_path_syntax(program: &str) -> bool { + let trimmed = program.trim().trim_matches('"').trim_matches('\''); + trimmed.contains('/') || trimmed.contains('\\') || trimmed.as_bytes().get(1) == Some(&b':') + } + /// Extract the first whitespace-separated token from a command string fn get_command_base(&self, command: &str) -> String { command.split_whitespace().next().unwrap_or("").to_string() @@ -564,11 +610,68 @@ mod tests { .unwrap(); assert!(matches!(blocked, ValidationResult::Blocked { .. })); - // Path basename matching - let path_ok = sanitizer + // A name-only entry must not authorize a caller-selected path merely + // because the basename is the same. + let path_blocked = sanitizer .authorize_process_execution("/bin/echo", false, "/bin/echo") .unwrap(); - assert_eq!(path_ok, ValidationResult::Safe); + assert!(matches!(path_blocked, ValidationResult::Blocked { .. })); + } + + #[test] + fn test_allowlist_path_requires_the_same_executable() { + let current_executable = std::env::current_exe().expect("current test executable"); + let current_executable = current_executable + .to_str() + .expect("test executable path is UTF-8") + .to_string(); + let config = WflConfig { + allow_shell_execution: true, + shell_execution_mode: ShellExecutionMode::AllowlistOnly, + allowed_shell_commands: vec![current_executable.clone()], + ..Default::default() + }; + let sanitizer = CommandSanitizer::new(Arc::new(config)); + + assert_eq!( + sanitizer + .authorize_process_execution(¤t_executable, false, ¤t_executable) + .unwrap(), + ValidationResult::Safe + ); + let substituted = format!( + "{}/{}", + std::env::temp_dir().display(), + CommandSanitizer::program_basename(¤t_executable) + ); + let result = sanitizer + .authorize_process_execution(&substituted, false, &substituted) + .unwrap(); + assert!(matches!(result, ValidationResult::Blocked { .. })); + } + + #[test] + fn test_allowlist_only_rejects_shell_features_for_allowlisted_program() { + let config = WflConfig { + allow_shell_execution: true, + shell_execution_mode: ShellExecutionMode::AllowlistOnly, + allowed_shell_commands: vec!["echo".to_string()], + ..Default::default() + }; + let sanitizer = CommandSanitizer::new(Arc::new(config)); + + for command in [ + "echo safe; unlisted-program", + "echo safe | unlisted-program", + "echo $(unlisted-program)", + "echo safe > output.txt", + ] { + let result = sanitizer.validate_command(command).unwrap(); + assert!( + matches!(result, ValidationResult::Blocked { .. }), + "allowlist_only must reject shell-backed command {command:?}, got {result:?}" + ); + } } #[test] @@ -584,6 +687,7 @@ mod tests { assert!(sanitizer.is_allowlisted("echo hello")); assert!(sanitizer.is_allowlisted("ls -la")); assert!(!sanitizer.is_allowlisted("rm -rf /")); + assert!(!sanitizer.is_allowlisted("echo safe; rm -rf /")); } #[test] diff --git a/tests/subprocess_security_test.rs b/tests/subprocess_security_test.rs index 1a596dd4..af7513f7 100644 --- a/tests/subprocess_security_test.rs +++ b/tests/subprocess_security_test.rs @@ -43,12 +43,13 @@ allowed_shell_commands = echo warn_on_shell_execution = false "#; -// Windows has no standalone echo.exe; allowlist cmd.exe for opt-in tests. +// Windows has no standalone echo.exe. Use a non-shell executable so the +// allowlist fixture does not itself grant arbitrary `/C` command execution. #[cfg(windows)] const ALLOWLIST_PROGRAM_CONFIG: &str = r#" allow_shell_execution = true shell_execution_mode = allowlist_only -allowed_shell_commands = cmd.exe +allowed_shell_commands = where.exe warn_on_shell_execution = false "#; @@ -283,7 +284,7 @@ fn test_allowlist_only_allows_listed_program() { "#; #[cfg(windows)] let code = r#" - wait for execute command "cmd.exe" with arguments ["/C", "echo allowlisted"] as result + wait for execute command "where.exe" with arguments ["cmd.exe"] as result display result "#; @@ -293,7 +294,31 @@ fn test_allowlist_only_allows_listed_program() { "Allowlisted program should run: {:?}", result ); + #[cfg(not(windows))] assert!(result.unwrap().contains("allowlisted")); + #[cfg(windows)] + assert!(result.unwrap().to_ascii_lowercase().contains("cmd.exe")); +} + +#[test] +fn test_allowlist_only_blocks_shell_chaining_after_allowlisted_program() { + #[cfg(not(windows))] + let code = r#" + execute command "echo allowlisted; echo injected" as result + "#; + #[cfg(windows)] + let code = r#" + execute command "where.exe cmd.exe & echo injected" as result + "#; + + let result = run_wfl_with_config(code, Some(ALLOWLIST_PROGRAM_CONFIG)); + assert_blocked(result.clone(), "Shell chaining after allowlisted program"); + if let Err(err) = result { + assert!( + !err.lines().any(|line| line.trim() == "injected"), + "The unlisted chained payload must not execute: {err}" + ); + } } #[test]