From f2ebdb1fb2d932101b764a237c32dc87c3df25e7 Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Sun, 25 Oct 2026 19:07:44 +0200 Subject: [PATCH 1/4] fix: drop supplementary groups for sandboxed launches Unprivileged bubblewrap keeps the caller's supplementary groups, and groups such as docker and lxd are root-equivalent: a network-only sandbox could reach /var/run/docker.sock. When the owner has any, sandboxed launches start through unshare with maps from the setuid newuidmap/newgidmap, drop every supplementary group and exec bubblewrap as the same uid without capabilities. Missing uidmap or subordinate ranges refuse the launch with a fix-it message. runtime_isolation_smoke.py now fails when a sandboxed app keeps any supplementary group; it fails on 0.4.0 and passes here. --- scripts/runtime_isolation_smoke.py | 5 +- src/launch.rs | 104 +++++++++++++++++++++++++++++ src/workspace.rs | 10 ++- 3 files changed, 116 insertions(+), 3 deletions(-) diff --git a/scripts/runtime_isolation_smoke.py b/scripts/runtime_isolation_smoke.py index a9ef160..22661d6 100644 --- a/scripts/runtime_isolation_smoke.py +++ b/scripts/runtime_isolation_smoke.py @@ -56,8 +56,9 @@ def cmd(*args): finally: s.close() foreign_x=[p.name for p in Path('/tmp/.X11-unix').iterdir() if p.name!=own_x_socket] host_bus=any(p.exists() for p in [Path('/run/dbus/system_bus_socket'),Path('/run/user')/str(os.getuid())/'bus']) -print(json.dumps({'reachable':reachable,'pid':os.getpid(),'host_proc':Path('/proc',hostpid).exists(),'foreign_x':foreign_x,'host_bus':host_bus})) -raise SystemExit(42 if reachable or Path('/proc',hostpid).exists() or foreign_x or host_bus else 0) +groups=[g for g in os.getgroups() if g!=os.getgid()] +print(json.dumps({'reachable':reachable,'pid':os.getpid(),'host_proc':Path('/proc',hostpid).exists(),'foreign_x':foreign_x,'host_bus':host_bus,'groups':groups})) +raise SystemExit(42 if reachable or Path('/proc',hostpid).exists() or foreign_x or host_bus or groups else 0) """ terminal=mcp.tool("workspace_run_in_terminal",{"id":id,"terminal_id":"private","command":["sh","-c","printf PRIVATE_TERMINAL_OK; exec tail -f /dev/null"],"wait_window":True,"timeout_ms":10000}) assert terminal["ok"],terminal diff --git a/src/launch.rs b/src/launch.rs index f8fab19..50c7a06 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -586,6 +586,96 @@ pub(crate) fn resolve_command_path(program: &str) -> Option { .find(|candidate| is_executable_file(candidate)) } +/// Unprivileged bubblewrap keeps the caller's supplementary groups, and groups +/// such as docker or lxd are root-equivalent. When the owner has any, a +/// sandboxed launch starts through `unshare` with a user namespace whose maps +/// come from the setuid newuidmap/newgidmap (so setgroups stays allowed), +/// drops every supplementary group and execs bubblewrap as the same uid with +/// no capabilities. Inner root maps to one subordinate id nothing runs as. +pub(crate) fn sandbox_launcher_prefix() -> Result> { + let (uid, gid) = unsafe { (libc::geteuid(), libc::getegid()) }; + if supplementary_groups()?.iter().all(|group| *group == gid) { + return Ok(Vec::new()); + } + let missing = |what: &str| { + anyhow::anyhow!( + "sandboxed launch refused: your supplementary groups cannot be dropped ({what}). \ + Install uidmap and give your user a subordinate range, e.g. \ + `sudo usermod --add-subuids 100000-165535 --add-subgids 100000-165535 $USER`" + ) + }; + for tool in ["unshare", "newuidmap", "newgidmap"] { + resolve_executable_path(tool).ok_or_else(|| missing(&format!("{tool} not found")))?; + } + let name = user_name(uid); + let sub_uid = subordinate_start(Path::new("/etc/subuid"), uid, name.as_deref()) + .ok_or_else(|| missing("no /etc/subuid range"))?; + let sub_gid = subordinate_start(Path::new("/etc/subgid"), uid, name.as_deref()) + .ok_or_else(|| missing("no /etc/subgid range"))?; + Ok(vec![ + "unshare".to_string(), + "--user".to_string(), + format!("--map-users={uid}:{uid}:1"), + format!("--map-users=0:{sub_uid}:1"), + format!("--map-groups={gid}:{gid}:1"), + format!("--map-groups=0:{sub_gid}:1"), + "--setuid".to_string(), + uid.to_string(), + "--setgid".to_string(), + gid.to_string(), + "--".to_string(), + ]) +} + +fn supplementary_groups() -> Result> { + let count = unsafe { libc::getgroups(0, std::ptr::null_mut()) }; + if count < 0 { + return Err(std::io::Error::last_os_error()).context("read supplementary groups"); + } + let mut groups = vec![0 as libc::gid_t; count as usize]; + let count = unsafe { libc::getgroups(count, groups.as_mut_ptr()) }; + if count < 0 { + return Err(std::io::Error::last_os_error()).context("read supplementary groups"); + } + groups.truncate(count as usize); + Ok(groups) +} + +fn user_name(uid: libc::uid_t) -> Option { + let mut buffer = vec![0 as libc::c_char; 16 * 1024]; + let mut entry: libc::passwd = unsafe { std::mem::zeroed() }; + let mut result = std::ptr::null_mut(); + let status = unsafe { + libc::getpwuid_r( + uid, + &mut entry, + buffer.as_mut_ptr(), + buffer.len(), + &mut result, + ) + }; + if status != 0 || result.is_null() { + return None; + } + unsafe { std::ffi::CStr::from_ptr(entry.pw_name) } + .to_str() + .ok() + .map(str::to_string) +} + +/// The first id of the user's range in an /etc/subuid-style file, matched by +/// name or numeric uid. +fn subordinate_start(path: &Path, uid: libc::uid_t, name: Option<&str>) -> Option { + let uid = uid.to_string(); + fs::read_to_string(path).ok()?.lines().find_map(|line| { + let mut fields = line.trim().split(':'); + let owner = fields.next()?; + let start = fields.next()?.parse::().ok()?; + let count = fields.next()?.parse::().ok()?; + (count >= 1 && (owner == uid || Some(owner) == name)).then_some(start) + }) +} + pub(crate) fn resolve_executable_path(program: &str) -> Option { if program.contains('/') { let path = PathBuf::from(program); @@ -603,6 +693,20 @@ fn is_executable_file(path: &Path) -> bool { #[cfg(test)] mod tests { + #[test] + fn subordinate_ranges_match_by_name_or_uid() { + let path = test_runtime("subuid"); + fs::write( + &path, + "other:200000:65536\nalice:100000:0\nalice:300000:65536\n1000:400000:10\n", + ) + .expect("write subuid"); + assert_eq!(subordinate_start(&path, 1001, Some("alice")), Some(300000)); + assert_eq!(subordinate_start(&path, 1000, None), Some(400000)); + assert_eq!(subordinate_start(&path, 1002, Some("bob")), None); + fs::remove_file(path).expect("remove subuid"); + } + use super::*; fn native_status( diff --git a/src/workspace.rs b/src/workspace.rs index 6efc040..09ffc9c 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -7109,7 +7109,15 @@ fn spawn_app(state: &mut DaemonState, spec: LaunchSpec) -> Result let (mount_isolation, network_isolation) = launch_isolation_labels_from_sandbox(sandbox.as_ref()); let mut child_command = if let Some(sandbox) = &sandbox { - let mut command = Command::new("bwrap"); + let prefix = crate::launch::sandbox_launcher_prefix()?; + let mut command = match prefix.split_first() { + Some((program, args)) => { + let mut command = Command::new(program); + command.args(args).arg("bwrap"); + command + } + None => Command::new("bwrap"), + }; command .args(&sandbox.args) .arg("--") From 7b57e4ac017e1e3ac542fd8cdcac61fe2a971233 Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Sun, 25 Oct 2026 20:04:53 +0200 Subject: [PATCH 2/4] fix: keep launch variables away from the sandbox launcher The launch environment (LD_PRELOAD, PATH, ...) was applied to bwrap itself, so a caller under a permission ceiling could run code outside the sandbox. The launcher and bwrap now start from absolute paths with an empty environment; the app environment and sandbox options reach bwrap through --args on a private memfd, so they also stay out of the world-readable command line. Apps receive the same variables as before. Group dropping is best effort: hosts without uidmap, a subordinate range, util-linux 2.40, unprivileged user namespaces or with no_new_privs keep the previous behavior; doctor reports why under sandbox_group_drop. Sandboxed tmux clients drop groups too. --- README.md | 3 +- src/launch.rs | 206 +++++++++++++++++++++++++++++++++++++++++++++-- src/workspace.rs | 102 +++++++++++++++++------ 3 files changed, 280 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index fbd4286..50cf096 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ Requires Linux. Install the runtime dependencies, then build and install in one ```bash sudo apt install xvfb openbox xdotool xauth x11-utils imagemagick xclip \ - bubblewrap pkg-config libxkbcommon-x11-dev + bubblewrap uidmap pkg-config libxkbcommon-x11-dev ./install.sh ``` @@ -325,6 +325,7 @@ The MCP exposes ~86 tools. To avoid dumping them all into the agent's context, i - **Pre-1.0.** Interfaces and tool schemas can change between versions. - **Single-user trust model.** The control socket is a same-uid Unix socket (mode 0600); there is no cross-user protection by design. Run as a dedicated user for multi-user isolation. - **Mount/network enforcement needs bubblewrap.** Without it, those policies are declared but not enforced (the runtime tells you which). +- **Sandboxed launches drop your supplementary groups where the host allows it.** Groups such as `docker` or `lxd` are root-equivalent. Dropping them needs `uidmap` (`newuidmap`/`newgidmap`), a subordinate id range for your user in `/etc/subuid` and `/etc/subgid` (Ubuntu's `adduser` creates one), util-linux 2.40 or newer, user namespaces that `unshare` may use, and a host without `no_new_privs`. Where any of this is missing, sandboxed apps keep your groups as before; `doctor` reports why under `sandbox_group_drop`. Ubuntu 23.10 and newer restrict unprivileged user namespaces with AppArmor (`kernel.apparmor_restrict_unprivileged_userns=1`). To allow it, either give `/usr/bin/unshare` an AppArmor profile with `userns,` (like Ubuntu's own `lxc-usernsexec` profile; this lifts the restriction for every program that runs `unshare`), or set the sysctl to 0. Alternatively, remove the root-equivalent groups from your user. - **Live viewer control is not the hard permission boundary** — the permission ceiling is authoritative, while unreadable live-control state fails closed for mutating daemon IPC. ## Docs diff --git a/src/launch.rs b/src/launch.rs index 50c7a06..35def53 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -592,28 +592,55 @@ pub(crate) fn resolve_command_path(program: &str) -> Option { /// come from the setuid newuidmap/newgidmap (so setgroups stays allowed), /// drops every supplementary group and execs bubblewrap as the same uid with /// no capabilities. Inner root maps to one subordinate id nothing runs as. -pub(crate) fn sandbox_launcher_prefix() -> Result> { +/// Best effort: when the groups cannot be dropped on this host, launches +/// behave as before and keep them; `doctor` reports why. +pub(crate) fn sandbox_launcher_prefix() -> Vec { + group_drop_prefix().unwrap_or_default() +} + +/// Whether sandboxed launches drop the owner's supplementary groups here, and +/// why not when they cannot. +pub(crate) fn group_drop_status() -> std::result::Result { + match group_drop_prefix() { + Ok(prefix) => Ok(!prefix.is_empty()), + Err(error) => Err(format!("{error:#}")), + } +} + +fn group_drop_prefix() -> Result> { let (uid, gid) = unsafe { (libc::geteuid(), libc::getegid()) }; if supplementary_groups()?.iter().all(|group| *group == gid) { return Ok(Vec::new()); } let missing = |what: &str| { anyhow::anyhow!( - "sandboxed launch refused: your supplementary groups cannot be dropped ({what}). \ - Install uidmap and give your user a subordinate range, e.g. \ + "supplementary groups are kept in sandboxed launches ({what}); \ + install uidmap and give your user a subordinate range, e.g. \ `sudo usermod --add-subuids 100000-165535 --add-subgids 100000-165535 $USER`" ) }; - for tool in ["unshare", "newuidmap", "newgidmap"] { + for tool in ["newuidmap", "newgidmap"] { resolve_executable_path(tool).ok_or_else(|| missing(&format!("{tool} not found")))?; } + let unshare = resolve_executable_path("unshare").ok_or_else(|| missing("unshare not found"))?; + // Repeated --map-users/--map-groups ranges arrived in util-linux 2.40. + let version = Command::new(&unshare) + .arg("--version") + .env_clear() + .output() + .ok() + .and_then(|output| String::from_utf8(output.stdout).ok()) + .and_then(|text| util_linux_version(&text)); + if version.is_none_or(|version| version < (2, 40)) { + return Err(missing("util-linux 2.40 or newer is required for unshare")); + } let name = user_name(uid); let sub_uid = subordinate_start(Path::new("/etc/subuid"), uid, name.as_deref()) .ok_or_else(|| missing("no /etc/subuid range"))?; let sub_gid = subordinate_start(Path::new("/etc/subgid"), uid, name.as_deref()) .ok_or_else(|| missing("no /etc/subgid range"))?; - Ok(vec![ - "unshare".to_string(), + let prefix = vec![ + unshare.display().to_string(), "--user".to_string(), format!("--map-users={uid}:{uid}:1"), format!("--map-users=0:{sub_uid}:1"), @@ -624,7 +651,138 @@ pub(crate) fn sandbox_launcher_prefix() -> Result> { "--setgid".to_string(), gid.to_string(), "--".to_string(), - ]) + ]; + group_drop_preflight(&prefix)?; + Ok(prefix) +} + +fn util_linux_version(text: &str) -> Option<(u32, u32)> { + let version = text.split_whitespace().last()?; + let mut parts = version.split('.'); + Some((parts.next()?.parse().ok()?, parts.next()?.parse().ok()?)) +} + +/// Runs the wrapper once per process with `true`, so a host that refuses it +/// (user namespaces restricted by AppArmor, newuidmap denied) fails the +/// launch with the reason instead of every app exiting right after spawn. +fn group_drop_preflight(prefix: &[String]) -> Result<()> { + static RESULT: std::sync::OnceLock> = + std::sync::OnceLock::new(); + RESULT + .get_or_init(|| { + let truth = + resolve_executable_path("true").ok_or_else(|| "true not found".to_string())?; + let output = Command::new(&prefix[0]) + .args(&prefix[1..]) + .arg(truth) + .env_clear() + .output() + .map_err(|error| format!("could not run unshare: {error}"))?; + if output.status.success() { + return Ok(()); + } + let restricted = + fs::read_to_string("/proc/sys/kernel/apparmor_restrict_unprivileged_userns") + .is_ok_and(|value| value.trim() == "1"); + Err(format!( + "supplementary groups are kept in sandboxed launches: dropping them failed ({}){}", + String::from_utf8_lossy(&output.stderr).trim(), + if restricted { + ". AppArmor restricts unprivileged user namespaces here \ + (kernel.apparmor_restrict_unprivileged_userns=1); see the README section \ + on supplementary groups" + } else { + "" + } + )) + }) + .clone() + .map_err(|error| anyhow::anyhow!(error)) +} + +/// A sandboxed launch command and the private descriptor carrying its +/// arguments. Keep `args_fd` alive until the command is spawned. +pub(crate) struct SandboxedCommand { + pub(crate) command: Command, + pub(crate) args_fd: std::os::fd::OwnedFd, +} + +/// Builds a sandboxed launch. The launcher and bubblewrap run from absolute +/// paths with an empty environment; the app's environment and argv reach +/// bubblewrap through `--args` on a close-on-exec memfd that only this child +/// inherits. Launch variables such as LD_PRELOAD or PATH therefore never +/// apply to the launcher, and stay out of the world-readable command line. +pub(crate) fn sandboxed_command( + sandbox_args: &[String], + program: &str, + program_args: &[String], + env: &[(std::ffi::OsString, std::ffi::OsString)], +) -> Result { + use std::os::fd::AsRawFd; + use std::os::unix::process::CommandExt; + let bwrap = resolve_executable_path("bwrap").context("bwrap not found")?; + let args_fd = bwrap_args_memfd(sandbox_args, env)?; + let prefix = sandbox_launcher_prefix(); + let mut command = match prefix.split_first() { + Some((launcher, args)) => { + let mut command = Command::new(launcher); + command.args(args).arg(&bwrap); + command + } + None => Command::new(&bwrap), + }; + let fd = args_fd.as_raw_fd(); + // bubblewrap reads only options from --args; the program stays on the + // command line after `--`. + command + .arg("--args") + .arg(fd.to_string()) + .arg("--") + .arg(program) + .args(program_args) + .env_clear(); + unsafe { + command.pre_exec(move || { + if libc::fcntl(fd, libc::F_SETFD, 0) != 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + Ok(SandboxedCommand { command, args_fd }) +} + +/// `--clearenv`, the environment and the sandbox options, as NUL-separated +/// bubblewrap arguments in a close-on-exec memfd. +fn bwrap_args_memfd( + sandbox_args: &[String], + env: &[(std::ffi::OsString, std::ffi::OsString)], +) -> Result { + use std::os::fd::FromRawFd; + use std::os::unix::ffi::OsStrExt; + let raw = unsafe { libc::memfd_create(c"bwrap-args".as_ptr(), libc::MFD_CLOEXEC) }; + if raw < 0 { + return Err(std::io::Error::last_os_error()).context("create bwrap argument memfd"); + } + let args_fd = unsafe { std::os::fd::OwnedFd::from_raw_fd(raw) }; + let mut bytes = Vec::new(); + let mut push = |value: &[u8]| { + bytes.extend_from_slice(value); + bytes.push(0); + }; + push(b"--clearenv"); + for (name, value) in env { + push(b"--setenv"); + push(name.as_bytes()); + push(value.as_bytes()); + } + for arg in sandbox_args { + push(arg.as_bytes()); + } + let mut file = std::fs::File::from(args_fd.try_clone()?); + std::io::Write::write_all(&mut file, &bytes).context("write bwrap arguments")?; + std::io::Seek::rewind(&mut file).context("rewind bwrap arguments")?; + Ok(args_fd) } fn supplementary_groups() -> Result> { @@ -693,6 +851,40 @@ fn is_executable_file(path: &Path) -> bool { #[cfg(test)] mod tests { + #[test] + fn launch_environment_reaches_bubblewrap_only_through_private_arguments() { + let env = [ + ("LD_PRELOAD".into(), "/tmp/inject.so".into()), + ("PATH".into(), "/nonexistent".into()), + ]; + let fd = bwrap_args_memfd( + &["--ro-bind".to_string(), "/".to_string(), "/".to_string()], + &env, + ) + .expect("memfd"); + let mut bytes = Vec::new(); + std::io::Read::read_to_end(&mut std::fs::File::from(fd), &mut bytes).expect("read"); + let args = bytes + .split(|byte| *byte == 0) + .map(|arg| String::from_utf8_lossy(arg).into_owned()) + .collect::>(); + assert_eq!( + &args[..10], + [ + "--clearenv", + "--setenv", + "LD_PRELOAD", + "/tmp/inject.so", + "--setenv", + "PATH", + "/nonexistent", + "--ro-bind", + "/", + "/", + ] + ); + } + #[test] fn subordinate_ranges_match_by_name_or_uid() { let path = test_runtime("subuid"); diff --git a/src/workspace.rs b/src/workspace.rs index 09ffc9c..9632e49 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -156,6 +156,9 @@ pub struct RuntimeReport { pub screenshot: Check, pub clipboard: Check, pub policy: PolicyRuntimeCapabilities, + /// Whether sandboxed launches can drop the owner's supplementary groups; + /// when they cannot, sandboxed launches are refused. + pub sandbox_group_drop: Check, } #[derive(Debug, Clone, Serialize, JsonSchema)] @@ -1151,6 +1154,21 @@ pub fn doctor_report() -> DoctorReport { screenshot: first_available_command(&["ffmpeg", "import", "scrot"]), clipboard: first_available_command(&["xclip", "xsel"]), policy: policy_runtime_capabilities(), + sandbox_group_drop: match crate::launch::group_drop_status() { + Ok(false) => Check { + ok: true, + detail: "no supplementary groups to drop".to_string(), + }, + Ok(true) => Check { + ok: true, + detail: "sandboxed launches drop supplementary groups via newuidmap/newgidmap" + .to_string(), + }, + Err(reason) => Check { + ok: false, + detail: reason, + }, + }, }; let viewer = ViewerRuntimeReport { host_display: host_display_check(), @@ -3855,6 +3873,30 @@ fn write_applied_policy_file( Ok(policy_path) } +/// The environment a sandboxed app gets: the daemon's own environment with +/// the launch rules applied (scrubbed host session variables, workspace +/// variables, the caller's launch variables, the client bus). Bubblewrap +/// receives it through `--setenv` instead of inheriting it. +fn sandboxed_app_environment( + status: &WorkspaceStatus, + launch_env: &[EnvVar], +) -> Vec<(std::ffi::OsString, std::ffi::OsString)> { + let mut rules = Command::new("true"); + crate::launch::configure_launch_environment(&mut rules, status, launch_env, true); + let mut env = env::vars_os().collect::>(); + for (name, value) in rules.get_envs() { + match value { + Some(value) => { + env.insert(name.to_os_string(), value.to_os_string()); + } + None => { + env.remove(name); + } + } + } + env.into_iter().collect() +} + fn create_client_runtime(runtime: &Path) -> Result<()> { create_owned_private_dir(&runtime.join("client"), "workspace client runtime") } @@ -7108,22 +7150,19 @@ fn spawn_app(state: &mut DaemonState, spec: LaunchSpec) -> Result )?; let (mount_isolation, network_isolation) = launch_isolation_labels_from_sandbox(sandbox.as_ref()); + // Held until spawn: the descriptor that carries a sandboxed launch's + // arguments and environment to bubblewrap. + let mut sandbox_args_fd = None; let mut child_command = if let Some(sandbox) = &sandbox { - let prefix = crate::launch::sandbox_launcher_prefix()?; - let mut command = match prefix.split_first() { - Some((program, args)) => { - let mut command = Command::new(program); - command.args(args).arg("bwrap"); - command - } - None => Command::new("bwrap"), - }; - command - .args(&sandbox.args) - .arg("--") - .arg(&spec.command[0]) - .args(&spec.command[1..]); - command + let env = sandboxed_app_environment(&state.status, &spec.env); + let built = crate::launch::sandboxed_command( + &sandbox.args, + &spec.command[0], + &spec.command[1..], + &env, + )?; + sandbox_args_fd = Some(built.args_fd); + built.command } else { let mut command = Command::new(&spec.command[0]); command.args(&spec.command[1..]); @@ -7144,16 +7183,19 @@ fn spawn_app(state: &mut DaemonState, spec: LaunchSpec) -> Result child_command.current_dir(cwd); } } - crate::launch::configure_launch_environment( - &mut child_command, - &state.status, - &spec.env, - sandbox.is_some(), - ); + if sandbox.is_none() { + crate::launch::configure_launch_environment( + &mut child_command, + &state.status, + &spec.env, + false, + ); + } child_command.process_group(0); let child = child_command .spawn() .with_context(|| format!("failed to launch {}", launch_description(&spec.command)))?; + drop(sandbox_args_fd); let pid = child.id(); let stdout_path = rename_app_log(&log_paths.stdout, pid, "stdout")?; let stderr_path = rename_app_log(&log_paths.stderr, pid, "stderr")?; @@ -7422,7 +7464,16 @@ fn tmux_command(socket_path: &Path, sandbox: &TmuxClientSandbox, args: &[&str]) TmuxClientSandbox::Unknown => bail!("terminal client sandbox was not resolved"), TmuxClientSandbox::Host => Command::new(tmux), TmuxClientSandbox::Bubblewrap(sandbox_args) => { - let mut command = Command::new("bwrap"); + let bwrap = resolve_executable_path("bwrap").context("bwrap not found")?; + let prefix = crate::launch::sandbox_launcher_prefix(); + let mut command = match prefix.split_first() { + Some((launcher, launcher_args)) => { + let mut command = Command::new(launcher); + command.args(launcher_args).arg(bwrap); + command + } + None => Command::new(bwrap), + }; command .args(sandbox_args) .arg("--die-with-parent") @@ -10047,7 +10098,12 @@ mod tests { })); let command = tmux_command(&socket, &TmuxClientSandbox::Bubblewrap(args), &["ls"]) .expect("client command"); - assert_eq!(command.get_program(), "bwrap"); + // bwrap, or unshare in front of it where supplementary groups are dropped. + let program = Path::new(command.get_program()); + assert!( + program.ends_with("bwrap") || program.ends_with("unshare"), + "{program:?}" + ); assert!(command.get_args().any(|arg| arg == "--die-with-parent")); assert!(command .get_envs() From 37416cb6a73b450ba3b83d09751193d10c432d7b Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Sun, 25 Oct 2026 20:14:08 +0200 Subject: [PATCH 3/4] fix: close review findings in the sandbox launcher Reject NUL bytes in launch variables, sandbox arguments and profile mount paths, drop --clearenv so bubblewrap older than 0.5 keeps working, run the group-drop preflight through bubblewrap and cache it per process, and keep the argument memfd off the standard streams. The isolation smoke expects dropped groups only on hosts that report the capability. --- README.md | 2 +- scripts/runtime_isolation_smoke.py | 9 +- src/launch.rs | 136 +++++++++++++++++------------ src/profile.rs | 3 + src/workspace.rs | 4 +- 5 files changed, 92 insertions(+), 62 deletions(-) diff --git a/README.md b/README.md index 50cf096..df8ece7 100644 --- a/README.md +++ b/README.md @@ -325,7 +325,7 @@ The MCP exposes ~86 tools. To avoid dumping them all into the agent's context, i - **Pre-1.0.** Interfaces and tool schemas can change between versions. - **Single-user trust model.** The control socket is a same-uid Unix socket (mode 0600); there is no cross-user protection by design. Run as a dedicated user for multi-user isolation. - **Mount/network enforcement needs bubblewrap.** Without it, those policies are declared but not enforced (the runtime tells you which). -- **Sandboxed launches drop your supplementary groups where the host allows it.** Groups such as `docker` or `lxd` are root-equivalent. Dropping them needs `uidmap` (`newuidmap`/`newgidmap`), a subordinate id range for your user in `/etc/subuid` and `/etc/subgid` (Ubuntu's `adduser` creates one), util-linux 2.40 or newer, user namespaces that `unshare` may use, and a host without `no_new_privs`. Where any of this is missing, sandboxed apps keep your groups as before; `doctor` reports why under `sandbox_group_drop`. Ubuntu 23.10 and newer restrict unprivileged user namespaces with AppArmor (`kernel.apparmor_restrict_unprivileged_userns=1`). To allow it, either give `/usr/bin/unshare` an AppArmor profile with `userns,` (like Ubuntu's own `lxc-usernsexec` profile; this lifts the restriction for every program that runs `unshare`), or set the sysctl to 0. Alternatively, remove the root-equivalent groups from your user. +- **Sandboxed launches drop your supplementary groups where the host allows it.** Groups such as `docker` or `lxd` are root-equivalent. Dropping them needs `uidmap` (`newuidmap`/`newgidmap`), a subordinate id range for your user in `/etc/subuid` and `/etc/subgid` (Ubuntu's `adduser` creates one), util-linux 2.40 or newer, user namespaces that `unshare` may use, and a host without `no_new_privs`. Where any of this is missing, sandboxed apps keep your groups as before; `doctor` reports why under `sandbox_group_drop`. When dropping applies, every supplementary group goes, including groups such as `video`, `audio` or a shared project group, so files a sandboxed app could reach only through such a group become unreadable to it. Ubuntu 23.10 and newer restrict unprivileged user namespaces with AppArmor (`kernel.apparmor_restrict_unprivileged_userns=1`). To allow it, either give `/usr/bin/unshare` an AppArmor profile with `userns,` (like Ubuntu's own `lxc-usernsexec` profile; this lifts the restriction for every program that runs `unshare`), or set the sysctl to 0. Alternatively, remove the root-equivalent groups from your user. - **Live viewer control is not the hard permission boundary** — the permission ceiling is authoritative, while unreadable live-control state fails closed for mutating daemon IPC. ## Docs diff --git a/scripts/runtime_isolation_smoke.py b/scripts/runtime_isolation_smoke.py index 22661d6..32b1c84 100644 --- a/scripts/runtime_isolation_smoke.py +++ b/scripts/runtime_isolation_smoke.py @@ -28,6 +28,9 @@ def cmd(*args): assert v.get("ok",True),v return v +# Hosts that cannot drop supplementary groups keep them, as before. +doctor=json.loads(subprocess.run([BIN,"doctor"],env=ENV,capture_output=True,text=True,timeout=60).stdout) +drops_groups="1" if "drop supplementary groups via" in doctor["runtime"]["sandbox_group_drop"]["detail"] else "0" mcp=Mcp(BIN,ROOT/"mcp.log",env=ENV,headless=True) try: for mounted in (False,True): @@ -45,7 +48,7 @@ def cmd(*args): probe="""import os,sys,json,socket from pathlib import Path runtime=Path(sys.argv[1]); hostpid=sys.argv[2]; own_x_socket='X'+sys.argv[3].split(':',1)[1].split('.')[0] -paths=[runtime/'control.sock',runtime/'session-bus',runtime/'i3.sock',Path('/proc')/hostpid/'root'/str(runtime).lstrip('/')/'control.sock',*map(Path,sys.argv[4:])] +paths=[runtime/'control.sock',runtime/'session-bus',runtime/'i3.sock',Path('/proc')/hostpid/'root'/str(runtime).lstrip('/')/'control.sock',*map(Path,sys.argv[5:])] ibus=Path.home()/'.cache'/'ibus' paths+=[p for p in ibus.glob('dbus-*')] if ibus.is_dir() else [] reachable=[] @@ -56,7 +59,7 @@ def cmd(*args): finally: s.close() foreign_x=[p.name for p in Path('/tmp/.X11-unix').iterdir() if p.name!=own_x_socket] host_bus=any(p.exists() for p in [Path('/run/dbus/system_bus_socket'),Path('/run/user')/str(os.getuid())/'bus']) -groups=[g for g in os.getgroups() if g!=os.getgid()] +groups=[g for g in os.getgroups() if g!=os.getgid()] if sys.argv[4]=='1' else [] print(json.dumps({'reachable':reachable,'pid':os.getpid(),'host_proc':Path('/proc',hostpid).exists(),'foreign_x':foreign_x,'host_bus':host_bus,'groups':groups})) raise SystemExit(42 if reachable or Path('/proc',hostpid).exists() or foreign_x or host_bus or groups else 0) """ @@ -64,7 +67,7 @@ def cmd(*args): assert terminal["ok"],terminal # Another sandboxed app must not reach the terminal's tmux server, a # sibling workspace's control socket, or the host input-method bus. - run=cmd("workspace","run","--id",id,"--timeout-ms",10000,"--","python3","-c",probe,status["runtime_dir"],status["daemon_pid"],status["display"], + run=cmd("workspace","run","--id",id,"--timeout-ms",10000,"--","python3","-c",probe,status["runtime_dir"],status["daemon_pid"],status["display"],drops_groups, terminal["terminal"]["tmux_socket_path"],Path(sibling["runtime_dir"])/"control.sock") assert run["exit_code"]==0,run screen=mcp.tool("workspace_terminal_read",{"id":id,"terminal_id":"private"}) diff --git a/src/launch.rs b/src/launch.rs index 35def53..5c2dc97 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -595,16 +595,23 @@ pub(crate) fn resolve_command_path(program: &str) -> Option { /// Best effort: when the groups cannot be dropped on this host, launches /// behave as before and keep them; `doctor` reports why. pub(crate) fn sandbox_launcher_prefix() -> Vec { - group_drop_prefix().unwrap_or_default() + cached_group_drop().clone().unwrap_or_default() } /// Whether sandboxed launches drop the owner's supplementary groups here, and -/// why not when they cannot. +/// why not when they cannot. Computed once per process: a daemon started +/// before the host was fixed keeps its result until it restarts. pub(crate) fn group_drop_status() -> std::result::Result { - match group_drop_prefix() { - Ok(prefix) => Ok(!prefix.is_empty()), - Err(error) => Err(format!("{error:#}")), - } + cached_group_drop() + .as_ref() + .map(|prefix| !prefix.is_empty()) + .map_err(Clone::clone) +} + +fn cached_group_drop() -> &'static std::result::Result, String> { + static PREFIX: std::sync::OnceLock, String>> = + std::sync::OnceLock::new(); + PREFIX.get_or_init(|| group_drop_prefix().map_err(|error| format!("{error:#}"))) } fn group_drop_prefix() -> Result> { @@ -662,42 +669,48 @@ fn util_linux_version(text: &str) -> Option<(u32, u32)> { Some((parts.next()?.parse().ok()?, parts.next()?.parse().ok()?)) } -/// Runs the wrapper once per process with `true`, so a host that refuses it -/// (user namespaces restricted by AppArmor, newuidmap denied) fails the -/// launch with the reason instead of every app exiting right after spawn. +/// Runs the real chain once (the wrapper, then bubblewrap inside it), so a +/// host that refuses either (user namespaces restricted by AppArmor, +/// newuidmap denied, no_new_privs) keeps the previous behavior instead of +/// every app exiting right after spawn. fn group_drop_preflight(prefix: &[String]) -> Result<()> { - static RESULT: std::sync::OnceLock> = - std::sync::OnceLock::new(); - RESULT - .get_or_init(|| { - let truth = - resolve_executable_path("true").ok_or_else(|| "true not found".to_string())?; - let output = Command::new(&prefix[0]) - .args(&prefix[1..]) - .arg(truth) - .env_clear() - .output() - .map_err(|error| format!("could not run unshare: {error}"))?; - if output.status.success() { - return Ok(()); - } - let restricted = - fs::read_to_string("/proc/sys/kernel/apparmor_restrict_unprivileged_userns") - .is_ok_and(|value| value.trim() == "1"); - Err(format!( - "supplementary groups are kept in sandboxed launches: dropping them failed ({}){}", - String::from_utf8_lossy(&output.stderr).trim(), - if restricted { - ". AppArmor restricts unprivileged user namespaces here \ - (kernel.apparmor_restrict_unprivileged_userns=1); see the README section \ - on supplementary groups" - } else { - "" - } - )) - }) - .clone() - .map_err(|error| anyhow::anyhow!(error)) + let bwrap = resolve_executable_path("bwrap").context("bwrap not found")?; + let truth = resolve_executable_path("true").context("true not found")?; + let output = Command::new(&prefix[0]) + .args(&prefix[1..]) + .arg(bwrap) + .args([ + "--ro-bind", + "/", + "/", + "--dev", + "/dev", + "--unshare-pid", + "--proc", + "/proc", + "--unshare-net", + "--", + ]) + .arg(truth) + .env_clear() + .output() + .context("could not run unshare")?; + if output.status.success() { + return Ok(()); + } + let restricted = fs::read_to_string("/proc/sys/kernel/apparmor_restrict_unprivileged_userns") + .is_ok_and(|value| value.trim() == "1"); + bail!( + "supplementary groups are kept in sandboxed launches: dropping them failed ({}){}", + String::from_utf8_lossy(&output.stderr).trim(), + if restricted { + ". AppArmor restricts unprivileged user namespaces here \ + (kernel.apparmor_restrict_unprivileged_userns=1); see the README section \ + on supplementary groups" + } else { + "" + } + ) } /// A sandboxed launch command and the private descriptor carrying its @@ -752,32 +765,42 @@ pub(crate) fn sandboxed_command( Ok(SandboxedCommand { command, args_fd }) } -/// `--clearenv`, the environment and the sandbox options, as NUL-separated -/// bubblewrap arguments in a close-on-exec memfd. +/// The environment (`--setenv`) and the sandbox options, as NUL-separated +/// bubblewrap arguments in a close-on-exec memfd. bubblewrap itself starts +/// with an empty environment, so it passes only these variables on. fn bwrap_args_memfd( sandbox_args: &[String], env: &[(std::ffi::OsString, std::ffi::OsString)], ) -> Result { use std::os::fd::FromRawFd; use std::os::unix::ffi::OsStrExt; - let raw = unsafe { libc::memfd_create(c"bwrap-args".as_ptr(), libc::MFD_CLOEXEC) }; - if raw < 0 { - return Err(std::io::Error::last_os_error()).context("create bwrap argument memfd"); - } - let args_fd = unsafe { std::os::fd::OwnedFd::from_raw_fd(raw) }; + // Arguments are NUL-separated: a NUL inside any value would split it + // into extra bubblewrap options. let mut bytes = Vec::new(); - let mut push = |value: &[u8]| { + let mut push = |value: &[u8]| -> Result<()> { + if value.contains(&0) { + bail!("sandbox argument or environment value contains a NUL byte"); + } bytes.extend_from_slice(value); bytes.push(0); + Ok(()) }; - push(b"--clearenv"); for (name, value) in env { - push(b"--setenv"); - push(name.as_bytes()); - push(value.as_bytes()); + push(b"--setenv")?; + push(name.as_bytes())?; + push(value.as_bytes())?; } for arg in sandbox_args { - push(arg.as_bytes()); + push(arg.as_bytes())?; + } + let raw = unsafe { libc::memfd_create(c"bwrap-args".as_ptr(), libc::MFD_CLOEXEC) }; + if raw < 0 { + return Err(std::io::Error::last_os_error()).context("create bwrap argument memfd"); + } + let args_fd = unsafe { std::os::fd::OwnedFd::from_raw_fd(raw) }; + // A memfd on a standard stream would be replaced by the child's stdio. + if raw <= 2 { + bail!("bwrap argument memfd landed on standard stream {raw}"); } let mut file = std::fs::File::from(args_fd.try_clone()?); std::io::Write::write_all(&mut file, &bytes).context("write bwrap arguments")?; @@ -869,9 +892,8 @@ mod tests { .map(|arg| String::from_utf8_lossy(arg).into_owned()) .collect::>(); assert_eq!( - &args[..10], + &args[..9], [ - "--clearenv", "--setenv", "LD_PRELOAD", "/tmp/inject.so", @@ -883,6 +905,8 @@ mod tests { "/", ] ); + assert!(bwrap_args_memfd(&["--chdir".into(), "/w\0--bind\0/\0/x".into()], &[]).is_err()); + assert!(bwrap_args_memfd(&[], &[("A".into(), "b\0c".into())]).is_err()); } #[test] diff --git a/src/profile.rs b/src/profile.rs index 7db37c1..5434b31 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -872,6 +872,9 @@ fn validate_mount_path(path: &Path, field: &str) -> Result<()> { if path.as_os_str().is_empty() { bail!("profile mount {field} cannot be empty"); } + if std::os::unix::ffi::OsStrExt::as_bytes(path.as_os_str()).contains(&0) { + bail!("profile mount {field} cannot contain a NUL byte"); + } if path .components() .any(|component| matches!(component, Component::ParentDir)) diff --git a/src/workspace.rs b/src/workspace.rs index 9632e49..0ebdc88 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -156,8 +156,8 @@ pub struct RuntimeReport { pub screenshot: Check, pub clipboard: Check, pub policy: PolicyRuntimeCapabilities, - /// Whether sandboxed launches can drop the owner's supplementary groups; - /// when they cannot, sandboxed launches are refused. + /// Whether sandboxed launches drop the owner's supplementary groups; when + /// they cannot, sandboxed apps keep them as before. pub sandbox_group_drop: Check, } From 59ebdab1c13cc888d5e89f8a50c795f84feaac31 Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Sun, 25 Oct 2026 20:14:27 +0200 Subject: [PATCH 4/4] docs: list uidmap in the contributor setup --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6d892c9..d621129 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -12,7 +12,7 @@ You need: ```bash sudo apt install xvfb openbox xdotool xauth x11-utils imagemagick xclip \ - bubblewrap pkg-config libxkbcommon-x11-dev + bubblewrap uidmap pkg-config libxkbcommon-x11-dev ``` ## Build