diff --git a/src/modules/command.rs b/src/modules/command.rs index bda958bb..51daa7e8 100644 --- a/src/modules/command.rs +++ b/src/modules/command.rs @@ -12,7 +12,7 @@ use super::{ ParamExt, }; use crate::connection::{Connection, ExecuteOptions}; -use crate::utils::{cmd_escape, powershell_escape, shell_escape}; +use crate::utils::{cmd_arg_escape, powershell_escape, shell_escape}; use once_cell::sync::Lazy; use std::path::Path; use std::process::Command; @@ -89,7 +89,7 @@ impl CommandModule { let escaped_args: Vec> = argv .iter() .map(|arg| match shell_type.as_str() { - "cmd" => cmd_escape(arg), + "cmd" => cmd_arg_escape(arg), "powershell" => powershell_escape(arg), "posix" | "sh" | "bash" => shell_escape(arg), _ => shell_escape(arg), // Default to POSIX for safety/backward compatibility diff --git a/src/modules/mod.rs b/src/modules/mod.rs index b73cf0bd..e0d1ca60 100644 --- a/src/modules/mod.rs +++ b/src/modules/mod.rs @@ -393,11 +393,11 @@ pub fn validate_command_args(args: &str) -> ModuleResult<()> { // If the string contains only safe characters, we can skip the detailed check. // This avoids checking 24 patterns for every safe string (O(N) vs O(M*N)). // - // Safe characters: alphanumeric, space, _, -, ., /, :, +, =, ,, @, % + // Safe characters: alphanumeric, space, _, -, ., /, :, +, =, ,, @ let is_safe = args.bytes().all(|b| matches!(b, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b' ' | b'_' | b'-' | b'.' | b'/' | b':' | - b'+' | b'=' | b',' | b'@' | b'%' + b'+' | b'=' | b',' | b'@' )); if is_safe { @@ -429,6 +429,7 @@ pub fn validate_command_args(args: &str) -> ModuleResult<()> { ("!", "history expansion !"), ("\\", "shell escaping \\"), ("$", "variable expansion $"), + ("%", "variable expansion %"), ("#", "shell comment #"), ]; @@ -2080,6 +2081,7 @@ mod tests { // Extended checks assert!(validate_command_args("bash;echo").is_err()); assert!(validate_command_args("cmd&").is_err()); + assert!(validate_command_args("echo %USERNAME%").is_err()); } #[test] diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 5fa508e7..6b0d2da2 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -187,6 +187,39 @@ pub fn cmd_escape(s: &str) -> Cow<'_, str> { Cow::Owned(escaped) } +/// Escape a string for use as a single argument in Windows cmd.exe. +/// +/// This function wraps the string in double quotes and escapes any internal double quotes, +/// similar to `cmd_escape`. Additionally, it escapes `%` characters by replacing them +/// with `%""` to prevent variable expansion inside the quoted string. +/// +/// This is crucial because `cmd.exe` performs variable expansion even inside double quotes. +/// By inserting an empty string `""` after the `%`, we break the variable name token +/// without changing the value (since `""` is empty), effectively preventing expansion. +/// +/// # Arguments +/// +/// * `s` - The argument string to escape +/// +/// # Returns +/// +/// * The escaped argument string safe for cmd.exe execution as a literal +pub fn cmd_arg_escape(s: &str) -> Cow<'_, str> { + let mut escaped = String::with_capacity(s.len() + 16); + escaped.push('"'); + + for c in s.chars() { + match c { + '"' => escaped.push_str("\"\""), + '%' => escaped.push_str("%\"\""), + _ => escaped.push(c), + } + } + + escaped.push('"'); + Cow::Owned(escaped) +} + /// Escapes a string for safe use in PowerShell commands. /// /// This function handles special characters that could cause issues @@ -321,6 +354,17 @@ mod tests { assert_eq!(cmd_escape(""), "\"\""); } + #[test] + fn test_cmd_arg_escape() { + assert_eq!(cmd_arg_escape("simple"), "\"simple\""); + assert_eq!(cmd_arg_escape("with space"), "\"with space\""); + assert_eq!(cmd_arg_escape("with\"quote"), "\"with\"\"quote\""); + // Verify % is escaped with %"" + assert_eq!(cmd_arg_escape("%USERNAME%"), "\"%\"\"USERNAME%\"\"\""); + assert_eq!(cmd_arg_escape("100%"), "\"100%\"\"\""); + assert_eq!(cmd_arg_escape(""), "\"\""); + } + #[test] fn test_powershell_escape() { assert_eq!(powershell_escape("simple"), "'simple'"); diff --git a/tests/security_windows_command_injection.rs b/tests/security_windows_command_injection.rs new file mode 100644 index 00000000..9e609dba --- /dev/null +++ b/tests/security_windows_command_injection.rs @@ -0,0 +1,43 @@ +use rustible::modules::{validate_command_args, command::CommandModule, Module, ModuleParams, ModuleContext}; +use rustible::utils::cmd_arg_escape; +use std::collections::HashMap; + +#[test] +fn test_validate_command_args_blocks_percent() { + // This should now fail (Err) because we added % to dangerous_patterns + assert!(validate_command_args("echo %USERNAME%").is_err()); +} + +#[test] +fn test_cmd_arg_escape_escapes_percent() { + let input = "%USERNAME%"; + let escaped = cmd_arg_escape(input); + // Should now return "%""USERNAME%" inside quotes (outer quotes + inner escaped quotes) + // cmd_arg_escape wraps in "...", so result is "%""USERNAME%" + // Wait, escaped string content: "%""USERNAME%" + // Representation in Rust string literal: "\"%\"\"USERNAME%\"\"\"" + assert_eq!(escaped, "\"%\"\"USERNAME%\"\"\""); +} + +#[test] +fn test_command_module_argv_escapes_percent() { + let module = CommandModule; + let mut params: ModuleParams = HashMap::new(); + params.insert( + "argv".to_string(), + serde_json::json!(["echo", "%USERNAME%"]), + ); + params.insert("shell_type".to_string(), serde_json::json!("cmd")); + + let context = ModuleContext::default().with_check_mode(true); + + let result = module.execute(¶ms, &context).unwrap(); + let msg = result.msg; + + // msg format: "Would execute: " + // cmd should be: "echo" "%""USERNAME%" + // Expected substring in msg: "\"%\"\"USERNAME%\"\"\"" + + println!("Message: {}", msg); + assert!(msg.contains("\"%\"\"USERNAME%\"\"\"")); +}