Tighten bash permission model against env-prefix, whitespace, and merge bypasses

Author: alexylon

CHANGELOG.md

@@ -4,6 +4,13 @@ All notable changes to Sofos are documented in this file.
 
 ## [Unreleased]
 
+### Security
+
+- **A global deny rule survives a local allow with the same name.** Adding `Bash(rm)` to `.sofos/config.local.toml`'s allow list used to silently strip the matching `Bash(rm)` from `~/.sofos/config.toml`'s deny list; both entries now coexist after merge, so the per-command verdict reflects every configured rule instead of dropping the global guarantee.
+- **`PATH=`, `LD_PRELOAD=`, `LD_LIBRARY_PATH=`, `DYLD_*=`, `NODE_PATH=`, and `PYTHONPATH=` prefixes now route the bash call to a confirmation prompt.** A command like `PATH=. cargo build` used to auto-allow as `cargo`; sofos now asks the user before running anything that swaps the binary the shell will execute, even when the base command is on the allow list or when blanket `Bash` allow is active. Built-in forbidden bases (`rm`, `chmod`, `sudo`, …) still take precedence and stay denied.
+- **A session-scoped Bash path grant now applies only to the file the user named.** Allowing `cat /home/me/.ssh/config` once used to permit every other file under `/home/me/.ssh` for the rest of the session; the grant is now scoped to the specific file, so a follow-up `cat /home/me/.ssh/id_rsa` re-prompts. Grants saved to config (yes-and-remember) still cover the whole `parent/**` directory because the user explicitly opts in to that wider scope.
+- **Repeating a denied command with extra whitespace no longer dodges the session deny.** `ls /etc` and `ls  /etc` (any internal whitespace) now hash to the same session-scoped key, so a model that retries a refused command cannot trigger a fresh prompt by adding spaces or tabs.
+
 ## [0.3.1] - 2026-05-19
 
 ### Added

src/tools/bash/executor.rs

@@ -114,13 +114,14 @@ impl BashExecutor {
     }
 
     pub fn execute(&self, command: &str) -> Result<String> {
-        let normalized = format!("Bash({})", command.trim());
+        let mut permission_manager = PermissionManager::new(self.workspace.clone())?;
+        let normalized = PermissionManager::normalize_command(command);
 
         // Check session-scoped decisions first (for "allow once" / "deny once")
         if let Ok(allowed) = self.session_allowed.lock() {
             if allowed.contains(&normalized) {
                 // Previously allowed this session, skip permission check
-                return self.execute_after_permission_check(command);
+                return self.execute_after_permission_check(command, &mut permission_manager);
             }
         }
         if let Ok(denied) = self.session_denied.lock() {
@@ -134,7 +135,6 @@ impl BashExecutor {
             }
         }
 
-        let mut permission_manager = PermissionManager::new(self.workspace.clone())?;
         let permission = permission_manager.check_command_permission(command)?;
 
         match permission {
@@ -171,14 +171,16 @@ impl BashExecutor {
             }
         }
 
-        self.execute_after_permission_check(command)
+        self.execute_after_permission_check(command, &mut permission_manager)
     }
 
