Skip to content

🛡️ Sentinel: [HIGH] Harden Windows command execution against injection #799

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.

Sign up for GitHub

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

Open
adolago wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🛡️ Sentinel: [HIGH] Harden Windows command execution against injection #799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/modules/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -430,6 +430,8 @@ pub fn validate_command_args(args: &str) -> ModuleResult<()> {
("\\", "shell escaping \\"),
("$", "variable expansion $"),
("#", "shell comment #"),
("%", "variable expansion %"),
("^", "cmd.exe escape ^"),
Comment on lines +433 to +434
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Scope %/^ rejection to Windows command paths

Adding % and ^ to the global dangerous_patterns list makes validate_command_args fail for these characters on every platform, but this validator is called unconditionally by modules like CommandModule::validate_params (src/modules/command.rs) and ServiceConfig::from_params (src/modules/service.rs) before any Windows shell selection. This creates a cross-platform regression where legitimate non-Windows arguments (for example format strings containing % or regex anchors with ^) are now rejected even when no cmd.exe expansion is involved.

Useful? React with 👍 / 👎.

];

for (pattern, description) in dangerous_patterns {
Expand Down Expand Up @@ -2090,6 +2092,13 @@ mod tests {
assert!(validate_command_args("bash -c 'echo pwned' #").is_err());
}

#[test]
fn test_validate_command_args_rejects_windows_metachars() {
assert!(validate_command_args("echo %USERNAME%").is_err());
assert!(validate_command_args("echo ^& calc.exe").is_err());
assert!(validate_command_args("%COMSPEC%").is_err());
}

#[test]
fn test_get_remote_tmp() {
// Test default
Expand Down
9 changes: 9 additions & 0 deletions src/utils/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@ pub fn cmd_escape(s: &str) -> Cow<'_, str> {
for c in s.chars() {
if c == '"' {
escaped.push_str("\"\"");
} else if c == '%' {
// Insert empty string ("") after % to prevent variable expansion in cmd.exe
// e.g. %VAR% becomes %""VAR%""
escaped.push_str("%\"\"");
} else {
Comment on lines +166 to 170
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve literal percent semantics in cmd_escape

Rewriting % to %"" is not a stable literal-escaping strategy for cmd.exe and can change the payload instead of preserving it as text (notably %NAME%-style substrings can be interpreted as a variable token with a different name). Since cmd_escape is used when building Windows command strings from argv (src/modules/command.rs), arguments containing literal percent markers can be corrupted at execution time rather than passed through safely.

Useful? React with 👍 / 👎.

escaped.push(c);
}
Expand Down Expand Up @@ -319,6 +323,11 @@ mod tests {
assert_eq!(cmd_escape("foo&bar"), "\"foo&bar\"");
assert_eq!(cmd_escape("foo|bar"), "\"foo|bar\"");
assert_eq!(cmd_escape(""), "\"\"");

// Test environment variable expansion prevention
// % is replaced with %"" which breaks the variable token in cmd.exe
assert_eq!(cmd_escape("%USERNAME%"), "\"%\"\"USERNAME%\"\"\"");
assert_eq!(cmd_escape("foo%bar"), "\"foo%\"\"bar\"");
}

#[test]
Expand Down
Loading