fix: address security review comments
1. Fix whitespace bypass in normalize_command (execpolicy/lib.rs:446)
- Collapse internal whitespace to prevent 'git status' bypassing 'git status'
- split_whitespace().join(' ') normalizes all whitespace
2. Fix 'never'/'deny' approval mapping (app-server/lib.rs:287)
- Map to AskForApproval::Never instead of OnRequest
- 'never'/'deny' should forbid commands, not prompt for approval
3. Optimize prefix matching (execpolicy/lib.rs:355, bash_arity.rs:375)
- Avoid format! allocation on every check
- Use byte comparison for space boundary check
This commit is contained in:
@@ -284,7 +284,7 @@ async fn tool_handler(
|
||||
.as_deref()
|
||||
.and_then(|p| match p.trim().to_ascii_lowercase().as_str() {
|
||||
"auto" | "yolo" => Some(codewhale_execpolicy::AskForApproval::UnlessTrusted),
|
||||
"never" | "deny" => Some(codewhale_execpolicy::AskForApproval::OnRequest),
|
||||
"never" | "deny" => Some(codewhale_execpolicy::AskForApproval::Never),
|
||||
_ => None,
|
||||
})
|
||||
.unwrap_or(codewhale_execpolicy::AskForApproval::OnRequest)
|
||||
|
||||
@@ -373,7 +373,8 @@ impl BashArityDict {
|
||||
.collect::<Vec<_>>()
|
||||
.join(" ");
|
||||
command_norm == pattern_norm
|
||||
|| command_norm.starts_with(&format!("{pattern_norm} "))
|
||||
|| (command_norm.starts_with(&pattern_norm)
|
||||
&& command_norm.as_bytes().get(pattern_norm.len()) == Some(&b' '))
|
||||
}
|
||||
|
||||
/// Iterate over all entries in the dictionary.
|
||||
|
||||
@@ -352,7 +352,9 @@ impl ExecPolicyEngine {
|
||||
// blocks "rm -rf /" but NOT "rmdir" or "rmview".
|
||||
if let Some(rule) = denied_prefixes.iter().find(|rule| {
|
||||
let norm_rule = normalize_command(rule);
|
||||
normalized == norm_rule || normalized.starts_with(&format!("{norm_rule} "))
|
||||
normalized == norm_rule
|
||||
|| (normalized.starts_with(&norm_rule)
|
||||
&& normalized.as_bytes().get(norm_rule.len()) == Some(&b' '))
|
||||
}) {
|
||||
return Ok(ExecPolicyDecision {
|
||||
allow: false,
|
||||
@@ -444,7 +446,12 @@ impl ExecPolicyEngine {
|
||||
}
|
||||
|
||||
fn normalize_command(value: &str) -> String {
|
||||
value.trim().to_ascii_lowercase()
|
||||
// Normalize: lowercase, collapse internal whitespace to single spaces.
|
||||
// This prevents bypass via "git status" (double space) vs "git status".
|
||||
value.split_whitespace()
|
||||
.collect::<Vec<_>>()
|
||||
.join(" ")
|
||||
.to_ascii_lowercase()
|
||||
}
|
||||
|
||||
fn first_token(command: &str) -> String {
|
||||
|
||||
Reference in New Issue
Block a user