-    fn execute_after_permission_check(&self, command: &str) -> Result<String> {
-        let mut permission_manager = PermissionManager::new(self.workspace.clone())?;
-
+    fn execute_after_permission_check(
+        &self,
+        command: &str,
+        permission_manager: &mut PermissionManager,
+    ) -> Result<String> {
         // Enforce read permissions on paths referenced in the command
-        self.enforce_read_permissions(&permission_manager, command)?;
+        self.enforce_read_permissions(permission_manager, command)?;
 
         // Non-path structural safety checks (parent traversal, redirection, git restrictions)
         if !self.is_safe_command_structure(command) {
@@ -197,7 +199,7 @@ impl BashExecutor {
         self.confirm_askable_command(command)?;
 
         // Check external paths in command — ask user for paths not covered by Bash path grants
-        self.check_bash_external_paths(command, &mut permission_manager)?;
+        self.check_bash_external_paths(command, permission_manager)?;
 
         let outcome = self.spawn_supervised(command)?;
 

src/tools/bash/validate.rs

@@ -322,14 +322,19 @@ impl BashExecutor {
         if allowed {
             if !remember {
                 if let Ok(mut dirs) = self.bash_path_session_allowed.lock() {
-                    dirs.insert(parent.to_string());
+                    // Session-only grant: store the file path itself, not
+                    // the parent, so a second file under the same parent
+                    // re-prompts. Persistent grants (remember=true) keep
+                    // `Bash(parent/**)` because the user explicitly
+                    // opted in to that scope through `ask_user_path_permission`.
+                    dirs.insert(check_path.to_string());
                 }
             }
             Ok(())
         } else {
             if !remember {
                 if let Ok(mut dirs) = self.bash_path_session_denied.lock() {
-                    dirs.insert(parent.to_string());
+                    dirs.insert(check_path.to_string());
                 }
             }
             Err(SofosError::ToolExecution(format!(

src/tools/permissions/command_parse.rs

@@ -26,6 +26,41 @@ pub(super) const COMPOUND_HEADERS_WITH_BODY: &[&str] = &["while", "until", "if"]
 /// base command at all and should yield no entry.
 pub(super) const COMPOUND_HEADERS_NO_BODY: &[&str] = &["for"];
 
+/// Env-assignment keys whose value can swap the binary the shell ends
+/// up running, regardless of what base command follows. `PATH=. cargo
+/// build` reads `cargo` from the current directory; `LD_PRELOAD=evil.so
+/// ls` runs `ls` under a hijacked loader. The base-command lookup
+/// would otherwise classify these as `cargo` / `ls` and auto-allow,
+/// so the permission system has to surface them explicitly.
+const DANGEROUS_ENV_KEYS: &[&str] = &[
+    "PATH",
+    "LD_PRELOAD",
+    "LD_LIBRARY_PATH",
+    "NODE_PATH",
+    "PYTHONPATH",
+];
+
+/// Returns the dangerous env-key (verbatim, uppercased) found among
+/// the leading env-assignment tokens of `segment`. Stops at the first
+/// token that is not a `KEY=value`, since that's the base command and
+/// any later occurrence isn't an env prefix any more.
+pub(super) fn leading_dangerous_env_prefix(segment: &str) -> Option<&'static str> {
+    for tok in segment.split_whitespace() {
+        if !is_env_assignment(tok) {
+            return None;
+        }
+        let key = tok.split_once('=').map(|(k, _)| k).unwrap_or(tok);
+        let upper = key.to_ascii_uppercase();
+        if upper.starts_with("DYLD_") {
+            return Some("DYLD_*");
+        }
+        if let Some(name) = DANGEROUS_ENV_KEYS.iter().find(|d| **d == upper.as_str()) {
+            return Some(*name);
+        }
+    }
+    None
+}
+
 /// Match POSIX-shell `KEY=value` assignment tokens: the key starts with
 /// a letter or underscore, is alphanumeric+`_` throughout, and is
 /// followed by `=`. Used by `extract_base_command` to skip leading

src/tools/permissions/manager.rs

@@ -1,6 +1,6 @@
 use crate::error::{Result, SofosError};
 use crate::tools::permissions::CommandPermission;
-use crate::tools::permissions::command_parse::command_lookup_key;
+use crate::tools::permissions::command_parse::{command_lookup_key, leading_dangerous_env_prefix};
 use crate::tools::permissions::pattern::BLANKET_BASH;
 use crate::tools::permissions::settings::PermissionSettings;
 use crate::tools::utils::{ConfirmationType, confirm_multi_choice};
@@ -406,6 +406,18 @@ impl PermissionManager {
             return Ok(CommandPermission::Denied);
         }
 
+        // Dangerous env prefixes (`PATH=.`, `LD_PRELOAD=...`, ...) can
+        // swap the binary the shell ends up running, so an Allowed
+        // verdict gets downgraded to Ask. Denies still fire normally
+        // — an explicit `Bash(...)` deny or a forbidden base wins
+        // because the deny is the conservative choice either way.
+        let dangerous_env = Self::command_has_dangerous_env_prefix(command);
+        let allow_verdict = if dangerous_env {
+            CommandPermission::Ask
+        } else {
+            CommandPermission::Allowed
+        };
+
         let normalized = Self::normalize_command(command);
         let base_command = Self::extract_base_command(command);
 
@@ -429,11 +441,11 @@ impl PermissionManager {
             {
                 return Ok(CommandPermission::Denied);
             }
-            return Ok(CommandPermission::Allowed);
+            return Ok(allow_verdict);
         }
 
         if self.settings.permissions.allow.contains(&normalized) {
-            return Ok(CommandPermission::Allowed);
+            return Ok(allow_verdict);
         }
 
         if self.settings.permissions.deny.contains(&normalized) {
@@ -442,7 +454,7 @@ impl PermissionManager {
 
         let wildcard_pattern = format!("Bash({}:*)", base_command);
         if self.settings.permissions.allow.contains(&wildcard_pattern) {
-            return Ok(CommandPermission::Allowed);
+            return Ok(allow_verdict);
         }
 
         if self.settings.permissions.deny.contains(&wildcard_pattern) {
@@ -478,7 +490,7 @@ impl PermissionManager {
             .iter()
             .all(|b| self.allowed_commands.contains(&command_lookup_key(b)))
         {
-            return Ok(CommandPermission::Allowed);
+            return Ok(allow_verdict);
         }
 
         Ok(CommandPermission::Ask)
@@ -497,6 +509,16 @@ impl PermissionManager {
         }
     }
 
+    /// True when any sub-command of a compound shell leads with a
+    /// dangerous env-assignment (`PATH=`, `LD_PRELOAD=`, etc.). Walking
+    /// every segment means `ls; PATH=. cargo build` is caught even though
+    /// the first segment is plain.
+    pub(super) fn command_has_dangerous_env_prefix(command: &str) -> bool {
+        Self::split_compound_command(command)
+            .iter()
+            .any(|seg| leading_dangerous_env_prefix(seg).is_some())
+    }
+
     pub fn ask_user_permission(&mut self, command: &str) -> Result<(bool, bool)> {
         let normalized = Self::normalize_command(command);
         let prompt = format!("Allow command `{}`?", command);

src/tools/permissions/mod.rs

@@ -1609,6 +1609,121 @@ ask = []
         assert_eq!(bases, vec!["echo".to_string(), "sudo".to_string()]);
     }
 
+    /// `PATH=. cargo build` previously classified as `cargo` (in the
+    /// allowed set), letting an attacker-controlled `./cargo` execute
+    /// under the auto-allow. The same trick works with `LD_PRELOAD`,
+    /// `DYLD_*`, `NODE_PATH`, `PYTHONPATH`. All must now route through
+    /// the Ask prompt regardless of the base command.
+    #[test]
+    fn dangerous_env_prefix_forces_ask() {
+        let temp_dir = TempDir::new().unwrap();
+        let mut manager = PermissionManager::new(temp_dir.path().to_path_buf()).unwrap();
+
+        for cmd in [
+            "PATH=. cargo build",
+            "LD_PRELOAD=./evil.so ls",
+            "LD_LIBRARY_PATH=. ./bin",
+            "DYLD_INSERT_LIBRARIES=./evil.dylib ls",
+            "DYLD_LIBRARY_PATH=. ls",
+            "NODE_PATH=. node script.js",
+            "PYTHONPATH=. python script.py",
+        ] {
+            assert_eq!(
+                manager.check_command_permission(cmd).unwrap(),
+                CommandPermission::Ask,
+                "dangerous env prefix should force Ask: {cmd}"
+            );
+        }
+
+        // Harmless env prefixes (PII-style, `FOO=bar`) still fall through
+        // to the base-command check.
+        assert_eq!(
+            manager
+                .check_command_permission("FOO=bar cargo build")
+                .unwrap(),
+            CommandPermission::Allowed
+        );
+    }
+
+    /// Even a blanket `"Bash"` allow does NOT short-circuit the
+    /// dangerous-env check — the user opted in to "trust this command
+    /// family", not to "swap my PATH".
+    #[test]
+    fn dangerous_env_prefix_overrides_blanket_allow() {
+        let temp_dir = TempDir::new().unwrap();
+        let mut manager = PermissionManager::new(temp_dir.path().to_path_buf()).unwrap();
+        manager.settings.permissions.allow.push("Bash".to_string());
+
+        assert_eq!(
+            manager
+                .check_command_permission("PATH=. cargo build")
+                .unwrap(),
+            CommandPermission::Ask
+        );
+    }
+
+    /// A dangerous env prefix in any segment of a compound shell trips
+    /// the gate — `ls; PATH=. cargo` still routes to Ask.
+    #[test]
+    fn dangerous_env_prefix_in_compound_segment_caught() {
+        let temp_dir = TempDir::new().unwrap();
+        let mut manager = PermissionManager::new(temp_dir.path().to_path_buf()).unwrap();
+
+        assert_eq!(
+            manager
+                .check_command_permission("ls; PATH=. cargo build")
+                .unwrap(),
+            CommandPermission::Ask
+        );
+        assert_eq!(
+            manager
+                .check_command_permission("cat foo && LD_PRELOAD=evil.so ls")
+                .unwrap(),
+            CommandPermission::Ask
+        );
+    }
+
+    /// A `Bash(rm)` rule in local-allow must NOT silently strip the
+    /// matching `Bash(rm)` from global-deny. Per-list seen sets keep the
+    /// deny entry around (the runtime then arbitrates).
+    #[test]
+    fn merge_does_not_drop_global_deny_for_local_allow() {
+        let mut global = PermissionSettings::default();
+        global.permissions.deny.push("Bash(rm)".to_string());
+
+        let mut local = PermissionSettings::default();
+        local.permissions.allow.push("Bash(rm)".to_string());
+
+        global.merge(local);
+
+        assert!(
+            global.permissions.deny.contains(&"Bash(rm)".to_string()),
+            "global deny must survive a local-allow with the same key"
+        );
+        assert!(
+            global.permissions.allow.contains(&"Bash(rm)".to_string()),
+            "local allow is still merged in"
+        );
+    }
+
+    /// Whitespace in the command must NOT bypass a session-scoped deny.
+    /// `Bash(ls /etc)` and `Bash(ls   /etc)` must produce the same key.
+    #[test]
+    fn normalize_command_collapses_internal_whitespace() {
+        assert_eq!(
+            PermissionManager::normalize_command("ls /etc"),
+            PermissionManager::normalize_command("ls  /etc"),
+        );
+        assert_eq!(
+            PermissionManager::normalize_command("ls\t/etc"),
+            PermissionManager::normalize_command("ls /etc"),
+        );
+        assert_eq!(
+            PermissionManager::normalize_command("  ls /etc  "),
+            "Bash(ls /etc)".to_string()
+        );
+    }
+
     /// `./secrets/../allowed.txt` lexically resolves to `./allowed.txt`,
     /// so the deny rule `Read(./secrets/**)` must NOT match it. The
     /// glob deny on `./secrets/**` must still catch the un-normalised

src/tools/permissions/pattern.rs

@@ -5,7 +5,7 @@
 //! place to land.
 
 use crate::tools::permissions::PermissionManager;
-use crate::tools::utils::is_absolute_path;
+use crate::tools::utils::{is_absolute_path, normalize_command_whitespace};
 
 /// Bare `"Bash"` in an `allow` or `deny` list acts as a blanket rule
 /// over all bash commands. In `allow` it auto-passes everything except
@@ -37,8 +37,15 @@ impl PermissionManager {
         None
     }
 
-    pub(super) fn normalize_command(command: &str) -> String {
-        format!("Bash({})", command.trim())
+    /// Canonical `Bash(...)` shape for a command string. Internal
+    /// whitespace runs are collapsed so `ls /etc` and `ls  /etc` resolve
+    /// to the same rule key — a session-scoped deny on the first form
+    /// stays in force when the model retries with extra spacing. Exposed
+    /// `pub` so the bash executor can key session state through the
+    /// same normaliser as the rule lookups.
+    pub fn normalize_command(command: &str) -> String {
+        let collapsed = normalize_command_whitespace(command);
+        format!("Bash({})", collapsed.trim())
     }
 
     pub(super) fn normalize_read(path: &str) -> String {

src/tools/permissions/scope.rs

@@ -228,6 +228,14 @@ impl PermissionManager {
             }
         }
 
+        // Default-Allowed when no explicit rule matches. Inside-workspace
+        // reads are expected to pass through; paths that leave the
+        // workspace are gated separately by
+        // `BashExecutor::check_bash_external_paths`, which runs the
+        // interactive prompt and the deny-glob check on its own. Do NOT
+        // collapse these two paths without preserving that boundary —
+        // the bash side enforces explicit confirmation for external
+        // paths, which this default branch would skip.
         CommandPermission::Allowed
     }