mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
feat: tighten runtime policy + transport guards v2 (#2636)
This commit is contained in:
@@ -524,17 +524,19 @@ async fn run_session_cycle<R: Runtime>(
|
||||
// - 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<R: Runtime>(
|
||||
// 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<R: Runtime>(
|
||||
// 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
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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=<cmd> git log` — `git` spawns `<cmd>` 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=<cmd> git log`). The bare command
|
||||
// after the assignment is allowlisted, but the prefix mutates
|
||||
// the downstream binary's execution to spawn `<cmd>` 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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user