diff --git a/app/src-tauri/src/cdp/session.rs b/app/src-tauri/src/cdp/session.rs index d110a995d..ae0550042 100644 --- a/app/src-tauri/src/cdp/session.rs +++ b/app/src-tauri/src/cdp/session.rs @@ -524,17 +524,19 @@ async fn run_session_cycle( // - audioCapture / videoCapture: getUserMedia for cam/mic so the // pre-call greenroom auto-grants instead of falling back to // Meet's "Use microphone and camera" consent dialog - // - displayCapture: getDisplayMedia for screen-share present // - clipboardReadWrite: copy meeting link / paste join code // Without these, Meet sits on the consent dialog forever and cam/mic // never enumerate (verified during #1022 smoke). + // + // displayCapture is intentionally NOT in this set. Pre-granting it + // via `Browser.grantPermissions` bypasses the transient-activation + // requirement Chromium enforces on `getDisplayMedia`, which would + // let the page initiate a desktop capture without any user gesture. + // Without the pre-grant the page's screen-share button triggers + // Chrome's native screen-picker on click — same UX, but the gesture + // gate stays in place. if origin_host_is(&origin, "meet.google.com") { - perms.extend_from_slice(&[ - "audioCapture", - "videoCapture", - "displayCapture", - "clipboardReadWrite", - ]); + perms.extend_from_slice(&["audioCapture", "videoCapture", "clipboardReadWrite"]); } // Slack Huddles need the same media-capture set as Meet: @@ -542,7 +544,6 @@ async fn run_session_cycle( // optional camera tile. Without these, the huddle pre-flight // enumerateDevices returns empty and the join button silently // no-ops. - // - displayCapture: getDisplayMedia for in-huddle screen share. // - clipboardReadWrite: huddle invite-link copy + slash-command // paste flows. // Mirrors the gmeet pattern from #1054. The huddle popup paint @@ -550,13 +551,13 @@ async fn run_session_cycle( // tracking issue — granting these perms now means once the paint // bug clears, the huddle is functional immediately rather than // requiring a follow-up perms wire-up. + // + // displayCapture deliberately omitted for the same reason as Meet: + // pre-granting bypasses Chromium's gesture gate on + // `getDisplayMedia`; screen-share inside a huddle still works via + // the native screen-picker on user click. if origin_host_is(&origin, "app.slack.com") { - perms.extend_from_slice(&[ - "audioCapture", - "videoCapture", - "displayCapture", - "clipboardReadWrite", - ]); + perms.extend_from_slice(&["audioCapture", "videoCapture", "clipboardReadWrite"]); } if let Err(e) = cdp diff --git a/src/openhuman/memory_tools/tools/put.rs b/src/openhuman/memory_tools/tools/put.rs index 1355afe3b..872456378 100644 --- a/src/openhuman/memory_tools/tools/put.rs +++ b/src/openhuman/memory_tools/tools/put.rs @@ -211,6 +211,9 @@ mod tests { #[tokio::test] async fn execute_success_path_persists_rule_in_isolated_workspace() { + let _serial = crate::openhuman::memory::ops::GLOBAL_MEMORY_TEST_LOCK + .lock() + .await; let tmp = TempDir::new().expect("tempdir"); let (_workspace, _cfg) = isolated_config(&tmp).await; let tool = MemoryToolsPutTool; @@ -250,6 +253,9 @@ mod tests { #[tokio::test] async fn execute_defaults_unknown_priority_to_normal() { + let _serial = crate::openhuman::memory::ops::GLOBAL_MEMORY_TEST_LOCK + .lock() + .await; let tmp = TempDir::new().expect("tempdir"); let (_workspace, _cfg) = isolated_config(&tmp).await; let tool = MemoryToolsPutTool; diff --git a/src/openhuman/security/policy.rs b/src/openhuman/security/policy.rs index 2071184eb..d25c9ff52 100644 --- a/src/openhuman/security/policy.rs +++ b/src/openhuman/security/policy.rs @@ -246,6 +246,98 @@ impl Default for SecurityPolicy { } } +/// Environment variable names that can trigger arbitrary command execution +/// when supplied as a leading inline assignment on an otherwise-allowed +/// command. Each name here is either a hook variable that a downstream tool +/// will spawn as a subprocess (`GIT_PAGER`, `GIT_SSH_COMMAND`, `EDITOR`, +/// `LESS`/`LESSOPEN`, `MANPAGER`, `BROWSER`, `BAT_PAGER`), a runtime +/// configuration knob that affects how Python or the shell evaluate user +/// input (`PYTHONSTARTUP`, `BASH_ENV`, `ENV`, `PROMPT_COMMAND`), or a loader +/// override that lets an attacker inject a library into the next process +/// (`LD_PRELOAD`, `LD_LIBRARY_PATH`, `LD_AUDIT`, `DYLD_INSERT_LIBRARIES`, +/// `DYLD_LIBRARY_PATH`, `DYLD_FORCE_FLAT_NAMESPACE`). +/// +/// `PATH` and `SHELL` are listed so an inline override cannot redirect +/// resolution of any allowed binary to an attacker-controlled path. `IFS` +/// is listed because the shell uses it for word splitting and a malicious +/// value can hide command boundaries from later parsers. +const DANGEROUS_ENV_PREFIXES: &[&str] = &[ + "BASH_ENV", + "BAT_PAGER", + "BROWSER", + "DYLD_FORCE_FLAT_NAMESPACE", + "DYLD_INSERT_LIBRARIES", + "DYLD_LIBRARY_PATH", + "EDITOR", + "ENV", + "GIT_EDITOR", + "GIT_EXTERNAL_DIFF", + "GIT_EXTERNAL_FILTER", + "GIT_PAGER", + "GIT_SSH", + "GIT_SSH_COMMAND", + "IFS", + "LD_AUDIT", + "LD_LIBRARY_PATH", + "LD_PRELOAD", + "LESS", + "LESSCLOSE", + "LESSOPEN", + "MANOPT", + "MANPAGER", + "PAGER", + "PATH", + "PROMPT_COMMAND", + "PS1", + "PS2", + "PS3", + "PS4", + "PYTHONPATH", + "PYTHONSTARTUP", + "SHELL", + "VISUAL", +]; + +/// Returns true if `s` starts with one or more inline env assignments and any +/// of the assigned names are in [`DANGEROUS_ENV_PREFIXES`]. +/// +/// The allowlist validation in [`SecurityPolicy::is_command_allowed`] uses +/// [`skip_env_assignments`] to look past the env prefix before matching the +/// command name. That leaves a class of attacks where the bare command (e.g. +/// `git log`) is allowlisted but the env prefix mutates how it executes (e.g. +/// `GIT_PAGER= git log` — `git` spawns `` as its pager). Because +/// the prefix is stripped before allowlisting and the shell evaluates the +/// prefix at execution time, the bypass lands without ever touching a +/// blocked command name. +/// +/// Treating any dangerous prefix as a denial keeps the allowlist +/// semantically meaningful without having to enumerate every shape of every +/// downstream tool's hook surface. +fn has_dangerous_env_prefix(s: &str) -> bool { + let mut rest = s.trim_start(); + loop { + let Some(word) = rest.split_whitespace().next() else { + return false; + }; + if !word.contains('=') { + return false; + } + if !word + .chars() + .next() + .is_some_and(|c| c.is_ascii_alphabetic() || c == '_') + { + return false; + } + let (name, _) = word.split_once('=').unwrap_or((word, "")); + let upper = name.to_ascii_uppercase(); + if DANGEROUS_ENV_PREFIXES.iter().any(|d| *d == upper.as_str()) { + return true; + } + rest = rest[word.len()..].trim_start(); + } +} + /// Skip leading environment variable assignments (e.g. `FOO=bar cmd args`). /// Returns the remainder starting at the first non-assignment word. fn skip_env_assignments(s: &str) -> &str { @@ -1301,6 +1393,15 @@ impl SecurityPolicy { // Split on unquoted command separators and validate each sub-command. let segments = split_unquoted_segments(command); for segment in &segments { + // Reject segments that prefix the command with a dangerous env + // assignment (e.g. `GIT_PAGER= git log`). The bare command + // after the assignment is allowlisted, but the prefix mutates + // the downstream binary's execution to spawn `` as a + // subprocess. See [`has_dangerous_env_prefix`]. + if has_dangerous_env_prefix(segment) { + return false; + } + // Strip leading env var assignments (e.g. FOO=bar cmd) let cmd_part = skip_env_assignments(segment); @@ -1345,8 +1446,13 @@ impl SecurityPolicy { match base.as_str() { "find" => { - // find -exec and find -ok allow arbitrary command execution - !args.iter().any(|arg| arg == "-exec" || arg == "-ok") + // -exec / -ok run a command per match. -execdir / -okdir do + // the same with the working directory set to the match's + // parent — same code-execution semantics, just with a + // different cwd, so they must be blocked alongside. + !args.iter().any(|arg| { + arg == "-exec" || arg == "-ok" || arg == "-execdir" || arg == "-okdir" + }) } "git" => { // git config, alias, and -c can be used to set dangerous options diff --git a/src/openhuman/security/policy_tests.rs b/src/openhuman/security/policy_tests.rs index 9c9cff580..6648ab66d 100644 --- a/src/openhuman/security/policy_tests.rs +++ b/src/openhuman/security/policy_tests.rs @@ -1111,6 +1111,10 @@ fn command_argument_injection_blocked() { // find -exec is a common bypass assert!(!p.is_command_allowed("find . -exec rm -rf {} +")); assert!(!p.is_command_allowed("find / -ok cat {} \\;")); + // -execdir / -okdir have identical command-execution semantics — same cwd + // bypass class, different option spelling. + assert!(!p.is_command_allowed("find /tmp -maxdepth 1 -name poc_proof.txt -execdir whoami \\;")); + assert!(!p.is_command_allowed("find /etc -name passwd -okdir head -3 {} \\;")); // git config/alias can execute commands assert!(!p.is_command_allowed("git config core.editor \"rm -rf /\"")); assert!(!p.is_command_allowed("git alias.st status")); @@ -1121,6 +1125,61 @@ fn command_argument_injection_blocked() { assert!(p.is_command_allowed("git add .")); } +#[test] +fn dangerous_env_var_prefix_blocked() { + let p = default_policy(); + // GIT_PAGER / PAGER / GIT_SSH_COMMAND / GIT_EXTERNAL_DIFF / EDITOR all + // cause git or other allowed binaries to spawn the assigned value as a + // subprocess. The bare command (`git log`, `git status`, `git diff`) + // is allowlisted, but the env prefix shifts execution to an arbitrary + // command. + assert!(!p.is_command_allowed("GIT_PAGER=/tmp/payload.sh git log")); + assert!(!p.is_command_allowed("PAGER=calc.exe git log")); + assert!(!p.is_command_allowed("GIT_SSH_COMMAND=/tmp/x git fetch")); + assert!(!p.is_command_allowed("GIT_EXTERNAL_DIFF=/tmp/x git diff")); + assert!(!p.is_command_allowed("EDITOR=/tmp/x git commit")); + assert!(!p.is_command_allowed("VISUAL=/tmp/x git commit")); + assert!(!p.is_command_allowed("LESS=/tmp/x cat /etc/passwd")); + assert!(!p.is_command_allowed("LESSOPEN=/tmp/x cat /etc/passwd")); + assert!(!p.is_command_allowed("MANPAGER=/tmp/x man bash")); + assert!(!p.is_command_allowed("BAT_PAGER=/tmp/x bat file")); + assert!(!p.is_command_allowed("BROWSER=/tmp/x git status")); + // Loader-override variables let an attacker inject a library into the + // next process. + assert!(!p.is_command_allowed("LD_PRELOAD=/tmp/x.so git status")); + assert!(!p.is_command_allowed("LD_LIBRARY_PATH=/tmp git status")); + assert!(!p.is_command_allowed("LD_AUDIT=/tmp/x.so git status")); + assert!(!p.is_command_allowed("DYLD_INSERT_LIBRARIES=/tmp/x.dylib git status")); + assert!(!p.is_command_allowed("DYLD_LIBRARY_PATH=/tmp git status")); + assert!(!p.is_command_allowed("DYLD_FORCE_FLAT_NAMESPACE=1 git status")); + // Shell-evaluation variables. + assert!(!p.is_command_allowed("BASH_ENV=/tmp/x git status")); + assert!(!p.is_command_allowed("ENV=/tmp/x git status")); + assert!(!p.is_command_allowed("PROMPT_COMMAND=/tmp/x git status")); + assert!(!p.is_command_allowed("IFS=$'\\n' git status")); + // Python startup hook + path override. + assert!(!p.is_command_allowed("PYTHONSTARTUP=/tmp/x python3 -V")); + assert!(!p.is_command_allowed("PYTHONPATH=/tmp python3 -V")); + // PATH / SHELL overrides redirect resolution of the next binary. + assert!(!p.is_command_allowed("PATH=/tmp git status")); + assert!(!p.is_command_allowed("SHELL=/tmp/x git status")); + // Lower-case spellings still match (env names are case-insensitive + // by convention here — most shells uppercase them, but the matcher + // should not be fooled by case folding). + assert!(!p.is_command_allowed("git_pager=/tmp/x git log")); + // Case-insensitive: should also catch mixed-case names. + assert!(!p.is_command_allowed("Ld_PrElOaD=/tmp/x.so git status")); + + // Benign env prefixes still pass: TZ, LANG, LC_ALL, custom names that + // don't trigger downstream subprocess execution. + assert!(p.is_command_allowed("TZ=UTC git log")); + assert!(p.is_command_allowed("LANG=en_US.UTF-8 git log")); + assert!(p.is_command_allowed("LC_ALL=C git status")); + assert!(p.is_command_allowed("FOO=bar git status")); + // No env prefix at all — unchanged. + assert!(p.is_command_allowed("git status")); +} + #[test] fn custom_allowlist_cannot_enable_command_executors() { let p = SecurityPolicy {