-
Notifications
You must be signed in to change notification settings - Fork 0
Harden allowlist-only process execution #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,29 +184,35 @@ 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 \ | ||
| (shell_execution_mode = forbidden)" | ||
| .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(), | ||
| }); | ||
| } | ||
|
Comment on lines
+202
to
+207
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Windows explicit-path commands in the string form are unreachable in allowlist_only The docs now advertise that explicit executable paths can be allowlisted in Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| 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 | ||
| ), | ||
| }) | ||
| } | ||
|
Comment on lines
193
to
218
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Non-trivial security behavior change without a Dev Diary entry This PR is a non-trivial security/behavior change (allowlist_only now rejects all shell-backed forms and binds path entries to a canonical executable). CLAUDE.md/AGENTS.md state that non-trivial behavior changes should ship a (Refers to lines 193-219) Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
@@ -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; | ||
| } | ||
|
Comment on lines
259
to
+262
|
||
| 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()); | ||
| } | ||
|
Comment on lines
+269
to
+293
|
||
| #[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] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
allowlist_onlyis used on Windows, a direct raw command such asexecute command "C:\\Tools\\tool.exe"is classified asneeds_shellbefore this call becausecontains_shell_metacharacterstreats backslashes as shell metacharacters; this new unconditional block then rejects it before the canonical path allowlist check can run, even ifallowed_shell_commandscontains that same executable path. That makes the documented explicit-path allowlist unusable for the common Windows path form unless callers add a dummywith argumentslist or rewrite paths with/, so this should distinguish path separators from actual shell syntax before blocking.Useful? React with 👍 / 👎.