fix: security bugs in execpolicy, app-server, and tools
1. Fix deny rule prefix matching without word boundary (execpolicy/lib.rs:351-353) - Deny rule 'rm' now blocks 'rm -rf /' but NOT 'rmdir' or 'rmview' - Previously used bare starts_with which matched any command starting with 'rm' - Add word-boundary check: command must equal rule or start with rule+space 2. Fix fallback prefix match clarity (execpolicy/bash_arity.rs:362-374) - Improve comment to clarify word-boundary matching behavior - The trailing space in starts_with already provides word boundary 3. Fix hardcoded AskForApproval::OnRequest in HTTP API (app-server/lib.rs:283) - Read approval_policy from config instead of hardcoding OnRequest - Users with 'auto'/'yolo' policy now get UnlessTrusted for API calls - Previously ignored user's configured security posture 4. Fix fuzzy indentation search destroying preceding text (tools/file.rs:714-735) - When match starts mid-line after whitespace stripping, use exact position - Previously always expanded to line start, destroying preceding content - Now only expands to line start when match is at a line boundary 5. Fix potential underflow in apply_hunk start index (tools/apply_patch.rs:1110-1115) - Use checked_add_signed to safely handle negative cumulative_offset - Prevents isize overflow on adversarial patch input - Clamp to lines.len() instead of relying on .max(0) cast
This commit is contained in:
@@ -277,12 +277,20 @@ async fn tool_handler(
|
||||
let cwd = req
|
||||
.cwd
|
||||
.unwrap_or_else(|| std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")));
|
||||
// Resolve approval policy from config instead of hardcoding.
|
||||
let approval_mode = {
|
||||
let cfg = state.config.read().await;
|
||||
cfg.approval_policy
|
||||
.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),
|
||||
_ => None,
|
||||
})
|
||||
.unwrap_or(codewhale_execpolicy::AskForApproval::OnRequest)
|
||||
};
|
||||
match runtime
|
||||
.invoke_tool(
|
||||
req.call,
|
||||
codewhale_execpolicy::AskForApproval::OnRequest,
|
||||
&cwd,
|
||||
)
|
||||
.invoke_tool(req.call, approval_mode, &cwd)
|
||||
.await
|
||||
{
|
||||
Ok(value) => Json(value),
|
||||
|
||||
@@ -359,8 +359,9 @@ impl BashArityDict {
|
||||
return true;
|
||||
}
|
||||
|
||||
// Fallback: plain normalised prefix match for patterns not in the table
|
||||
// (preserves backward compatibility with exact-match allow rules).
|
||||
// Fallback: word-boundary prefix match for patterns not in the arity table.
|
||||
// Matches the exact pattern or the pattern followed by a space (i.e., at
|
||||
// word boundary), so "ls" matches "ls" and "ls -la" but NOT "lsof".
|
||||
let command_lower = command.trim().to_ascii_lowercase();
|
||||
// Normalise whitespace in both sides before comparing.
|
||||
let pattern_norm: String = pattern_lower
|
||||
@@ -371,7 +372,8 @@ impl BashArityDict {
|
||||
.split_whitespace()
|
||||
.collect::<Vec<_>>()
|
||||
.join(" ");
|
||||
command_norm == pattern_norm || command_norm.starts_with(&format!("{pattern_norm} "))
|
||||
command_norm == pattern_norm
|
||||
|| command_norm.starts_with(&format!("{pattern_norm} "))
|
||||
}
|
||||
|
||||
/// Iterate over all entries in the dictionary.
|
||||
|
||||
@@ -347,11 +347,13 @@ impl ExecPolicyEngine {
|
||||
pub fn check(&self, ctx: ExecPolicyContext<'_>) -> Result<ExecPolicyDecision> {
|
||||
let normalized = normalize_command(ctx.command);
|
||||
let (trusted_prefixes, denied_prefixes) = self.resolve_prefixes();
|
||||
// Deny rules use simple prefix matching (no arity semantics needed).
|
||||
if let Some(rule) = denied_prefixes
|
||||
.iter()
|
||||
.find(|rule| normalized.starts_with(&normalize_command(rule)))
|
||||
{
|
||||
// Deny rules use word-boundary prefix matching: the command must either
|
||||
// equal the rule or start with the rule followed by a space, so "rm"
|
||||
// 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} "))
|
||||
}) {
|
||||
return Ok(ExecPolicyDecision {
|
||||
allow: false,
|
||||
requires_approval: false,
|
||||
|
||||
@@ -1106,13 +1106,18 @@ fn apply_hunk(
|
||||
.collect();
|
||||
|
||||
// Try to find the location with fuzzy matching
|
||||
// Apply cumulative offset from previous hunks
|
||||
// Apply cumulative offset from previous hunks, clamping to valid range.
|
||||
let base_idx = if hunk.old_start > 0 {
|
||||
hunk.old_start - 1
|
||||
} else {
|
||||
0
|
||||
};
|
||||
let start_idx = ((base_idx as isize) + *cumulative_offset).max(0) as usize;
|
||||
// Use checked_add_signed to safely handle negative offsets without
|
||||
// risking isize overflow on adversarial input.
|
||||
let start_idx = base_idx
|
||||
.checked_add_signed(*cumulative_offset)
|
||||
.unwrap_or(0)
|
||||
.min(lines.len());
|
||||
|
||||
for fuzz in 0..=max_fuzz {
|
||||
// Try at exact position first, then nearby
|
||||
|
||||
@@ -726,7 +726,17 @@ fn leading_whitespace_fuzzy_matches(contents: &str, search: &str) -> Vec<(usize,
|
||||
let Some(&mapped_start) = byte_map.get(norm_start) else {
|
||||
break;
|
||||
};
|
||||
let original_start = line_start_before(contents, mapped_start);
|
||||
// Use the actual match start position, expanding to line start only
|
||||
// when the match begins at a line boundary in the normalized text.
|
||||
// This prevents destroying preceding text on the same line when
|
||||
// the match starts mid-line after whitespace stripping.
|
||||
let original_start = if norm_start == 0 || normalized_contents.as_bytes()[norm_start - 1] == b'\n' {
|
||||
// Match starts at a line boundary — use line start for full-line replacement.
|
||||
line_start_before(contents, mapped_start)
|
||||
} else {
|
||||
// Match starts mid-line — use the exact mapped position.
|
||||
mapped_start
|
||||
};
|
||||
let original_end = byte_map.get(norm_end).copied().unwrap_or(contents.len());
|
||||
matches.push((original_start, original_end));
|
||||
cursor = norm_start.saturating_add(1);
|
||||
|
||||
Reference in New Issue
Block a user