From f85c737b462d8161e2a1ad9dca5cd37a16e9d297 Mon Sep 17 00:00:00 2001 From: kewton Date: Wed, 14 Oct 2026 01:14:22 +0900 Subject: [PATCH 1/7] fix(tools): detach sandboxed children from the controlling terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #667 (F1/F2). A bounded child used `process_group(0)`, so it stayed in the caller's session and could inject into the controlling terminal with `TIOCSTI`, and Landlock allowed writes to the whole `/dev/pts` directory, so it could write to another terminal's slave. - `bounded_process`: start each child with `setsid()` instead of `process_group(0)`. The child keeps a process group whose id equals its pid, so `terminate_process_group*` still signals exactly its own group, and it now has no controlling terminal, so the kernel refuses `TIOCSTI`. - `landlock`: drop `/dev/tty` and `/dev/pts` from the write allow list. On ABI5+ this also leaves them out of the handled `IoctlDev` access. `/dev/ptmx` stays. - `seatbelt`: keep the `/dev` write allow and re-deny `/dev/tty*` last, so `/dev/null` and friends still work but terminal slaves do not. - Tests: session/process-group detachment, a pty-based `TIOCSTI` probe with a positive and negative control, a terminal-slave write denial on both backends, and a `git` pager case; plus a Seatbelt profile-order unit test. - SECURITY.md documents the change and the visible difference for a program that opens `/dev/tty` directly. 判断: removed `/dev/tty` from the Linux write allow as well as `/dev/pts`, so `TIOCSTI` is closed even below Landlock ABI5 where device ioctls cannot be filtered; the child has no controlling terminal, so it has no use for `/dev/tty`. 判断: kept `/dev/ptmx` writable and, on macOS, kept the broad `/dev` allow with a last-match `/dev/tty*` deny instead of enumerating device literals, to keep the change minimal and avoid breaking the required-tool cases. 判断: relied on `setsid()` for `TIOCSTI` rather than adding a dedicated ABI5 `IoctlDev` rule; a `TIOCSTI` attempt also needs the terminal to be the process's controlling terminal, which the child no longer has. 本文に無い指摘: an interactive pager that reads keypresses from `/dev/tty` directly now sees no controlling terminal; standard streams and the captured (non-interactive) pager path are unchanged. Co-authored-by: CommandCodeBot --- SECURITY.md | 16 + src/bounded_process.rs | 41 ++- src/tools/os_sandbox/landlock.rs | 15 +- src/tools/os_sandbox/seatbelt.rs | 29 ++ tests/issue502_os_sandbox_write.rs | 496 ++++++++++++++++++++++++++++- 5 files changed, 588 insertions(+), 9 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 710e6960..8a6bcad0 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -89,6 +89,22 @@ command before the shell starts. `sed -i` — and creating a hardlink to an inode outside the workspace. The only writable locations are the workspace, the run's temp directory, the known dependency caches, and device files such as `/dev/null`. +- Terminal isolation (Issue #667): a command runs in its own session with no + controlling terminal (`setsid()` at spawn), so it cannot inject keystrokes into + the terminal the run was started from with `TIOCSTI` — the kernel only allows + `TIOCSTI` on the caller's controlling terminal, and the child no longer has + one. It also cannot write to another terminal's slave: on Linux `/dev/pts` is + not writable, and on macOS the profile re-denies `/dev/tty*` last. The visible + change is to a program that opens the terminal *directly*: an interactive pager + waiting for keys on `/dev/tty` now sees no controlling terminal. A command + whose standard streams are the terminal is unchanged — it keeps reading and + writing its inherited descriptors exactly as before, so the non-interactive + `git` pager path (output captured, pager writes to stdout) is unaffected. + The same `setsid()` applies to every bounded child, including the launches + that are not confined — the workspace `git` state check, the `lsof` port + lookup, the fetch and interaction probes, and the backend availability probes. + They run non-interactive commands with captured standard streams, so losing + the controlling terminal changes nothing for them. - Read confinement (Issue #657) denies reading the home secret locations (`.ssh`, `.aws`, `.gnupg`, `.netrc`, `.git-credentials`, `.npmrc`, `.pypirc`, `.docker/config.json`, `.kube`, `.config/gh`, `.config/gcloud`) and, on macOS, diff --git a/src/bounded_process.rs b/src/bounded_process.rs index e73e6d5a..b9b60459 100644 --- a/src/bounded_process.rs +++ b/src/bounded_process.rs @@ -50,7 +50,7 @@ impl BoundedProcessOutput { pub fn spawn_child(command: &mut Command) -> io::Result { apply_child_env_allowlist(command); - configure_process_group(command); + configure_child_session(command); command.spawn() } @@ -399,7 +399,7 @@ pub fn terminate_process_group(child: &mut Child) { /// Resolves a spawned child PID into a process-group id that is safe to /// signal with a negative `kill` target. The child is spawned with -/// `process_group(0)`, so its PID equals its group id, but `-1` would signal +/// `setsid()`, so its PID equals its group id, but `-1` would signal /// every signalable process and `0`/`1` would target the caller or init. #[cfg(unix)] fn signalable_process_group(pid: u32) -> Option { @@ -416,7 +416,7 @@ pub fn terminate_process_group_by_pid(pid: u32) -> io::Result<()> { )); }; // SAFETY: `pgid` is greater than 1 and derived from a child spawned - // with `process_group(0)`, so `-pgid` names only that child's own + // with `setsid()`, so `-pgid` names only that child's own // process group, never pid 0/1 or the caller's group. unsafe { libc::kill(-pgid, libc::SIGTERM); @@ -476,7 +476,7 @@ fn terminate_process_group_with_grace( { if let Some(pgid) = signalable_process_group(child.id()) { // SAFETY: `pgid` is greater than 1 and was created with - // `configure_process_group(0)` for this child, so `-pgid` targets + // `configure_child_session()` for this child, so `-pgid` targets // only the child's process group, never pid 0/1 or the caller's. unsafe { libc::kill(-pgid, libc::SIGTERM); @@ -520,11 +520,40 @@ fn output_from_child( }) } -fn configure_process_group(command: &mut Command) { +/// Put the child in a new session, and so a new process group, with no +/// controlling terminal (Issue #667). +/// +/// `setsid()` sets both the session id and the process-group id to the child's +/// pid, so the child's group id still equals its pid and +/// `terminate_process_group*` keeps signalling exactly the child's own group +/// with `-pid`. Dropping the controlling terminal is the point: the kernel only +/// allows `TIOCSTI` when the fd names the caller's controlling terminal, so a +/// child in its own session can no longer inject keystrokes into the terminal +/// the caller was started from (`/dev/tty` also stops resolving for it). +/// +/// This replaces `Command::process_group(0)`: `setsid()` fails with `EPERM` for a +/// process that is already a process-group leader, so the two cannot be combined +/// and `setsid()` alone provides the group. +fn configure_child_session(command: &mut Command) { #[cfg(unix)] { use std::os::unix::process::CommandExt; - command.process_group(0); + // SAFETY: `pre_exec` runs after fork and before exec. The closure calls + // only `setsid`; it does not allocate, lock, or log. A forked child is + // never a process-group leader (its pid is new), so `setsid` cannot fail + // with `EPERM`. + unsafe { + command.pre_exec(|| { + if libc::setsid() == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + } + #[cfg(not(unix))] + { + let _ = command; } } diff --git a/src/tools/os_sandbox/landlock.rs b/src/tools/os_sandbox/landlock.rs index f6ce2df4..dccf9ab3 100644 --- a/src/tools/os_sandbox/landlock.rs +++ b/src/tools/os_sandbox/landlock.rs @@ -40,6 +40,11 @@ //! - Availability is probed once at run start from the Landlock ABI query and a //! child `restrict_self`, so a seccomp filter that denies either syscall is an //! honest "unavailable" instead of a silent pass. +//! - Terminal isolation (Issue #667): `/dev/tty` and the `/dev/pts` directory +//! are not allowed for writing, so a confined command cannot write to another +//! terminal's slave; the child's own session (a `setsid()` in +//! `bounded_process`) has no controlling terminal, so `TIOCSTI` is refused by +//! the kernel even where Landlock cannot filter device ioctls (below ABI5). use std::collections::HashSet; use std::os::fd::{AsRawFd, OwnedFd}; @@ -64,15 +69,21 @@ const PROBE_TIMEOUT: Duration = Duration::from_secs(10); /// Device files that either read-only tools or an attached terminal expect to /// write. Only the ones that exist are allowed. +/// +/// `/dev/tty` and `/dev/pts` are deliberately absent (Issue #667). A sandboxed +/// child runs in its own session with no controlling terminal, so it has no +/// legitimate use for either, and allowing the whole `/dev/pts` directory let it +/// write to another terminal's slave. Landlock grants the handled `IoctlDev` +/// access (ABI5) only where a rule covers the location, so leaving these two out +/// also closes device ioctls on them — including `TIOCSTI` — not just writes. +/// `/dev/ptmx` stays allowed so allocating a pty still works. const DEVICE_PATHS: &[&str] = &[ "/dev/null", "/dev/zero", "/dev/full", "/dev/random", "/dev/urandom", - "/dev/tty", "/dev/ptmx", - "/dev/pts", ]; /// The Landlock ABI detected for the running kernel, or the reason it cannot be diff --git a/src/tools/os_sandbox/seatbelt.rs b/src/tools/os_sandbox/seatbelt.rs index ab35704d..a9b31095 100644 --- a/src/tools/os_sandbox/seatbelt.rs +++ b/src/tools/os_sandbox/seatbelt.rs @@ -13,6 +13,11 @@ //! keep working. For an `--offline` run (#658) the profile also denies every //! network operation except loopback TCP and unix sockets, so an arbitrary //! program the lexical guard cannot name cannot reach the network. +//! +//! Terminal isolation (Issue #667): the write profile re-denies `/dev/tty*` +//! last, so a confined command cannot write to a terminal (its own or another +//! user's slave), and the child's own session (a `setsid()` in +//! `bounded_process`) has no controlling terminal, so `TIOCSTI` is refused. use std::path::Path; use std::process::{Command, Stdio}; @@ -31,6 +36,11 @@ const UNSET_PATH: &str = "/commandagent-sandbox-unset"; /// `(deny file-write*)` then confines every write to the parameters below. The /// read-deny rules are appended per run by [`profile`], because the number of /// home secret locations is fixed but their names travel as parameters. +/// +/// The device rule allows the whole `/dev` tree and then re-denies `/dev/tty*` +/// last (Seatbelt applies the last matching rule), so a confined command can +/// still use `/dev/null` and friends but cannot write to a terminal — its own or +/// another user's slave (`/dev/ttys*`) — see Issue #667. const WRITE_PROFILE: &str = r#" (version 1) (allow default) @@ -44,6 +54,7 @@ const WRITE_PROFILE: &str = r#" (allow file-write* (subpath (param "CACHE_PLAYWRIGHT"))) (allow file-write* (literal (param "CACHE_CARGO_PACKAGE"))) (allow file-write* (regex #"^/dev/")) +(deny file-write* (regex #"^/dev/tty")) "#; /// The `--offline` network rules (Issue #658). `(deny network*)` blocks every @@ -283,6 +294,24 @@ mod tests { assert!(!offline.contains("(allow network*"), "{offline}"); } + /// Issue #667: the device allow comes first and the terminal deny last, so + /// Seatbelt's last-match rule closes `/dev/tty*` (including another + /// terminal's `/dev/ttys*`) while `/dev/null` and friends stay writable. + #[test] + fn profile_denies_terminal_writes_after_allowing_devices() { + let built = profile(0, false); + let allow_devices = built + .find("(allow file-write* (regex #\"^/dev/\"))") + .expect("device allow rule"); + let deny_terminal = built + .find("(deny file-write* (regex #\"^/dev/tty\"))") + .expect("terminal deny rule"); + assert!( + allow_devices < deny_terminal, + "the terminal deny must come after the device allow: {built}" + ); + } + #[test] fn probe_classifies_a_zero_exit_as_available() { assert_eq!( diff --git a/tests/issue502_os_sandbox_write.rs b/tests/issue502_os_sandbox_write.rs index c00594ec..5a6b8c05 100644 --- a/tests/issue502_os_sandbox_write.rs +++ b/tests/issue502_os_sandbox_write.rs @@ -15,6 +15,10 @@ //! share the `off` and unavailable paths. The outside fixtures live inside one //! test tempdir and are never written outside it. +#[cfg(unix)] +use std::ffi::CStr; +#[cfg(unix)] +use std::os::fd::{AsRawFd, FromRawFd}; use std::path::{Path, PathBuf}; #[cfg(unix)] use std::process::{Command, Stdio}; @@ -22,7 +26,7 @@ use std::sync::{Mutex, MutexGuard}; use std::time::Duration; #[cfg(unix)] -use commandagent::bounded_process::{self, BoundedProcessOutput}; +use commandagent::bounded_process::{self, BoundedProcessOutcomeKind, BoundedProcessOutput}; use commandagent::minimal_loop::verifier_env; use commandagent::planner::verify::{NormalizedVerifyCommand, normalize_verify_command}; use commandagent::tools::allow_policy::bash_verify_command_is_auto_approvable; @@ -1155,3 +1159,493 @@ fn sandbox_off_and_unavailable_do_not_change_the_lexical_decision() { } os_sandbox::reset_for_test(); } + +// --------------------------------------------------------------------------- +// Terminal isolation (Issue #667): a confined command must not be able to +// inject keystrokes into the caller's terminal with `TIOCSTI`, nor write to +// another terminal's slave. +// --------------------------------------------------------------------------- + +/// The `TIOCSCTTY` request number, which the `libc` crate does not name on +/// macOS. `TCGETS`/BSD `_IO('t', 97)` differs per platform. +#[cfg(target_os = "macos")] +const TIOCSCTTY_REQUEST: libc::c_ulong = 0x2000_7461; +#[cfg(target_os = "linux")] +const TIOCSCTTY_REQUEST: libc::c_ulong = 0x540E; + +/// A Python one-liner that tries `TIOCSTI` with `inject` on fd 0 and reports the +/// outcome, so the attempt travels through a real program exactly as a command +/// would. The kernel only allows `TIOCSTI` when fd 0 is the caller's controlling +/// terminal. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn tiocsti_script(inject: char) -> String { + format!( + r#"import fcntl, termios, sys +try: + fcntl.ioctl(0, termios.TIOCSTI, b'{inject}') + sys.stdout.write('TIOCSTI_OK\n') +except OSError as error: + sys.stdout.write('TIOCSTI_FAIL %d\n' % error.errno) +"# + ) +} + +/// Open a pty the test owns and return `(master, slave, slave_path)`. The master +/// is non-blocking so a test can drain it without waiting; both descriptors stay +/// open in the returned tuple. Only ptys the test created are ever used — a test +/// never opens the worker's or the user's real terminal. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn open_pty() -> (std::fs::File, std::fs::File, PathBuf) { + let master = unsafe { libc::posix_openpt(libc::O_RDWR | libc::O_NOCTTY) }; + assert!( + master >= 0, + "posix_openpt: {}", + std::io::Error::last_os_error() + ); + assert_eq!( + unsafe { libc::grantpt(master) }, + 0, + "grantpt: {}", + std::io::Error::last_os_error() + ); + assert_eq!( + unsafe { libc::unlockpt(master) }, + 0, + "unlockpt: {}", + std::io::Error::last_os_error() + ); + let name = unsafe { libc::ptsname(master) }; + assert!( + !name.is_null(), + "ptsname: {}", + std::io::Error::last_os_error() + ); + let flags = unsafe { libc::fcntl(master, libc::F_GETFL) }; + let _ = unsafe { libc::fcntl(master, libc::F_SETFL, flags | libc::O_NONBLOCK) }; + let slave_path = PathBuf::from( + unsafe { CStr::from_ptr(name) } + .to_string_lossy() + .into_owned(), + ); + let slave = unsafe { libc::open(name, libc::O_RDWR | libc::O_NOCTTY) }; + assert!( + slave >= 0, + "open slave: {}", + std::io::Error::last_os_error() + ); + ( + unsafe { std::fs::File::from_raw_fd(master) }, + unsafe { std::fs::File::from_raw_fd(slave) }, + slave_path, + ) +} + +/// A `Stdio` for a duplicate of `file`, so the caller keeps the original open. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn dup_stdio(file: &std::fs::File) -> Stdio { + let fd = unsafe { libc::dup(file.as_raw_fd()) }; + assert!(fd >= 0, "dup: {}", std::io::Error::last_os_error()); + Stdio::from(unsafe { std::fs::File::from_raw_fd(fd) }) +} + +/// Drain the non-blocking master side of a pty into a string. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn drain_master(master: &std::fs::File) -> String { + use std::io::Read; + + let mut reader: &std::fs::File = master; + let mut text = String::new(); + let mut buffer = [0_u8; 256]; + loop { + match reader.read(&mut buffer) { + Ok(0) => break, + Ok(read) => text.push_str(&String::from_utf8_lossy(&buffer[..read])), + Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => break, + Err(_) => break, + } + } + text +} + +/// Run `sh -c command` with no sandbox and no session change, taking `stdout` as +/// the command's own terminal. The positive control for a terminal-slave write +/// must not go through `bounded_process`: that starts a new session, and a +/// session leader that opens a fresh tty blocks, so the write is measured +/// without the session change. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn run_unwrapped(root: &Path, command: &str, stdout: &std::fs::File) -> std::process::Output { + Command::new("sh") + .arg("-c") + .arg(command) + .current_dir(root) + .stdin(Stdio::null()) + .stdout(dup_stdio(stdout)) + .stderr(Stdio::piped()) + .output() + .expect("spawn unwrapped command") +} + +/// Run `sh -c command` through the installed sandbox, taking `stdout` as the +/// command's own terminal. Standard I/O is set after wrapping because the macOS +/// backend rewrites the command and a `Stdio` set beforehand would not survive. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn run_wrapped_with_stdout( + root: &Path, + command: &str, + stdout: &std::fs::File, +) -> BoundedProcessOutput { + let mut process = Command::new("sh"); + process.arg("-c").arg(command).current_dir(root); + let mut process = os_sandbox::wrap_command(process).expect("available sandbox wraps"); + process + .stdin(Stdio::null()) + .stdout(dup_stdio(stdout)) + .stderr(Stdio::piped()); + bounded_process::run_with_timeout(&mut process, TIMEOUT).expect("spawn wrapped command") +} + +/// Issue #667: every bounded child is moved into its own session by `setsid()`, +/// so it keeps a process group whose id equals its pid (termination by `-pid` +/// still works) and has no controlling terminal to inject into. The child's +/// session must differ from the test process's session. +#[cfg(unix)] +#[test] +fn bounded_child_runs_in_its_own_session_and_process_group() { + let mut command = Command::new("python3"); + command + .arg("-c") + .arg("import os; print(os.getsid(0), os.getpgrp(), os.getpid())") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + let output = bounded_process::run_with_timeout(&mut command, TIMEOUT).expect("spawn child"); + assert!(output.success(), "{output:?}"); + let text = String::from_utf8_lossy(&output.stdout); + let mut parts = text.split_whitespace(); + let sid: i32 = parts.next().expect("sid").parse().expect("sid number"); + let pgrp: i32 = parts.next().expect("pgrp").parse().expect("pgrp number"); + let pid: i32 = parts.next().expect("pid").parse().expect("pid number"); + assert_eq!(pgrp, pid, "the child's group id must equal its pid: {text}"); + assert_eq!(sid, pid, "the child must lead its own session: {text}"); + let caller_sid = unsafe { libc::getsid(0) }; + assert_ne!( + sid, caller_sid, + "the child must not share the caller's session: {text}" + ); +} + +/// Issue #667: an isolated command cannot inject input into the controlling +/// terminal with `TIOCSTI`. This runs as its own process because it must become +/// a session leader and own a pty; that must not touch the shared test process, +/// and it only ever uses the pty the probe created. Injection is judged by +/// whether the injected byte appears on the pty's master side. When the +/// environment cannot exercise `TIOCSTI` (a kernel with `legacy_tiocsti=0`, or no +/// controlling terminal) the probe reports `UNVERIFIED (reason)` and records a +/// line for the run summary instead of passing silently. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[test] +fn issue667_tiocsti_session_probe() { + if std::env::var_os("COMMANDAGENT_ISSUE667_TIOCSTI_PROBE").is_none() { + return; + } + let (master, slave, _slave_path) = open_pty(); + assert_ne!( + unsafe { libc::setsid() }, + -1, + "setsid: {}", + std::io::Error::last_os_error() + ); + assert_eq!( + unsafe { libc::ioctl(slave.as_raw_fd(), TIOCSCTTY_REQUEST, 0) }, + 0, + "TIOCSCTTY: {}", + std::io::Error::last_os_error() + ); + // The probe now owns a controlling terminal; the kernel sends SIGHUP to the + // session leader when it exits, so ignore it and finish normally. + unsafe { + libc::signal(libc::SIGHUP, libc::SIG_IGN); + } + // Echo injected bytes back to the master, so the master side is the evidence + // that an injection happened. + let mut termios: libc::termios = unsafe { std::mem::zeroed() }; + assert_eq!( + unsafe { libc::tcgetattr(slave.as_raw_fd(), &mut termios) }, + 0, + "tcgetattr: {}", + std::io::Error::last_os_error() + ); + termios.c_lflag |= libc::ECHO; + assert_eq!( + unsafe { libc::tcsetattr(slave.as_raw_fd(), libc::TCSANOW, &termios) }, + 0, + "tcsetattr: {}", + std::io::Error::last_os_error() + ); + + // Positive control: a plain child in this session inherits the pty as its + // controlling terminal, so the kernel allows `TIOCSTI` and the injected 'Z' + // must echo onto the master. If it does not, the environment cannot exercise + // `TIOCSTI` and the denial cannot be measured. + let positive_out = run_tiocsti_attempt(&slave, 'Z', false); + let positive_echo = drain_master(&master); + if !positive_out.contains("TIOCSTI_OK") || !positive_echo.contains('Z') { + record_unverified(&format!( + "the positive control could not inject (child said {:?}, master echoed {:?})", + positive_out.trim(), + positive_echo + )); + return; + } + + // Negative control: the same attempt through the bounded-process launcher, + // which puts the child in its own session with no controlling terminal. Its + // injected 'Y' must not reach the master. + let negative_out = run_tiocsti_attempt(&slave, 'Y', true); + let negative_echo = drain_master(&master); + assert!( + !negative_echo.contains('Y'), + "an isolated child injected into the controlling terminal: master echoed {negative_echo:?}" + ); + assert!( + negative_out.contains("TIOCSTI_FAIL"), + "TIOCSTI must fail without a controlling terminal: {negative_out:?}" + ); + record_tiocsti_step_summary("blocked"); + println!( + "issue667-tiocsti: BLOCKED (the positive control injected 'Z'; the isolated child could not inject 'Y')" + ); +} + +/// Run one `TIOCSTI` attempt with fd 0 as `slave` and return the child's stdout. +/// A plain attempt keeps the probe's session and its controlling terminal; a +/// wrapped attempt goes through `bounded_process`, which `setsid()`s the child. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn run_tiocsti_attempt(slave: &std::fs::File, inject: char, wrapped: bool) -> String { + let script = tiocsti_script(inject); + let mut command = Command::new("python3"); + command + .arg("-c") + .arg(script) + .stdin(dup_stdio(slave)) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + if wrapped { + let output = bounded_process::run_with_timeout(&mut command, TIMEOUT) + .expect("run the isolated attempt"); + String::from_utf8_lossy(&output.stdout).into_owned() + } else { + let output = command.output().expect("run the plain attempt"); + String::from_utf8_lossy(&output.stdout).into_owned() + } +} + +/// Report an unmeasurable `TIOCSTI` probe without passing silently. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn record_unverified(reason: &str) { + eprintln!("issue667-tiocsti: UNVERIFIED ({reason})"); + record_tiocsti_step_summary(&format!("unverified: {reason}")); +} + +/// Append the `TIOCSTI` verdict to `$GITHUB_STEP_SUMMARY` when it is set, with +/// the kernel's `legacy_tiocsti` value when the file exists, so a runner can tell +/// a measured denial from an unverifiable environment. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn record_tiocsti_step_summary(status: &str) { + let legacy = std::fs::read_to_string("/proc/sys/dev/tty/legacy_tiocsti") + .map(|value| value.trim().to_string()) + .unwrap_or_else(|_| "unknown".to_string()); + let Some(path) = std::env::var_os("GITHUB_STEP_SUMMARY") else { + return; + }; + let line = format!("issue667/tiocsti: {status}; legacy_tiocsti={legacy}\n"); + let _ = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(path) + .and_then(|mut file| std::io::Write::write_all(&mut file, line.as_bytes())); +} + +/// Runs [`issue667_tiocsti_session_probe`] in a fresh process and asserts it +/// reaches a verdict. The probe prints `BLOCKED` when it demonstrated the +/// injection and its denial, or `UNVERIFIED (reason)` when the environment +/// cannot exercise `TIOCSTI`; either verdict records a `$GITHUB_STEP_SUMMARY` +/// line, so an unmeasurable environment is never a silent pass. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[test] +fn isolated_child_cannot_inject_into_the_controlling_terminal() { + let exe = std::env::current_exe().expect("current exe"); + let output = Command::new(exe) + .args([ + "--exact", + "issue667_tiocsti_session_probe", + "--nocapture", + "--test-threads=1", + ]) + .env("COMMANDAGENT_ISSUE667_TIOCSTI_PROBE", "1") + .output() + .expect("run the tiocsti probe as a subprocess"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "the tiocsti probe failed:\n{stdout}\n{stderr}" + ); + let verdict = stdout + .lines() + .find(|line| line.contains("issue667-tiocsti:")) + .unwrap_or_else(|| panic!("the tiocsti probe reported no verdict:\n{stdout}\n{stderr}")); + eprintln!("{verdict}"); +} + +/// Issue #667: a confined command keeps its own terminal (its inherited stdio) +/// but cannot open another terminal's slave for writing. Both ptys are created +/// by the test — the command's own terminal is one and the "other terminal" is +/// the second — so no real terminal is ever touched. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn assert_terminal_slave_write_is_denied(fixture: &Fixture) { + let (_own_master, own_slave, _own_path) = open_pty(); + let (_other_master, _other_slave, other_path) = open_pty(); + let command = format!("echo written > '{}'", other_path.display()); + + // Positive control: without a sandbox the same write succeeds, so the + // fixture proves the sandbox is what denies it. + os_sandbox::reset_for_test(); + let before = run_unwrapped(&fixture.root, &command, &own_slave); + assert!( + before.status.success(), + "a write to a terminal slave must succeed without the sandbox: {}", + String::from_utf8_lossy(&before.stderr) + ); + + install_available(fixture); + let after = run_wrapped_with_stdout(&fixture.root, &command, &own_slave); + assert_eq!( + after.kind, + BoundedProcessOutcomeKind::Exited, + "the denied write must fail, not hang or time out: {after:?}" + ); + assert!( + !after.success(), + "a write to another terminal's slave must be denied: {}", + String::from_utf8_lossy(&after.stderr) + ); + let denial = String::from_utf8_lossy(&after.stderr); + assert!( + denial.contains("Permission denied") || denial.contains("Operation not permitted"), + "the denial must be a permission error, not a hang: {denial}" + ); + os_sandbox::reset_for_test(); +} + +/// Issue #667 on Linux: `/dev/pts` is no longer in the Landlock write allow +/// list, so an `open` for writing to another terminal's slave is refused. +#[cfg(target_os = "linux")] +#[test] +fn linux_applied_terminal_slave_write_is_denied() { + let _guard = sandbox_lock(); + if !require_landlock_backend() { + eprintln!("SKIP: Landlock backend unavailable"); + return; + } + let fixture = fixture(); + assert_terminal_slave_write_is_denied(&fixture); +} + +/// Issue #667 on macOS: the write profile allows `/dev` and then re-denies +/// `/dev/tty*` last, which covers the `/dev/ttys*` pty slaves, so writing to +/// another terminal is refused. +#[cfg(target_os = "macos")] +#[test] +fn applied_terminal_slave_write_is_denied() { + let _guard = sandbox_lock(); + if !backend_available() { + eprintln!("SKIP: sandbox backend unavailable (nested sandbox?)"); + return; + } + let fixture = fixture(); + assert_terminal_slave_write_is_denied(&fixture); +} + +/// Issue #667: a command that needs a terminal (a `git` pager) still works. +/// `git` runs its pager only when stdout is a tty, so the fixture gives the +/// wrapped command the pty slave as stdout; the pager (`cat > pager.out`) then +/// runs inside the sandbox and writes the allowed workspace. This is the +/// behavior SECURITY.md keeps: stdio still works, only opening `/dev/tty` +/// directly changed. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn assert_git_pager_runs_inside_sandbox(fixture: &Fixture) { + let (_master, slave, _slave_path) = open_pty(); + std::process::Command::new("git") + .args(["init", "-q"]) + .current_dir(&fixture.root) + .status() + .expect("git init"); + std::process::Command::new("git") + .args([ + "-c", + "user.email=probe@example.invalid", + "-c", + "user.name=probe", + "commit", + "--allow-empty", + "-qm", + "issue667 pager probe", + ]) + .current_dir(&fixture.root) + .status() + .expect("git commit"); + + let mut process = Command::new("sh"); + process + .arg("-c") + .arg("git -c core.pager='cat > pager.out' --paginate log --oneline") + .current_dir(&fixture.root); + let mut process = os_sandbox::wrap_command(process).expect("available sandbox wraps"); + // Standard I/O is set after wrapping: the macOS backend rewrites the + // command, so a `Stdio` set beforehand would not survive the transfer. + process + .stdin(Stdio::null()) + .stdout(dup_stdio(&slave)) + .stderr(dup_stdio(&slave)); + let output = + bounded_process::run_with_timeout(&mut process, TIMEOUT).expect("run the pager command"); + assert!( + output.success(), + "the pager command failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let pager_out = std::fs::read_to_string(fixture.root.join("pager.out")).unwrap_or_default(); + assert!( + pager_out.contains("issue667 pager probe"), + "git's pager did not run inside the sandbox: {pager_out:?}" + ); +} + +#[cfg(target_os = "linux")] +#[test] +fn linux_applied_git_pager_still_runs_inside_the_sandbox() { + let _guard = sandbox_lock(); + if !require_landlock_backend() { + eprintln!("SKIP: Landlock backend unavailable"); + return; + } + let fixture = fixture(); + install_available(&fixture); + assert_git_pager_runs_inside_sandbox(&fixture); + os_sandbox::reset_for_test(); +} + +#[cfg(target_os = "macos")] +#[test] +fn applied_git_pager_still_runs_inside_the_sandbox() { + let _guard = sandbox_lock(); + if !backend_available() { + eprintln!("SKIP: sandbox backend unavailable (nested sandbox?)"); + return; + } + let fixture = fixture(); + install_available(&fixture); + assert_git_pager_runs_inside_sandbox(&fixture); + os_sandbox::reset_for_test(); +} From 3e6158cd9815a848e19891413e585e28298d271a Mon Sep 17 00:00:00 2001 From: kewton Date: Wed, 14 Oct 2026 02:22:44 +0900 Subject: [PATCH 2/7] fix(tools): detach a terminal stdin for bounded children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #667 follow-up. `setsid()` removes the controlling terminal, so the job-control check no longer applied and a child that inherited the caller's terminal on fd 0 could read the keys the user types during a run (previously a background process group was refused with `SIGTTIN`/`EIO`). - `bounded_process`: in the same `pre_exec`, after `setsid()`, replace a terminal fd 0 with `/dev/null` (`isatty`/`open`/`dup2`/`close` only). A `Stdio::null()`, piped, or file stdin is unchanged, as are stdout and stderr. - Tests: a probe that makes its own pty the controlling terminal and fd 0, then starts a child with stdin left unset through `bounded_process` in both the isolated and unisolated launches, and asserts `isatty(0)` is false and the read returns EOF at once. - `SECURITY.md`: correct the stdin statement and list the launch points that inherited a terminal stdin. 判断: only a terminal stdin is replaced; pipes, files, `/dev/null`, and every standard output and error are left alone, so a caller that pipes input keeps working. 判断: the probe asserts the unisolated launch always and the isolated launch only when the sandbox backend probes as available, printing which legs it measured. Co-authored-by: CommandCodeBot --- SECURITY.md | 28 ++++-- src/bounded_process.rs | 53 ++++++++++- tests/issue502_os_sandbox_write.rs | 148 +++++++++++++++++++++++++++++ 3 files changed, 215 insertions(+), 14 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 8a6bcad0..cc0075a0 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -96,15 +96,25 @@ command before the shell starts. one. It also cannot write to another terminal's slave: on Linux `/dev/pts` is not writable, and on macOS the profile re-denies `/dev/tty*` last. The visible change is to a program that opens the terminal *directly*: an interactive pager - waiting for keys on `/dev/tty` now sees no controlling terminal. A command - whose standard streams are the terminal is unchanged — it keeps reading and - writing its inherited descriptors exactly as before, so the non-interactive - `git` pager path (output captured, pager writes to stdout) is unaffected. - The same `setsid()` applies to every bounded child, including the launches - that are not confined — the workspace `git` state check, the `lsof` port - lookup, the fetch and interaction probes, and the backend availability probes. - They run non-interactive commands with captured standard streams, so losing - the controlling terminal changes nothing for them. + waiting for keys on `/dev/tty` now sees no controlling terminal. A terminal + standard input is replaced with `/dev/null`, so a command cannot read the keys + the user types during a run — it could not before either, because the child sat + in a background process group of the session's terminal and a read was refused + with `SIGTTIN` (`EIO` once that signal is ignored); the change makes that + explicit instead of relying on the job-control check. A standard output or + error that is the terminal is unchanged: the command keeps writing its + inherited descriptors, so the non-interactive `git` pager path (output + captured, pager writes to stdout) is unaffected. + Every standard input is either set to `null` or a pipe, or inherited; an + inherited terminal stdin is the only case the `/dev/null` replacement changes. + The confined launches are the model Bash and its `bash:verify` interval, the + verifier/acceptance commands, and the runtime dependency setup. The + non-confined launches are the workspace `git` state check, the `lsof` port + lookups, the fetch, interaction, browser, pipeline, CLI-argv, and + declarative-check probes, the community `esbuild` bundle, the dev-server start, + and the backend availability probes. All of them run non-interactive commands + with a non-terminal standard input or an inherited one, so the change is a + non-readable stdin rather than a behavior change. - Read confinement (Issue #657) denies reading the home secret locations (`.ssh`, `.aws`, `.gnupg`, `.netrc`, `.git-credentials`, `.npmrc`, `.pypirc`, `.docker/config.json`, `.kube`, `.config/gh`, `.config/gcloud`) and, on macOS, diff --git a/src/bounded_process.rs b/src/bounded_process.rs index b9b60459..3d7761ee 100644 --- a/src/bounded_process.rs +++ b/src/bounded_process.rs @@ -533,21 +533,23 @@ fn output_from_child( /// /// This replaces `Command::process_group(0)`: `setsid()` fails with `EPERM` for a /// process that is already a process-group leader, so the two cannot be combined -/// and `setsid()` alone provides the group. +/// and `setsid()` alone provides the group. Losing the controlling terminal also +/// makes a terminal stdin readable, so [`detach_terminal_stdin`] points it at +/// `/dev/null` in the same `pre_exec`. fn configure_child_session(command: &mut Command) { #[cfg(unix)] { use std::os::unix::process::CommandExt; // SAFETY: `pre_exec` runs after fork and before exec. The closure calls - // only `setsid`; it does not allocate, lock, or log. A forked child is - // never a process-group leader (its pid is new), so `setsid` cannot fail - // with `EPERM`. + // only `setsid`, `isatty`, `open`, `dup2`, and `close`; it does not + // allocate, lock, or log. A forked child is never a process-group leader + // (its pid is new), so `setsid` cannot fail with `EPERM`. unsafe { command.pre_exec(|| { if libc::setsid() == -1 { return Err(std::io::Error::last_os_error()); } - Ok(()) + detach_terminal_stdin() }); } } @@ -557,6 +559,47 @@ fn configure_child_session(command: &mut Command) { } } +/// Replace a terminal standard input with `/dev/null` in the freshly forked +/// child. +/// +/// Before `setsid()` the child sat in a background process group of the caller's +/// session, so a read from the terminal was refused with `SIGTTIN` (or `EIO` +/// once that signal is ignored). After `setsid()` the child has no controlling +/// terminal, so the job-control check no longer applies and a read would succeed, +/// letting a command read the keys the user types during a run. Pointing a +/// terminal stdin at `/dev/null` restores a non-readable stdin. A stdin that is +/// not a terminal (`Stdio::null()`, a pipe, or a file) is left unchanged, as are +/// every standard output and error. +/// +/// # pre_exec contract +/// +/// This runs between `fork` and `exec`, so it uses only `isatty`, `open`, +/// `dup2`, and `close` and never allocates, locks, or logs. Errors are built with +/// [`std::io::Error::last_os_error`], which does not allocate. +#[cfg(unix)] +fn detach_terminal_stdin() -> std::io::Result<()> { + // SAFETY: this is the classic `isatty`/`open`/`dup2`/`close` sequence on the + // child's own descriptors; the path is a fixed static C string. + unsafe { + if libc::isatty(0) != 1 { + return Ok(()); + } + let devnull = libc::open(c"/dev/null".as_ptr(), libc::O_RDONLY); + if devnull < 0 { + return Err(std::io::Error::last_os_error()); + } + if libc::dup2(devnull, 0) < 0 { + let error = std::io::Error::last_os_error(); + libc::close(devnull); + return Err(error); + } + if devnull != 0 { + libc::close(devnull); + } + Ok(()) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/issue502_os_sandbox_write.rs b/tests/issue502_os_sandbox_write.rs index 5a6b8c05..6b0649e7 100644 --- a/tests/issue502_os_sandbox_write.rs +++ b/tests/issue502_os_sandbox_write.rs @@ -1498,6 +1498,154 @@ fn isolated_child_cannot_inject_into_the_controlling_terminal() { eprintln!("{verdict}"); } +/// Issue #667: with `setsid()` the child has no controlling terminal, so a +/// terminal standard input would become readable (the job-control check no +/// longer applies). The launcher must replace a terminal stdin with `/dev/null` +/// for every bounded child, isolated and unisolated alike. The probe makes its +/// own pty the session's controlling terminal and puts it on fd 0, then starts a +/// child with stdin left unset so it would inherit that terminal. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[test] +fn issue667_stdin_probe() { + if std::env::var_os("COMMANDAGENT_ISSUE667_STDIN_PROBE").is_none() { + return; + } + let (_master, slave, _slave_path) = open_pty(); + assert_ne!( + unsafe { libc::setsid() }, + -1, + "setsid: {}", + std::io::Error::last_os_error() + ); + assert_eq!( + unsafe { libc::ioctl(slave.as_raw_fd(), TIOCSCTTY_REQUEST, 0) }, + 0, + "TIOCSCTTY: {}", + std::io::Error::last_os_error() + ); + unsafe { + libc::signal(libc::SIGHUP, libc::SIG_IGN); + } + // Make the pty the probe's own standard input, so a child that leaves stdin + // unset inherits a terminal and the launcher has to detach it. + assert_ne!( + unsafe { libc::dup2(slave.as_raw_fd(), 0) }, + -1, + "dup2: {}", + std::io::Error::last_os_error() + ); + + // The unisolated launch. + assert_stdin_is_detached(&run_stdin_child(false)); + + // The isolated launch, when the backend is available. + if matches!(os_sandbox::probe(), SandboxAvailability::Available) { + let dir = tempfile::tempdir().expect("tempdir"); + let workspace = dir.path().join("ws"); + let run_tmp = dir.path().join("tmp"); + std::fs::create_dir_all(&workspace).unwrap(); + std::fs::create_dir_all(&run_tmp).unwrap(); + os_sandbox::install_for_test( + SandboxMode::Auto, + SandboxPolicy::new(&workspace, &run_tmp), + SandboxAvailability::Available, + ); + let sandboxed = run_stdin_child(true); + os_sandbox::reset_for_test(); + assert_stdin_is_detached(&sandboxed); + println!( + "issue667-stdin: BLOCKED (both the isolated and unisolated launches detach a terminal stdin)" + ); + } else { + println!( + "issue667-stdin: BLOCKED (the unisolated launch detaches a terminal stdin; the sandbox backend is unavailable)" + ); + } +} + +/// A child that reports whether fd 0 is a terminal and whether a read returns +/// EOF at once. It ignores `SIGTTIN` and sets fd 0 non-blocking so a terminal +/// stdin (the pre-fix behavior) reports `READ_ERR` instead of stopping. +#[cfg(any(target_os = "linux", target_os = "macos"))] +const STDIN_PROBE_SCRIPT: &str = r#"import fcntl, os, signal, sys +try: + signal.signal(signal.SIGTTIN, signal.SIG_IGN) +except (ValueError, OSError): + pass +sys.stdout.write('ISATTY %d\n' % (1 if os.isatty(0) else 0)) +sys.stdout.flush() +try: + flags = fcntl.fcntl(0, fcntl.F_GETFL) + fcntl.fcntl(0, fcntl.F_SETFL, flags | os.O_NONBLOCK) +except OSError: + pass +try: + sys.stdout.write('READ %d\n' % len(os.read(0, 1))) +except OSError as error: + sys.stdout.write('READ_ERR %d\n' % error.errno) +"#; + +/// Run a child through `bounded_process` with standard input left unset, so it +/// inherits the probe's pty, and return its stdout. The wrapped form goes through +/// the installed sandbox. Standard I/O is set after wrapping because the macOS +/// backend rewrites the command. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn run_stdin_child(wrapped: bool) -> String { + let mut command = Command::new("python3"); + command.arg("-c").arg(STDIN_PROBE_SCRIPT); + let mut command = if wrapped { + os_sandbox::wrap_command(command).expect("available sandbox wraps") + } else { + command + }; + command.stdout(Stdio::piped()).stderr(Stdio::piped()); + let output = + bounded_process::run_with_timeout(&mut command, TIMEOUT).expect("run the stdin child"); + String::from_utf8_lossy(&output.stdout).into_owned() +} + +/// Assert the child reports a non-terminal stdin that reads EOF at once. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn assert_stdin_is_detached(output: &str) { + assert!( + output.contains("ISATTY 0"), + "the child's stdin must not be a terminal: {output:?}" + ); + assert!( + output.contains("READ 0"), + "a detached stdin must read EOF immediately: {output:?}" + ); +} + +/// Runs [`issue667_stdin_probe`] in a fresh process and asserts it reaches a +/// verdict. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[test] +fn isolated_child_stdin_is_detached_from_the_terminal() { + let exe = std::env::current_exe().expect("current exe"); + let output = Command::new(exe) + .args([ + "--exact", + "issue667_stdin_probe", + "--nocapture", + "--test-threads=1", + ]) + .env("COMMANDAGENT_ISSUE667_STDIN_PROBE", "1") + .output() + .expect("run the stdin probe as a subprocess"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "the stdin probe failed:\n{stdout}\n{stderr}" + ); + let verdict = stdout + .lines() + .find(|line| line.contains("issue667-stdin:")) + .unwrap_or_else(|| panic!("the stdin probe reported no verdict:\n{stdout}\n{stderr}")); + eprintln!("{verdict}"); +} + /// Issue #667: a confined command keeps its own terminal (its inherited stdio) /// but cannot open another terminal's slave for writing. Both ptys are created /// by the test — the command's own terminal is one and the "other terminal" is From 569e85def80df00db04f17f9ce17f4004bb5b0df Mon Sep 17 00:00:00 2001 From: kewton Date: Wed, 14 Oct 2026 20:35:26 +0900 Subject: [PATCH 3/7] test(tools): detect a leaked /dev/null descriptor for a terminal stdin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #667 follow-up. To replace a terminal stdin the launcher opens `/dev/null` in the forked child and duplicates it onto fd 0, then closes the descriptor it opened. No test pinned that `close`, so mutating the guard `if devnull != 0` to `devnull == 0` left the descriptor open without failing anything, and the exec'd command inherited an extra `/dev/null` descriptor. - Tests: a probe that makes its own pty the controlling terminal and fd 0, then starts a child with stdin left unset through `bounded_process` in both the isolated and unisolated launches. The child reports every descriptor above the standard three whose device is `/dev/null`, and the probe asserts that list is empty. A positive control opens an extra `/dev/null` in the child first, so the scan is shown to see a retained descriptor. - The probe runs in a fresh process, like the existing stdin probe, and only ever uses the pty the probe created. 判断: only the test file changes; the product guard in `bounded_process` is unchanged. The `devnull == 0` mutation now fails the new test (`DEVNULL_FDS 9`), while the unmutated build passes it. Co-authored-by: CommandCodeBot --- tests/issue502_os_sandbox_write.rs | 160 +++++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) diff --git a/tests/issue502_os_sandbox_write.rs b/tests/issue502_os_sandbox_write.rs index 6b0649e7..4df51032 100644 --- a/tests/issue502_os_sandbox_write.rs +++ b/tests/issue502_os_sandbox_write.rs @@ -1646,6 +1646,166 @@ fn isolated_child_stdin_is_detached_from_the_terminal() { eprintln!("{verdict}"); } +/// Issue #667: to replace a terminal stdin the launcher opens `/dev/null` in the +/// freshly forked child and duplicates it onto fd 0. The descriptor it opened +/// must be closed before `exec`, otherwise the exec'd command inherits an extra +/// descriptor pointing at `/dev/null`. The probe makes its own pty the session's +/// controlling terminal and puts it on fd 0, then starts a child with stdin left +/// unset so it would inherit that terminal, and the child reports every +/// descriptor above the standard three whose device is `/dev/null`. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[test] +fn issue667_devnull_fd_probe() { + if std::env::var_os("COMMANDAGENT_ISSUE667_DEVNULL_FD_PROBE").is_none() { + return; + } + let (_master, slave, _slave_path) = open_pty(); + assert_ne!( + unsafe { libc::setsid() }, + -1, + "setsid: {}", + std::io::Error::last_os_error() + ); + assert_eq!( + unsafe { libc::ioctl(slave.as_raw_fd(), TIOCSCTTY_REQUEST, 0) }, + 0, + "TIOCSCTTY: {}", + std::io::Error::last_os_error() + ); + unsafe { + libc::signal(libc::SIGHUP, libc::SIG_IGN); + } + // Make the pty the probe's own standard input, so a child that leaves stdin + // unset inherits a terminal and the launcher opens `/dev/null` for it. + assert_ne!( + unsafe { libc::dup2(slave.as_raw_fd(), 0) }, + -1, + "dup2: {}", + std::io::Error::last_os_error() + ); + + // Positive control: the scanner must report a `/dev/null` descriptor the + // child itself opens, so a clean scan below is evidence rather than a blind + // spot. + let leaked = run_devnull_fd_child(false, true); + assert!( + leaked.contains("DEVNULL_FDS") && !leaked.contains("DEVNULL_FDS none"), + "the scanner must detect an extra /dev/null descriptor: {leaked:?}" + ); + + // The unisolated launch. + assert_no_devnull_descriptor(&run_devnull_fd_child(false, false)); + + // The isolated launch, when the backend is available. + if matches!(os_sandbox::probe(), SandboxAvailability::Available) { + let dir = tempfile::tempdir().expect("tempdir"); + let workspace = dir.path().join("ws"); + let run_tmp = dir.path().join("tmp"); + std::fs::create_dir_all(&workspace).unwrap(); + std::fs::create_dir_all(&run_tmp).unwrap(); + os_sandbox::install_for_test( + SandboxMode::Auto, + SandboxPolicy::new(&workspace, &run_tmp), + SandboxAvailability::Available, + ); + let sandboxed = run_devnull_fd_child(true, false); + os_sandbox::reset_for_test(); + assert_no_devnull_descriptor(&sandboxed); + println!( + "issue667-devnull-fd: BLOCKED (both the isolated and unisolated launches close the /dev/null descriptor)" + ); + } else { + println!( + "issue667-devnull-fd: BLOCKED (the unisolated launch closes the /dev/null descriptor; the sandbox backend is unavailable)" + ); + } +} + +/// A child that reports every descriptor above the standard three whose device +/// matches `/dev/null`, so a descriptor the launcher left open is visible from +/// inside the exec'd program. `leak` additionally opens `/dev/null` in the child +/// as a positive control that the scan can see a retained descriptor. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn devnull_fd_script(leak: bool) -> String { + let leak_line = if leak { + "os.open('/dev/null', os.O_RDONLY)\n" + } else { + "" + }; + format!( + r#"import os, stat, sys +{leak_line}null_rdev = os.stat('/dev/null').st_rdev +hits = [] +for fd in range(3, 256): + try: + info = os.fstat(fd) + except OSError: + continue + if stat.S_ISCHR(info.st_mode) and info.st_rdev == null_rdev: + hits.append(fd) +sys.stdout.write('DEVNULL_FDS %s\n' % (','.join(map(str, hits)) if hits else 'none')) +"# + ) +} + +/// Run a child through `bounded_process` with standard input left unset, so it +/// inherits the probe's pty, and return its stdout. The wrapped form goes through +/// the installed sandbox. Standard I/O is set after wrapping because the macOS +/// backend rewrites the command. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn run_devnull_fd_child(wrapped: bool, leak: bool) -> String { + let mut command = Command::new("python3"); + command.arg("-c").arg(devnull_fd_script(leak)); + let mut command = if wrapped { + os_sandbox::wrap_command(command).expect("available sandbox wraps") + } else { + command + }; + command.stdout(Stdio::piped()).stderr(Stdio::piped()); + let output = + bounded_process::run_with_timeout(&mut command, TIMEOUT).expect("run the devnull-fd child"); + String::from_utf8_lossy(&output.stdout).into_owned() +} + +/// Assert the child reports no descriptor above the standard three whose device +/// is `/dev/null`. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn assert_no_devnull_descriptor(output: &str) { + assert!( + output.contains("DEVNULL_FDS none"), + "no descriptor above the standard three may point at /dev/null: {output:?}" + ); +} + +/// Runs [`issue667_devnull_fd_probe`] in a fresh process and asserts it reaches a +/// verdict. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[test] +fn isolated_child_closes_the_devnull_descriptor_for_a_terminal_stdin() { + let exe = std::env::current_exe().expect("current exe"); + let output = Command::new(exe) + .args([ + "--exact", + "issue667_devnull_fd_probe", + "--nocapture", + "--test-threads=1", + ]) + .env("COMMANDAGENT_ISSUE667_DEVNULL_FD_PROBE", "1") + .output() + .expect("run the devnull-fd probe as a subprocess"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "the devnull-fd probe failed:\n{stdout}\n{stderr}" + ); + let verdict = stdout + .lines() + .find(|line| line.contains("issue667-devnull-fd:")) + .unwrap_or_else(|| panic!("the devnull-fd probe reported no verdict:\n{stdout}\n{stderr}")); + eprintln!("{verdict}"); +} + /// Issue #667: a confined command keeps its own terminal (its inherited stdio) /// but cannot open another terminal's slave for writing. Both ptys are created /// by the test — the command's own terminal is one and the "other terminal" is From 0e0cada1f5c047fd041e51813b92f2c41f3db13f Mon Sep 17 00:00:00 2001 From: kewton Date: Wed, 14 Oct 2026 21:48:12 +0900 Subject: [PATCH 4/7] fix(tools): deny reading the run terminal from inside the sandbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #667 (H-32). `setsid()` closes `TIOCSTI`, but a confined child can still re-open the run's terminal by path (`/dev/pts/N`, `/dev/ttys*`, `/dev/tty`) with `O_RDONLY|O_NOCTTY` and read the keys the user types. Base refused that read only through job control (`SIGTTIN`/`EIO`), which `setsid()` removed, so `SECURITY.md` claimed the read was impossible. Keep `setsid()` (it is what closes `TIOCSTI`) and close the path instead. - `seatbelt`: re-deny `^/dev/tty` for `file-read*` after the terminal write re-deny, so a confined command cannot re-open a terminal slave for reading; `/dev/null`, `/dev/urandom`, `/dev/ptmx`, and the rest of `/dev` are unchanged, and the already-open standard descriptors are not re-checked. - `landlock`: add `/dev/pts`, `/dev/tty`, and every existing `/dev/tty*` and `/dev/console` to the read-deny list, so the expansion leaves them (and their subtrees) out of the read allow list. `/dev` is an ancestor of a denied location, so it is granted `ReadDir` alone through a new `list_only` rule: the listing keeps working (`ls /dev`) while `ReadFile` on the terminal children stays denied. The read layer still needs only ABI1. - Tests: a pty probe that makes its own pty the controlling terminal and fd 0, writes one typed line to the master, and starts a child through `bounded_process` that re-opens the slave by path and reads it; `reopen` and `setsid_reopen` must fail (`ERR`) inside the isolation and must read outside it as the positive control, with `UNVERIFIED` when the backend is unavailable. Unit tests pin the seatbelt rule and its order and the landlock read allow. - `SECURITY.md`: a confined command cannot open a terminal device by path for reading or writing; a terminal stdin becomes `/dev/null`; an inherited stdout/stderr descriptor is not re-checked, but the confined launches set both to pipes; an unconfined launch is not a boundary. 判断: the read deny is by path on the device, so it closes the keystroke read without touching the inherited stdio; the product's confined launches pipe stdout/stderr, so no confined command holds a terminal descriptor. 判断: landlock keeps `/dev` listable with a `ReadDir`-only grant instead of allowing the whole tree, so the terminal children stay unreadable, and `read_isolation_gaps` keeps reporting the same meaning. Co-authored-by: CommandCodeBot --- SECURITY.md | 40 ++++-- src/tools/os_sandbox/landlock.rs | 147 +++++++++++++++++++- src/tools/os_sandbox/seatbelt.rs | 30 ++++- tests/issue502_os_sandbox_write.rs | 208 +++++++++++++++++++++++++++++ 4 files changed, 402 insertions(+), 23 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index cc0075a0..ecddea9e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -93,23 +93,35 @@ command before the shell starts. controlling terminal (`setsid()` at spawn), so it cannot inject keystrokes into the terminal the run was started from with `TIOCSTI` — the kernel only allows `TIOCSTI` on the caller's controlling terminal, and the child no longer has - one. It also cannot write to another terminal's slave: on Linux `/dev/pts` is - not writable, and on macOS the profile re-denies `/dev/tty*` last. The visible - change is to a program that opens the terminal *directly*: an interactive pager - waiting for keys on `/dev/tty` now sees no controlling terminal. A terminal - standard input is replaced with `/dev/null`, so a command cannot read the keys - the user types during a run — it could not before either, because the child sat - in a background process group of the session's terminal and a read was refused - with `SIGTTIN` (`EIO` once that signal is ignored); the change makes that - explicit instead of relying on the job-control check. A standard output or - error that is the terminal is unchanged: the command keeps writing its - inherited descriptors, so the non-interactive `git` pager path (output - captured, pager writes to stdout) is unaffected. + one. It also cannot name a terminal device by path from inside the isolation: + on Linux `/dev/pts` and `/dev/tty` (plus every existing `/dev/tty*` and + `/dev/console`) are outside both the write allow list and the read allow list, + and on macOS the profile re-denies `/dev/tty*` for writes and reads last, so a + command can neither write to a terminal's slave nor re-open the run's terminal + by path and read the keys the user types. `/dev/null`, `/dev/zero`, + `/dev/urandom`, `/dev/ptmx`, and the rest of `/dev` stay usable. A terminal + standard input is replaced with `/dev/null`, so the command's standard input is + non-readable; the pre-#667 job-control check had left it non-readable only by + accident (the child sat in a background process group of the session's + terminal, and a read was refused with `SIGTTIN`, or `EIO` once that signal is + ignored). The visible change is to a program that opens the terminal + *directly*: an interactive pager waiting for keys on `/dev/tty` now sees no + controlling terminal. + Two cases are outside this boundary. A standard output or error that is already + the terminal keeps working from its inherited descriptor, which the read deny + does not re-check, so a confined command that inherited a terminal on + stdout/stderr could still read it; the confined launches set both to pipes, so + none inherits a terminal descriptor. And a command launched without the + isolation is not a boundary at all: it runs with the user's own privilege and, + because the launcher `setsid()`s every child, it can re-open the run's terminal + by path even though job control would otherwise have refused the read. Every standard input is either set to `null` or a pipe, or inherited; an inherited terminal stdin is the only case the `/dev/null` replacement changes. The confined launches are the model Bash and its `bash:verify` interval, the - verifier/acceptance commands, and the runtime dependency setup. The - non-confined launches are the workspace `git` state check, the `lsof` port + verifier/acceptance commands, and the runtime dependency setup; each sets its + `stdout` and `stderr` to pipes (`src/tools/bash.rs`, + `src/minimal_loop/verifier_env.rs`, `src/minimal_loop/dependency_setup.rs`). + The non-confined launches are the workspace `git` state check, the `lsof` port lookups, the fetch, interaction, browser, pipeline, CLI-argv, and declarative-check probes, the community `esbuild` bundle, the dev-server start, and the backend availability probes. All of them run non-interactive commands diff --git a/src/tools/os_sandbox/landlock.rs b/src/tools/os_sandbox/landlock.rs index dccf9ab3..5b3515ce 100644 --- a/src/tools/os_sandbox/landlock.rs +++ b/src/tools/os_sandbox/landlock.rs @@ -45,6 +45,10 @@ //! terminal's slave; the child's own session (a `setsid()` in //! `bounded_process`) has no controlling terminal, so `TIOCSTI` is refused by //! the kernel even where Landlock cannot filter device ioctls (below ABI5). +//! The same locations are removed from the read allow list +//! ([`terminal_read_deny_paths`]), so a command cannot re-open the run's +//! terminal by path and read the keys the user types; `/dev` stays listable +//! through a `ReadDir`-only rule that never grants `ReadFile`. use std::collections::HashSet; use std::os::fd::{AsRawFd, OwnedFd}; @@ -239,13 +243,18 @@ fn probe_command() -> Command { } /// Create a ruleset that denies every handled read and write access outside -/// `writable` (read+write+execute) and `read_only` (read+execute), returning its -/// file descriptor for the child to enforce. +/// `writable` (read+write+execute), `read_only` (read+execute), and `list_only` +/// (read-dir only), returning its file descriptor for the child to enforce. /// /// `handle_access` uses `from_all(abi)` so both the read layer and the write /// layer are enforced; `Execute | ReadFile | ReadDir` all exist from ABI1, so /// this adds no ABI requirement over the write layer. /// +/// `list_only` grants `ReadDir` alone, never `ReadFile`, so a directory that +/// [`expand_read_allow`] had to recurse into (an ancestor of a denied location, +/// such as `/dev` once the terminal devices are denied) stays listable while the +/// denied subtree beneath it stays unreadable. +/// /// For an `--offline` run with ABI4 (Issue #658), `AccessNet::ConnectTcp` is also /// handled, which denies every TCP connect, and each `dev_server_ports` entry is /// re-allowed with a [`NetPort`] rule. Before ABI4 Landlock cannot restrict the @@ -255,6 +264,7 @@ fn create_ruleset( abi: u32, writable: impl IntoIterator, read_only: impl IntoIterator, + list_only: impl IntoIterator, offline: bool, dev_server_ports: &[u16], ) -> Result { @@ -286,6 +296,9 @@ fn create_ruleset( let created = created .add_rules(path_beneath_rules(read_only, read_access)) .map_err(|err| format!("landlock read rule failed: {err}"))?; + let created = created + .add_rules(path_beneath_rules(list_only, AccessFs::ReadDir)) + .map_err(|err| format!("landlock list-only rule failed: {err}"))?; let created = if restrict_network { add_network_ports(created, dev_server_ports)? } else { @@ -414,6 +427,7 @@ pub fn wrap(command: &mut Command, policy: &SandboxPolicy) -> Result<(), String> detected_abi(), allowed_paths(policy), read_allow_paths(policy), + list_only_paths(), policy.offline, &policy.dev_server_ports, )?; @@ -458,15 +472,58 @@ pub(crate) fn allowed_paths(policy: &SandboxPolicy) -> Vec { paths } -/// The read-only locations: the expansion of the home secret deny list into its +/// The read-only locations: the expansion of the read-deny list into its /// complement (see [`expand_read_allow`]). Without this, handling the read /// rights would make every location unreadable. `home_secret_locations` /// canonicalizes each location, so a symlinked secret location (`~/.ssh` that -/// points outside the home) denies its target too. +/// points outside the home) denies its target too; the terminal devices of +/// [`terminal_read_deny_paths`] deny the run's terminal like any other denied +/// location. fn read_allow_paths(policy: &SandboxPolicy) -> Vec { - expand_read_allow(&super::secret_locations::home_secret_locations( - &policy.home, - )) + let mut denied = super::secret_locations::home_secret_locations(&policy.home); + denied.extend(terminal_read_deny_paths()); + expand_read_allow(&denied) +} + +/// The terminal device locations removed from the read allow list (Issue #667). +/// +/// A bounded child `setsid()`s, so it has no controlling terminal, but it can +/// still re-open the run's terminal by path and read the keys the user types: +/// `/dev/pts/N` on Linux (the run's terminal is always a pty slave) and +/// `/dev/tty` (the controlling-terminal alias). Landlock cannot deny part of an +/// allowed tree, so these locations join the read-deny list and +/// [`expand_read_allow`] leaves them and their subtrees out. +/// +/// The physical console and serial terminals (`/dev/console`, `/dev/tty0`, +/// `/dev/ttyS*`, …) are the other terminals a child could name, so every +/// existing `/dev` entry whose name is `console` or starts with `tty` is denied +/// too; only the entries that exist are listed, and denying them costs nothing a +/// confined command needs. `/dev/ptmx` and the null/zero/random devices are not +/// terminal paths and stay readable. +fn terminal_read_deny_paths() -> Vec { + let mut paths = vec![PathBuf::from("/dev/pts"), PathBuf::from("/dev/tty")]; + if let Ok(entries) = std::fs::read_dir("/dev") { + for entry in entries.flatten() { + let name = entry.file_name(); + let name = name.to_string_lossy(); + if name == "console" || name.starts_with("tty") { + paths.push(entry.path()); + } + } + } + paths.sort(); + paths.dedup(); + paths +} + +/// The directories granted `ReadDir` alone. [`expand_read_allow`] recurses into +/// every ancestor of a denied location without allowing the ancestor itself, so +/// listing such a directory would fail once the terminal devices are denied. +/// `/dev` is enumerated by tools (`ls /dev`), so it is listed here; the grant is +/// `ReadDir` only, so it never re-opens the terminal devices beneath it. +fn list_only_paths() -> Vec { + let dev = PathBuf::from("/dev"); + if dev.is_dir() { vec![dev] } else { Vec::new() } } /// The home secret locations whose read denial a writable location defeats, so @@ -833,6 +890,82 @@ mod tests { ); } + /// Issue #667: the terminal devices are removed from the read allow list, + /// while the devices a confined command needs and their non-terminal siblings + /// stay readable, and `/dev` itself is not allowed wholesale (it is an + /// ancestor of a denied location). + #[test] + fn read_allow_excludes_terminal_devices_and_keeps_the_required_ones() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + let dev = root.join("dev"); + std::fs::create_dir_all(dev.join("pts")).unwrap(); + std::fs::create_dir_all(dev.join("shm")).unwrap(); + for file in [ + "tty", "tty0", "console", "null", "zero", "urandom", "random", "ptmx", + ] { + std::fs::write(dev.join(file), "").unwrap(); + } + let denied = vec![ + dev.join("pts"), + dev.join("tty"), + dev.join("tty0"), + dev.join("console"), + ]; + + let allow = expand_read_allow_from(root, &denied); + for kept in ["null", "zero", "urandom", "random", "ptmx", "shm"] { + assert!( + allow.contains(&dev.join(kept)), + "{kept} must stay readable: {allow:?}" + ); + } + for gone in ["pts", "tty", "tty0", "console"] { + let path = dev.join(gone); + assert!( + !allow.contains(&path), + "{path:?} must stay unreadable: {allow:?}" + ); + assert!( + allow.iter().all(|entry| !entry.starts_with(&path)), + "no rule may reach below {path:?}: {allow:?}" + ); + } + assert!( + !allow.contains(&dev), + "/dev is an ancestor of a denied location and must be recursed, not allowed: {allow:?}" + ); + } + + /// Issue #667: the read-deny list names the pty-slave directory and the + /// controlling-terminal alias, so `read_allow_paths` leaves the run's + /// terminal out. + #[test] + fn terminal_read_deny_paths_names_the_pty_directory_and_the_controlling_tty() { + let denied = terminal_read_deny_paths(); + assert!(denied.contains(&PathBuf::from("/dev/pts")), "{denied:?}"); + assert!(denied.contains(&PathBuf::from("/dev/tty")), "{denied:?}"); + } + + /// Issue #667: `/dev` stays listable through the `ReadDir`-only rule even + /// though a denied location sits below it. + #[test] + fn list_only_paths_keeps_dev_listable() { + assert_eq!(list_only_paths(), vec![PathBuf::from("/dev")]); + } + + /// Issue #657: the read layer needs no ABI beyond ABI1, so removing the + /// terminal devices from the read allow list adds no new minimum. `Execute`, + /// `ReadFile`, and `ReadDir` are all part of `from_read` from ABI1, and + /// `ReadDir` (the `list_only` grant) is therefore always enforceable. + #[test] + fn read_access_is_available_from_abi1() { + let read = AccessFs::from_read(ABI::from(1)); + assert!(read.contains(AccessFs::Execute), "{read:?}"); + assert!(read.contains(AccessFs::ReadFile), "{read:?}"); + assert!(read.contains(AccessFs::ReadDir), "{read:?}"); + } + #[test] fn limitation_note_reports_the_abi2_and_abi3_gaps() { let abi1 = limitation_for("6.1.0", 1).expect("ABI 1 has gaps"); diff --git a/src/tools/os_sandbox/seatbelt.rs b/src/tools/os_sandbox/seatbelt.rs index a9b31095..445e02a9 100644 --- a/src/tools/os_sandbox/seatbelt.rs +++ b/src/tools/os_sandbox/seatbelt.rs @@ -16,8 +16,10 @@ //! //! Terminal isolation (Issue #667): the write profile re-denies `/dev/tty*` //! last, so a confined command cannot write to a terminal (its own or another -//! user's slave), and the child's own session (a `setsid()` in -//! `bounded_process`) has no controlling terminal, so `TIOCSTI` is refused. +//! user's slave), and the same pattern is re-denied for reads, so it cannot +//! re-open the run's terminal by path and read the keys the user types; the +//! child's own session (a `setsid()` in `bounded_process`) has no controlling +//! terminal, so `TIOCSTI` is refused. use std::path::Path; use std::process::{Command, Stdio}; @@ -41,6 +43,13 @@ const UNSET_PATH: &str = "/commandagent-sandbox-unset"; /// last (Seatbelt applies the last matching rule), so a confined command can /// still use `/dev/null` and friends but cannot write to a terminal — its own or /// another user's slave (`/dev/ttys*`) — see Issue #667. +/// +/// The same `/dev/tty*` pattern is re-denied for reads. A confined command runs +/// in its own session with no controlling terminal, but it can still re-open the +/// run's terminal by path (`/dev/ttys*`) and read the keys the user types; the +/// read re-deny closes that path, while the already-open standard descriptors are +/// untouched. `/dev/null`, `/dev/urandom`, `/dev/ptmx`, and the rest of `/dev` +/// are unaffected. const WRITE_PROFILE: &str = r#" (version 1) (allow default) @@ -55,6 +64,7 @@ const WRITE_PROFILE: &str = r#" (allow file-write* (literal (param "CACHE_CARGO_PACKAGE"))) (allow file-write* (regex #"^/dev/")) (deny file-write* (regex #"^/dev/tty")) +(deny file-read* (regex #"^/dev/tty")) "#; /// The `--offline` network rules (Issue #658). `(deny network*)` blocks every @@ -312,6 +322,22 @@ mod tests { ); } + /// Issue #667: the same `/dev/tty*` pattern is re-denied for reads, after the + /// `(allow default)`, so a confined command cannot re-open a terminal by path + /// and read the keys the user types. + #[test] + fn profile_denies_terminal_reads() { + let built = profile(0, false); + let allow_default = built.find("(allow default)").expect("allow default rule"); + let deny_read = built + .find("(deny file-read* (regex #\"^/dev/tty\"))") + .expect("terminal read deny rule"); + assert!( + allow_default < deny_read, + "the terminal read deny must follow (allow default): {built}" + ); + } + #[test] fn probe_classifies_a_zero_exit_as_available() { assert_eq!( diff --git a/tests/issue502_os_sandbox_write.rs b/tests/issue502_os_sandbox_write.rs index 4df51032..4420d051 100644 --- a/tests/issue502_os_sandbox_write.rs +++ b/tests/issue502_os_sandbox_write.rs @@ -1806,6 +1806,214 @@ fn isolated_child_closes_the_devnull_descriptor_for_a_terminal_stdin() { eprintln!("{verdict}"); } +/// Issue #667: a confined command runs in its own session with no controlling +/// terminal, but it can still re-open the run's terminal by path (`/dev/pts/N` +/// on Linux, `/dev/ttys*` on macOS) and read the keys the user types. The read +/// layer must refuse that open. The probe makes its own pty the session's +/// controlling terminal, writes one line to the master as the "user", then +/// starts a child through `bounded_process` that re-opens the slave by path and +/// reads it. `reopen` opens it directly; `setsid_reopen` forks a grandchild that +/// `setsid()`s first, which is the hole that also existed before this change. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[test] +fn issue667_terminal_read_probe() { + if std::env::var_os("COMMANDAGENT_ISSUE667_TERMINAL_READ_PROBE").is_none() { + return; + } + let (mut master, slave, slave_path) = open_pty(); + assert_ne!( + unsafe { libc::setsid() }, + -1, + "setsid: {}", + std::io::Error::last_os_error() + ); + assert_eq!( + unsafe { libc::ioctl(slave.as_raw_fd(), TIOCSCTTY_REQUEST, 0) }, + 0, + "TIOCSCTTY: {}", + std::io::Error::last_os_error() + ); + unsafe { + libc::signal(libc::SIGHUP, libc::SIG_IGN); + } + + // Positive control: without the sandbox the child re-opens the run's + // terminal by path and reads the line, so a denial below is the sandbox. + for mode in ["reopen", "setsid_reopen"] { + let output = run_terminal_read_child(&mut master, &slave, &slave_path, false, mode); + assert!( + output.contains(&format!("RESULT {mode} READ")) + && output.contains(&terminal_read_token(mode, false)), + "the unisolated child must read the run's terminal by path ({mode}): {output:?}" + ); + } + + // The isolated launch, when the backend is available. + if matches!(os_sandbox::probe(), SandboxAvailability::Available) { + let dir = tempfile::tempdir().expect("tempdir"); + let workspace = dir.path().join("ws"); + let run_tmp = dir.path().join("tmp"); + std::fs::create_dir_all(&workspace).unwrap(); + std::fs::create_dir_all(&run_tmp).unwrap(); + os_sandbox::install_for_test( + SandboxMode::Auto, + SandboxPolicy::new(&workspace, &run_tmp), + SandboxAvailability::Available, + ); + for mode in ["reopen", "setsid_reopen"] { + let output = run_terminal_read_child(&mut master, &slave, &slave_path, true, mode); + assert!( + output.contains(&format!("RESULT {mode} ERR")), + "the isolated child must not open the run's terminal by path ({mode}): {output:?}" + ); + assert!( + !output.contains(&terminal_read_token(mode, true)), + "no typed key may reach the isolated child ({mode}): {output:?}" + ); + } + os_sandbox::reset_for_test(); + record_terminal_read_step_summary("blocked"); + println!( + "issue667-terminal-read: BLOCKED (the isolated child could not re-open the terminal by path; the unisolated control read it)" + ); + } else { + record_terminal_read_step_summary("unverified: the sandbox backend is unavailable"); + println!( + "issue667-terminal-read: UNVERIFIED (the sandbox backend is unavailable; only the unisolated control was measured)" + ); + } +} + +/// A Python child that re-opens the pty at `path` by name and reads it: `reopen` +/// opens it directly, `setsid_reopen` forks a grandchild that `setsid()`s first. +/// It reports `RESULT READ `, `RESULT NOTHING`, or `RESULT +/// ERR `, so the caller can tell a read from a denied open. +#[cfg(any(target_os = "linux", target_os = "macos"))] +const TERMINAL_READ_SCRIPT: &str = r#"import os, select, signal, sys +signal.signal(signal.SIGTTIN, signal.SIG_IGN) +mode, path = sys.argv[1], sys.argv[2] +if mode == 'setsid_reopen': + pid = os.fork() + if pid: + os.waitpid(pid, 0) + sys.exit(0) + os.setsid() +try: + fd = os.open(path, os.O_RDONLY | os.O_NOCTTY | os.O_NONBLOCK) + ready, _, _ = select.select([fd], [], [], 2.0) + if ready: + sys.stdout.write('RESULT %s READ %r\n' % (mode, os.read(fd, 128))) + else: + sys.stdout.write('RESULT %s NOTHING\n' % mode) +except OSError as error: + sys.stdout.write('RESULT %s ERR %d\n' % (mode, error.errno)) +sys.stdout.flush() +"#; + +/// Run a child that re-opens `slave_path` after `master` receives one typed +/// line, optionally through the installed sandbox, and return its output. The +/// queued input is flushed afterwards so the next mode starts clean. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn run_terminal_read_child( + master: &mut std::fs::File, + slave: &std::fs::File, + slave_path: &Path, + sandbox: bool, + mode: &str, +) -> String { + use std::io::Write; + + let mut command = Command::new("python3"); + command + .arg("-c") + .arg(TERMINAL_READ_SCRIPT) + .arg(mode) + .arg(slave_path); + let mut command = if sandbox { + os_sandbox::wrap_command(command).expect("available sandbox wraps") + } else { + command + }; + // Standard I/O is set after wrapping because the macOS backend rewrites the + // command (same as `run_stdin_child`). + command + .stdin(dup_stdio(slave)) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + // The "user" types one line on the run's terminal while the child runs. + master + .write_all(format!("{}\n", terminal_read_token(mode, sandbox)).as_bytes()) + .expect("write the typed line"); + master.flush().expect("flush the typed line"); + let output = bounded_process::run_with_timeout(&mut command, TIMEOUT) + .expect("run the terminal-read child"); + // Drop anything the child left queued so a previous mode's line is not read + // by the next one. + unsafe { + libc::tcflush(slave.as_raw_fd(), libc::TCIFLUSH); + } + format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ) +} + +/// The one line the "user" types for a mode, so the caller can tell whether the +/// child actually received it. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn terminal_read_token(mode: &str, sandbox: bool) -> String { + format!("KEY_{mode}_{sandbox}") +} + +/// Append the terminal-read verdict to `$GITHUB_STEP_SUMMARY` when it is set, so +/// a run that could not measure the denial records why instead of passing +/// silently. +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn record_terminal_read_step_summary(status: &str) { + let Some(path) = std::env::var_os("GITHUB_STEP_SUMMARY") else { + return; + }; + let line = format!("issue667/terminal-read: {status}\n"); + let _ = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(path) + .and_then(|mut file| std::io::Write::write_all(&mut file, line.as_bytes())); +} + +/// Runs [`issue667_terminal_read_probe`] in a fresh process and asserts it +/// reaches a verdict. A run without the sandbox backend reports `UNVERIFIED` +/// and records a `$GITHUB_STEP_SUMMARY` line, so it is never a silent pass. +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[test] +fn isolated_child_cannot_read_the_terminal_by_path() { + let exe = std::env::current_exe().expect("current exe"); + let output = Command::new(exe) + .args([ + "--exact", + "issue667_terminal_read_probe", + "--nocapture", + "--test-threads=1", + ]) + .env("COMMANDAGENT_ISSUE667_TERMINAL_READ_PROBE", "1") + .output() + .expect("run the terminal-read probe as a subprocess"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "the terminal-read probe failed:\n{stdout}\n{stderr}" + ); + let verdict = stdout + .lines() + .find(|line| line.contains("issue667-terminal-read:")) + .unwrap_or_else(|| { + panic!("the terminal-read probe reported no verdict:\n{stdout}\n{stderr}") + }); + eprintln!("{verdict}"); +} + /// Issue #667: a confined command keeps its own terminal (its inherited stdio) /// but cannot open another terminal's slave for writing. Both ptys are created /// by the test — the command's own terminal is one and the "other terminal" is From 2b3223b248bd3472b09a3f3153164b0d498967c9 Mon Sep 17 00:00:00 2001 From: kewton Date: Wed, 14 Oct 2026 22:49:53 +0900 Subject: [PATCH 5/7] test(tools): pin the terminal device selection in the landlock read deny MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #667 follow-up. `terminal_read_deny_paths` selects the terminal devices with `name == "console" || name.starts_with("tty")`, but no unit test pinned the selection, so the mutants `||` to `&&` and `==` to `!=` were not detected; both widen the read allow list and make the physical console and serial terminals readable inside the isolation. - Split the reader into `terminal_read_deny_paths_in(dev: &Path)`, which builds the hardcoded `pts` and `tty` entries relative to `dev` and enumerates `dev`, so a test can drive it against a tempdir; `terminal_read_deny_paths()` now returns `terminal_read_deny_paths_in(Path::new("/dev"))` and is unchanged for the real `/dev`. - Tests: a tempdir with `pts/`, `shm/`, and `tty`, `console`, `tty0`, `ttyS0`, `ttyUSB0`, `null`, `urandom`, `ptmx` must yield exactly the six terminal entries, so the `||`/`&&` and `==`/`!=` mutants change the set and fail it. The existing `terminal_read_deny_paths_names_the_pty_directory_and_the_controlling_tty` test stays. 判断: only the read-deny selection is refactored; `terminal_read_deny_paths()` returns the same list for `/dev`, so the product behavior is unchanged. Co-authored-by: CommandCodeBot --- src/tools/os_sandbox/landlock.rs | 58 ++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/src/tools/os_sandbox/landlock.rs b/src/tools/os_sandbox/landlock.rs index 5b3515ce..bf889e57 100644 --- a/src/tools/os_sandbox/landlock.rs +++ b/src/tools/os_sandbox/landlock.rs @@ -485,24 +485,25 @@ fn read_allow_paths(policy: &SandboxPolicy) -> Vec { expand_read_allow(&denied) } -/// The terminal device locations removed from the read allow list (Issue #667). +/// The terminal device locations removed from the read allow list (Issue #667), +/// for the given device directory (normally `/dev`). /// /// A bounded child `setsid()`s, so it has no controlling terminal, but it can /// still re-open the run's terminal by path and read the keys the user types: -/// `/dev/pts/N` on Linux (the run's terminal is always a pty slave) and -/// `/dev/tty` (the controlling-terminal alias). Landlock cannot deny part of an +/// `/pts/N` on Linux (the run's terminal is always a pty slave) and +/// `/tty` (the controlling-terminal alias). Landlock cannot deny part of an /// allowed tree, so these locations join the read-deny list and /// [`expand_read_allow`] leaves them and their subtrees out. /// -/// The physical console and serial terminals (`/dev/console`, `/dev/tty0`, -/// `/dev/ttyS*`, …) are the other terminals a child could name, so every -/// existing `/dev` entry whose name is `console` or starts with `tty` is denied -/// too; only the entries that exist are listed, and denying them costs nothing a -/// confined command needs. `/dev/ptmx` and the null/zero/random devices are not +/// The physical console and serial terminals (`/console`, `/tty0`, +/// `/ttyS*`, …) are the other terminals a child could name, so every +/// existing entry whose name is `console` or starts with `tty` is denied too; +/// only the entries that exist are listed, and denying them costs nothing a +/// confined command needs. `/ptmx` and the null/zero/random devices are not /// terminal paths and stay readable. -fn terminal_read_deny_paths() -> Vec { - let mut paths = vec![PathBuf::from("/dev/pts"), PathBuf::from("/dev/tty")]; - if let Ok(entries) = std::fs::read_dir("/dev") { +fn terminal_read_deny_paths_in(dev: &Path) -> Vec { + let mut paths = vec![dev.join("pts"), dev.join("tty")]; + if let Ok(entries) = std::fs::read_dir(dev) { for entry in entries.flatten() { let name = entry.file_name(); let name = name.to_string_lossy(); @@ -516,6 +517,11 @@ fn terminal_read_deny_paths() -> Vec { paths } +/// The terminal device locations for the real `/dev`. +fn terminal_read_deny_paths() -> Vec { + terminal_read_deny_paths_in(Path::new("/dev")) +} + /// The directories granted `ReadDir` alone. [`expand_read_allow`] recurses into /// every ancestor of a denied location without allowing the ancestor itself, so /// listing such a directory would fail once the terminal devices are denied. @@ -947,6 +953,36 @@ mod tests { assert!(denied.contains(&PathBuf::from("/dev/tty")), "{denied:?}"); } + /// Issue #667: given a device directory, the deny list is exactly the + /// pty-slave directory and every terminal device name — `console`, `tty`, + /// `tty0`, `ttyS0`, `ttyUSB0` — and never a non-terminal entry (`null`, + /// `urandom`, `ptmx`) or a non-terminal directory (`shm`). A mutation that + /// widens or narrows the name test (`||` to `&&`, `==` to `!=`) changes this + /// set, so the check pins both. + #[test] + fn terminal_read_deny_paths_in_selects_exactly_the_terminal_devices() { + let dir = tempfile::tempdir().unwrap(); + let dev = dir.path(); + std::fs::create_dir_all(dev.join("pts")).unwrap(); + std::fs::create_dir_all(dev.join("shm")).unwrap(); + for file in [ + "tty", "console", "tty0", "ttyS0", "ttyUSB0", "null", "urandom", "ptmx", + ] { + std::fs::write(dev.join(file), "").unwrap(); + } + + let denied = terminal_read_deny_paths_in(dev); + let expected = vec![ + dev.join("console"), + dev.join("pts"), + dev.join("tty"), + dev.join("tty0"), + dev.join("ttyS0"), + dev.join("ttyUSB0"), + ]; + assert_eq!(denied, expected); + } + /// Issue #667: `/dev` stays listable through the `ReadDir`-only rule even /// though a denied location sits below it. #[test] From 56c67689252db17fcf6f255f0eeeca5eaca5faa6 Mon Sep 17 00:00:00 2001 From: kewton Date: Wed, 14 Oct 2026 23:26:16 +0900 Subject: [PATCH 6/7] docs(tools): correct the pty allocation claim for the sandbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #667 follow-up (H-33). The comments and SECURITY.md said `/dev/ptmx` stays usable "so allocating a pty still works". Inside the isolation `/dev/ptmx` is openable but the pty slave it creates is refused (Linux: `/dev/pts` is neither readable nor writable; macOS: `/dev/tty*` is denied), so `openpty`, `script`, `expect`, and `pexpect` cannot use a pty. That pty loss is the cost of closing terminal reads and writes; it worked before #667. - `landlock.rs`: correct the `DEVICE_PATHS` comment; the `/dev/pts` denial already documents why the slave is refused. - `seatbelt.rs`: correct the `WRITE_PROFILE` comment the same way. - `SECURITY.md`: state that `/dev/ptmx` is openable but the pty slave is refused, so a confined command cannot allocate a pty, and that this is a #667 cost. 判断: comments and one documentation sentence only; no code line or test changes, so the behavior is unchanged. Co-authored-by: CommandCodeBot --- SECURITY.md | 7 ++++++- src/tools/os_sandbox/landlock.rs | 6 +++++- src/tools/os_sandbox/seatbelt.rs | 8 ++++++-- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index ecddea9e..ae994616 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -99,7 +99,12 @@ command before the shell starts. and on macOS the profile re-denies `/dev/tty*` for writes and reads last, so a command can neither write to a terminal's slave nor re-open the run's terminal by path and read the keys the user types. `/dev/null`, `/dev/zero`, - `/dev/urandom`, `/dev/ptmx`, and the rest of `/dev` stay usable. A terminal + `/dev/urandom`, and the rest of `/dev` stay usable. `/dev/ptmx` is still + openable, but that does not give a usable pty: the slave it would create + (`/dev/pts/*` on Linux, `/dev/ttys*` on macOS) is refused, so a confined + command cannot allocate a pty (`openpty`, `script`, `expect`, `pexpect` fail). + That pty loss is the cost of closing terminal reads and writes (Issue #667); + allocating a pty worked before #667. A terminal standard input is replaced with `/dev/null`, so the command's standard input is non-readable; the pre-#667 job-control check had left it non-readable only by accident (the child sat in a background process group of the session's diff --git a/src/tools/os_sandbox/landlock.rs b/src/tools/os_sandbox/landlock.rs index bf889e57..0bf89f4d 100644 --- a/src/tools/os_sandbox/landlock.rs +++ b/src/tools/os_sandbox/landlock.rs @@ -80,7 +80,11 @@ const PROBE_TIMEOUT: Duration = Duration::from_secs(10); /// write to another terminal's slave. Landlock grants the handled `IoctlDev` /// access (ABI5) only where a rule covers the location, so leaving these two out /// also closes device ioctls on them — including `TIOCSTI` — not just writes. -/// `/dev/ptmx` stays allowed so allocating a pty still works. +/// `/dev/ptmx` stays allowed, but that does not give a usable pty: the slave it +/// would create (`/dev/pts/*`) is not allowed either, so `openpty`, `script`, +/// `expect`, and `pexpect` fail inside the isolation. That pty loss is the cost +/// of closing terminal reads and writes (Issue #667); allocating a pty worked +/// before #667. const DEVICE_PATHS: &[&str] = &[ "/dev/null", "/dev/zero", diff --git a/src/tools/os_sandbox/seatbelt.rs b/src/tools/os_sandbox/seatbelt.rs index 445e02a9..c3462186 100644 --- a/src/tools/os_sandbox/seatbelt.rs +++ b/src/tools/os_sandbox/seatbelt.rs @@ -48,8 +48,12 @@ const UNSET_PATH: &str = "/commandagent-sandbox-unset"; /// in its own session with no controlling terminal, but it can still re-open the /// run's terminal by path (`/dev/ttys*`) and read the keys the user types; the /// read re-deny closes that path, while the already-open standard descriptors are -/// untouched. `/dev/null`, `/dev/urandom`, `/dev/ptmx`, and the rest of `/dev` -/// are unaffected. +/// untouched. `/dev/null`, `/dev/urandom`, and the rest of `/dev` are unaffected, +/// and `/dev/ptmx` is still openable — but that does not give a usable pty: the +/// slave it would create (`/dev/ttys*`) is denied, so `openpty`, `script`, +/// `expect`, and `pexpect` cannot use a pty inside the isolation. That pty loss +/// is the cost of closing terminal reads and writes (Issue #667); allocating a +/// pty worked before #667. const WRITE_PROFILE: &str = r#" (version 1) (allow default) From 4648597797661868d98e59a28a050f6cfa58268e Mon Sep 17 00:00:00 2001 From: kewton Date: Wed, 14 Oct 2026 23:47:43 +0900 Subject: [PATCH 7/7] test(tools): print the TIOCSTI unverified verdict on stdout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #667 (PR #674 CI). On a runner with `legacy_tiocsti=0` the positive control cannot inject, so the probe correctly reports UNVERIFIED — but `record_unverified` wrote it with `eprintln!`, and the outer test `isolated_child_cannot_inject_into_the_controlling_terminal` looks for the verdict only on stdout, so it failed as if no verdict were reported. The stdin, devnull-fd, and terminal-read probes already print their UNVERIFIED verdict on stdout. - Print the verdict with `println!`; the `$GITHUB_STEP_SUMMARY` line is still recorded, so an unmeasurable environment is still not a silent pass. 判断: the verdict line only moves from stderr to stdout; no expectation or other line changes. Co-authored-by: CommandCodeBot --- tests/issue502_os_sandbox_write.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/issue502_os_sandbox_write.rs b/tests/issue502_os_sandbox_write.rs index 4420d051..832b6db6 100644 --- a/tests/issue502_os_sandbox_write.rs +++ b/tests/issue502_os_sandbox_write.rs @@ -1440,10 +1440,12 @@ fn run_tiocsti_attempt(slave: &std::fs::File, inject: char, wrapped: bool) -> St } } -/// Report an unmeasurable `TIOCSTI` probe without passing silently. +/// Report an unmeasurable `TIOCSTI` probe on stdout, like the other probes, so +/// the outer test can find the verdict instead of reading it as missing; the +/// `$GITHUB_STEP_SUMMARY` line still records it, so it is not a silent pass. #[cfg(any(target_os = "linux", target_os = "macos"))] fn record_unverified(reason: &str) { - eprintln!("issue667-tiocsti: UNVERIFIED ({reason})"); + println!("issue667-tiocsti: UNVERIFIED ({reason})"); record_tiocsti_step_summary(&format!("unverified: {reason}")); }