diff --git a/crates/openjd-sessions/src/cross_user_helper.rs b/crates/openjd-sessions/src/cross_user_helper.rs index c4b9b959..98acf980 100644 --- a/crates/openjd-sessions/src/cross_user_helper.rs +++ b/crates/openjd-sessions/src/cross_user_helper.rs @@ -272,7 +272,18 @@ impl CrossUserHelper { user: &dyn SessionUser, ) -> Result<(Self, std::fs::File), SessionError> { let auth_token = generate_auth_token()?; - let mut child = std::process::Command::new("sudo") + // Resolved from a fixed list of trusted directories rather than through + // PATH. Note the job does not influence the PATH used here -- see the + // system_commands module docs for why this is hardening rather than the fix + // for a reachable hole. Bound once and reused below so the error message + // names the same binary that was actually launched. + let sudo = crate::system_commands::system_command_path("sudo").map_err(|source| { + SessionError::SubprocessStart { + command: "sudo".to_string(), + source, + } + })?; + let mut child = std::process::Command::new(&sudo) .args([ "-u", user.user(), @@ -288,7 +299,8 @@ impl CrossUserHelper { .map_err(|source| SessionError::SubprocessStart { // Don't include the token in error messages. command: format!( - "sudo -u {} -i {} --auth-token ", + "{} -u {} -i {} --auth-token ", + sudo.display(), user.user(), helper_path.display() ), diff --git a/crates/openjd-sessions/src/lib.rs b/crates/openjd-sessions/src/lib.rs index 6faf7bcb..e0d18b05 100644 --- a/crates/openjd-sessions/src/lib.rs +++ b/crates/openjd-sessions/src/lib.rs @@ -19,6 +19,10 @@ pub mod runner; pub mod session; pub mod session_user; pub(crate) mod subprocess; +// Unix-only: its callers (the `sudo` invocations) are themselves `cfg(unix)`, so +// compiling it on Windows would only produce dead-code warnings. +#[cfg(unix)] +pub(crate) mod system_commands; pub mod tempdir; #[cfg(windows)] pub mod win32; diff --git a/crates/openjd-sessions/src/session.rs b/crates/openjd-sessions/src/session.rs index 16c1d328..f6a64aca 100644 --- a/crates/openjd-sessions/src/session.rs +++ b/crates/openjd-sessions/src/session.rs @@ -149,6 +149,42 @@ fn format_exit_code(code: Option) -> String { } } +/// Build the argv passed to `sudo` to delete session files as the job user. +/// +/// Note what is *not* here: `-i`. Per sudo(8), `-i` does not exec the command, it +/// concatenates the argv into one string and hands it to the target user's login +/// shell via `-c`, "escaping each character (including white space) with a +/// backslash except for alphanumerics, underscores, hyphens, and dollar signs". +/// +/// Dollar signs are the problem. These paths come from `read_dir` of the session +/// working directory, so they are filenames the job chose. A job that creates a file +/// named `$HOME` gets that name through unescaped, the login shell expands it to a +/// path that does not exist, and `rm -rf` on a nonexistent path is a no-op that +/// exits 0. The caller's `status.success()` check would then report a clean cleanup +/// while the file remained, owned by the job user, and the `remove_dir_all` fallback +/// cannot remove it -- a silently leaked session directory, reachable by the job +/// rather than by host misconfiguration. +/// +/// Without `-i` sudo execs `rm` directly, so no shell re-parses these strings and +/// the argv is the argv. `rm` needs no login environment, and dropping the login +/// shell also means the resolved absolute path is the one that runs: with `-i` the +/// shell resolved it again in the job user's own context, which need not agree with +/// the agent's. +#[cfg(unix)] +fn cross_user_cleanup_args(user: &str, rm: &std::path::Path, files: Vec) -> Vec { + let mut args = vec![ + "-u".to_string(), + user.to_string(), + rm.to_string_lossy().to_string(), + "-rf".to_string(), + // Everything after this is a job-chosen filename, so it must not be read as + // an option however it is spelled. + "--".to_string(), + ]; + args.extend(files); + args +} + /// Normalize an environment variable name for the current platform. /// On Windows, env vars are case-insensitive, so we uppercase all keys /// to avoid undefined behavior from mixed-case duplicates in the Win32 API. @@ -1032,26 +1068,128 @@ impl Session { if let Some(ref user) = self.cross_user.user { if !user.is_process_user() { if let Ok(entries) = std::fs::read_dir(&self.working_directory) { + // The helpers directory is excluded, and without this the + // status check below is a guaranteed false positive on every + // cross-user session. + // + // `create_helpers_dir` chowns `.helpers-` to the job + // user's group and sets 0o750, so the group gets r-x and no w + // -- deliberately, so the job user can traverse and read the + // helper but not modify it. `rm -rf` therefore cannot unlink + // the binary inside it, cannot rmdir a non-empty directory, + // and exits nonzero; `-f` suppresses missing operands, not + // EACCES. The `remove_dir_all` fallback below removes it + // without trouble, because the process user owns it. + // + // Leaving it in the operand list made the warning fire on + // every session, routed to the worker agent log, naming a + // leak that had not happened and blaming job-user ownership + // for a file the process user owns. A warning that always + // fires is one operators filter, which costs exactly the + // signal this check exists to add. + let helpers_dir = self.cross_user.helpers_dir.clone(); let files: Vec = entries .filter_map(|e| e.ok()) - .map(|e| e.path().to_string_lossy().to_string()) + .map(|e| e.path()) + .filter(|path| helpers_dir.as_deref() != Some(path.as_path())) + .map(|path| path.to_string_lossy().to_string()) .collect(); + // Both resolved from trusted directories rather than + // through PATH. If either is missing we skip this + // best-effort removal and fall through to the TempDir + // cleanup below, rather than falling back to a bare + // name -- a bare name here is the defect this resolver + // exists to remove. + // + // `rm` runs as the job user, so qualifying it is + // hardening rather than a privilege boundary: it stops + // the removal depending on that user's own login-shell + // PATH. + // Looked up separately rather than zipped, so the warning can + // name the one that is actually missing. `sudo` absent and + // `rm` absent are different host problems with different + // fixes. + let sudo = crate::system_commands::find_system_command("sudo"); + let rm = crate::system_commands::find_system_command("rm"); if !files.is_empty() { - let mut args = vec![ - "-u".to_string(), - user.user().to_string(), - "-i".to_string(), - "rm".to_string(), - "-rf".to_string(), - "--".to_string(), - ]; - args.extend(files); - let _ = std::process::Command::new("sudo") + let missing: Vec<&str> = [("sudo", &sudo), ("rm", &rm)] + .iter() + .filter(|(_, found)| found.is_none()) + .map(|(name, _)| *name) + .collect(); + if !missing.is_empty() { + // Skipping silently would be the worst outcome here. + // The fallback below is std::fs::remove_dir_all under + // a `let _ =`, which cannot remove files owned by the + // job user, so the session directory leaks with job + // data still in it and nothing explains why. + // + // PROCESS_CONTROL as well as FILE_PATH: per + // logging.rs, only EXCEPTION_INFO, PROCESS_CONTROL and + // HOST_INFO reach the worker log, so a FILE_PATH-only + // record would go to the session stream alone and the + // operator who needs this would never see it. + session_log!( + warn, + &self.session_id, + LogContent::FILE_PATH | LogContent::PROCESS_CONTROL, + "Could not locate {} in a trusted directory; \ + skipping cross-user cleanup of {} file(s) in {}. \ + Files owned by the job user may remain.", + // " and ", not " or ": when both are absent the + // code knows both are, and "sudo or rm" reads as + // uncertainty -- an operator would install sudo, + // retry, and hit the same warning. A one-element + // join never emits the separator, so the + // single-missing message is unchanged. + missing.join(" and "), + files.len(), + self.working_directory.display() + ); + } + } + if let (false, (Some(sudo), Some(rm))) = (files.is_empty(), (sudo, rm)) { + let args = cross_user_cleanup_args(user.user(), &rm, files); + // The status is inspected rather than discarded. Both + // binaries resolving is the common case -- `sudo` was + // already resolved once at helper start -- so the + // failures an operator actually hits happen here, not + // in the branch above: a sudoers rule that does not + // permit this command, or `rm` refusing a path. + // Discarding the status left every one of those silent, + // and the fallback below cannot remove job-user-owned + // files, so the directory leaked with no explanation on + // the record. + // + // The check means what it says only because the argv + // above carries no `-i`; see cross_user_cleanup_args for + // why a login shell would make exit 0 unreliable here. + match std::process::Command::new(&sudo) .args(&args) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) - .status(); + .status() + { + Ok(status) if status.success() => {} + Ok(status) => session_log!( + warn, + &self.session_id, + LogContent::FILE_PATH | LogContent::PROCESS_CONTROL, + "Cross-user cleanup of {} exited {}; files owned by \ + the job user may remain.", + self.working_directory.display(), + status + ), + Err(e) => session_log!( + warn, + &self.session_id, + LogContent::FILE_PATH | LogContent::PROCESS_CONTROL, + "Could not run cross-user cleanup of {}: {e}; files \ + owned by the job user may remain.", + self.working_directory.display() + ), + } } } } @@ -3020,6 +3158,69 @@ impl Drop for Session { } } +#[cfg(all(test, unix))] +mod cross_user_cleanup_tests { + //! Unit tests for the cross-user cleanup argv. + use super::*; + use std::path::Path; + + #[test] + fn does_not_interpose_a_login_shell() { + // The load-bearing assertion. With `-i`, sudo hands the concatenated argv to + // the job user's login shell via `-c`, escaping everything except + // alphanumerics, underscores, hyphens and DOLLAR SIGNS (sudo(8)). These paths + // are job-chosen filenames, so a file named `$HOME` would be expanded by that + // shell into a path that does not exist, and `rm -rf` on a nonexistent path + // exits 0 -- making the caller's status.success() check report a clean + // cleanup over a file that is still there. + let args = cross_user_cleanup_args( + "job-user", + Path::new("/trusted/rm"), + vec!["/sessions/abc/$HOME".to_string()], + ); + + assert!( + !args.iter().any(|a| a == "-i"), + "argv must not request a login shell: {args:?}" + ); + } + + #[test] + fn passes_the_resolved_rm_and_terminates_options_before_filenames() { + // The negative control for the assertion above: the argv must still be a + // usable one, or "no -i" would be satisfied by an argv that does nothing. + let args = cross_user_cleanup_args( + "job-user", + Path::new("/trusted/rm"), + vec![ + "/sessions/abc/-rf".to_string(), + "/sessions/abc/b".to_string(), + ], + ); + + assert_eq!( + args, + vec![ + "-u", + "job-user", + "/trusted/rm", + "-rf", + "--", + "/sessions/abc/-rf", + "/sessions/abc/b", + ] + ); + // `--` must precede every filename, so a job-chosen name that looks like an + // option is still treated as a path. + let end_of_options = args.iter().position(|a| a == "--").expect("-- is present"); + let first_file = args + .iter() + .position(|a| a.starts_with("/sessions/")) + .expect("a file is present"); + assert!(end_of_options < first_file); + } +} + #[cfg(test)] mod wrap_actions_tests { //! Unit tests for the small pure helpers that back RFC 0008 wrap-hook diff --git a/crates/openjd-sessions/src/system_commands.rs b/crates/openjd-sessions/src/system_commands.rs new file mode 100644 index 00000000..8a2c5b30 --- /dev/null +++ b/crates/openjd-sessions/src/system_commands.rs @@ -0,0 +1,417 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright by contributors to this project. +// SPDX-License-Identifier: (Apache-2.0 OR MIT) + +//! Resolution of system command names to absolute paths, without consulting `PATH`. +//! +//! The problem: `Command::new("sudo")` resolves a bare name through a `PATH`, +//! which makes correctness depend on whose `PATH` that is. Be precise about what +//! that means here, because this crate's exposure is narrower than the general +//! shape of the problem suggests. +//! +//! Two reasons, worth separating because only the first is about the environment. +//! `CrossUserHelper::spawn` sets no environment on its `Command`, so the helper +//! inherits the session process's own and `Command::new` resolves against the +//! parent's `PATH`. And `sudo` is spawned once when the helper starts, before any +//! job action exists; per-action environments travel to the helper over its stdin +//! protocol afterwards, so they cannot reach the argv that located `sudo`. +//! +//! Note it is *not* true that job environments are only ever applied after an +//! `env_clear()`. That holds on the same-user path (`subprocess.rs`), but the +//! helper's own runner layers the action's environment onto whatever it inherited, +//! with no `env_clear()` anywhere under `src/helper/`. The conclusion above stands +//! on ordering and channel, not on clearing. +//! +//! Resolving here is therefore about removing assumptions rather than closing a +//! reachable hole: +//! +//! * The bare name is safe only while no caller sets an environment on the helper's +//! `Command`. Nothing states or enforces that, so adding `.env()` or `.envs()` +//! later would change the security of a line nobody edited. +//! * It assumes the session process's own `PATH` is trustworthy, which is a +//! property of how the agent is launched, not of this crate. +//! * It keeps this crate and `openjd-sessions-for-python` answering the same +//! question the same way, so an auditor does not have to derive the difference +//! between them to audit either one. +//! +//! The solution: names are resolved here, by scanning a fixed list of trusted +//! absolute directories. +//! +//! Three properties make that work, and all three are easy to undo by accident: +//! +//! * `PATH` is never read. Not directly, and not through a `which` crate or +//! `command -v`, which resolve via `PATH` and so would restore the original +//! behaviour while looking like a fix. +//! * Only paths under [`TRUSTED_SYSTEM_DIRECTORIES`] are returned. A name +//! containing a path separator is rejected, because joining `"/usr/bin"` with +//! `"../../tmp/evil"` would otherwise escape the directory being searched. +//! * A missing command is an error, never a fallback to the bare name. Falling +//! back would put resolution on `PATH` again while the code still read as though +//! it did not. + +use std::io; +use std::path::{Path, PathBuf}; + +/// Absolute directories searched for system commands, in order. +/// +/// The order matters in exactly one place, and it is not against `/usr/bin`. On +/// NixOS `/usr/bin` holds only `env`, so there is no `/usr/bin/sudo` for the wrapper +/// directory to beat. The real competitor is `/run/current-system/sw/bin`, which also +/// holds a `sudo` -- a non-setuid one, because nix store paths cannot carry the +/// setuid bit. Only `/run/wrappers/bin/sudo` can elevate, so the wrapper directory +/// must precede the system profile; that pairing is what +/// `setuid_wrapper_precedes_the_nixos_system_profile` pins. On every other platform +/// both `/run` entries are absent and cost one `stat` each. +/// +/// `sbin` entries are last: on non-usr-merged distributions some commands exist +/// only under `/sbin`. +pub(crate) const TRUSTED_SYSTEM_DIRECTORIES: &[&str] = &[ + "/run/wrappers/bin", + // Paired with the entry above. /run/wrappers/bin holds only the setuid/setcap + // wrappers, so on NixOS it resolves `sudo` and nothing else: /usr/bin holds just + // `env`, /bin just `sh`, and the sbin directories are absent. `rm` lives in this + // symlink farm, which nixos-rebuild manages and root owns, making it + // trust-equivalent to /usr/bin there. Without it the ordering above would + // resolve `sudo` and then skip the cross-user cleanup for want of `rm`. + "/run/current-system/sw/bin", + "/usr/bin", + "/bin", + // FreeBSD and the other BSDs install sudo from ports into /usr/local/bin and + // have no /usr/bin/sudo at all, so omitting this made cross-user sessions + // impossible to start there -- a regression from the bare `Command::new("sudo")` + // this module replaced, which the login PATH would have resolved. + "/usr/local/bin", + "/usr/local/sbin", + "/usr/sbin", + "/sbin", +]; + +/// True if `name` is a bare command name, with no path component. +fn is_bare_command_name(name: &str) -> bool { + !name.is_empty() && name != "." && name != ".." && !name.contains('/') && !name.contains('\\') +} + +/// True if `path` is a regular file with at least one execute bit set. +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = std::fs::metadata(path) else { + return false; + }; + if !metadata.is_file() { + return false; + } + // No cfg branch here. This module is declared `#[cfg(unix)]` in lib.rs, so a + // non-unix build never compiles it. A `cfg(not(unix))` arm returning `true` + // would be dead code giving the opposite answer, which a reader would have to + // rule out before trusting the executable check. + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 +} + +/// Resolve `name` within an explicit list of directories. +/// +/// Separated from [`find_system_command`] so that tests can supply a directory +/// they control; production callers should use the wrappers below. +pub(crate) fn find_system_command_in(name: &str, directories: &[&str]) -> Option { + if !is_bare_command_name(name) { + return None; + } + directories + .iter() + .map(|directory| Path::new(directory).join(name)) + .find(|candidate| is_executable_file(candidate)) +} + +/// Resolve `name` to an absolute path, or `None` if it is not installed. +/// +/// Use this when the command's absence is tolerable; use [`system_command_path`] +/// when it is required. +pub(crate) fn find_system_command(name: &str) -> Option { + find_system_command_in(name, TRUSTED_SYSTEM_DIRECTORIES) +} + +/// Resolve `name` to an absolute path, or fail. +/// +/// Returns [`io::ErrorKind::NotFound`] so that callers can fold this into the +/// same error they already report for a process that could not be started. +pub(crate) fn system_command_path(name: &str) -> io::Result { + find_system_command(name).ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + format!( + "could not find the system command {name:?} in any trusted directory ({}); \ + PATH is deliberately not searched", + TRUSTED_SYSTEM_DIRECTORIES.join(", ") + ), + ) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + + /// Create a directory containing an executable file named `name`. + fn dir_with_executable(name: &str) -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join(name); + fs::write(&path, "#!/bin/sh\ntrue\n").expect("write"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).expect("chmod"); + } + dir + } + + #[test] + fn resolves_a_command_that_exists_in_a_searched_directory() { + // The negative control for the tests below: resolution must actually + // work, or "nothing was found" results prove nothing. + let dir = dir_with_executable("openjd-test-cmd"); + let dir_s = dir.path().to_str().expect("utf8"); + + let found = find_system_command_in("openjd-test-cmd", &[dir_s]); + + assert_eq!(found, Some(dir.path().join("openjd-test-cmd"))); + } + + #[test] + fn does_not_resolve_a_command_outside_the_searched_directories() { + // Pins "only the listed directories are searched". A `which`-style + // implementation fails this: the command is not on PATH, so it would + // return None where this must return the path. + let dir = dir_with_executable("openjd-test-cmd"); + + let found = find_system_command_in("openjd-test-cmd", &["/usr/bin", "/bin"]); + + assert_eq!(found, None, "resolved a command from an unlisted directory"); + // ...and the same name IS found once its directory is listed, so the + // assertion above is about the directory list and not about the file. + let dir_s = dir.path().to_str().expect("utf8"); + assert!(find_system_command_in("openjd-test-cmd", &[dir_s]).is_some()); + } + + /// This module's own source, minus its tests, for + /// [`production_source_contains_no_environment_lookup`]. + fn production_source() -> &'static str { + let source = include_str!("system_commands.rs"); + // Split off the test module, whose own assertions mention the very tokens + // being searched for. Without this the test fails on itself. + source + .split_once("#[cfg(test)]") + .map(|(production, _tests)| production) + .expect("this file contains a #[cfg(test)] module") + } + + #[test] + fn production_source_contains_no_environment_lookup() { + // A source lint, not a behavioural pin, and worth being honest about which. + // + // It catches the mutation that matters in practice -- adding a PATH fallback + // for names missing from the trusted list -- because such a fallback has to + // read the environment to work, and the behavioural test below cannot see it + // (a command absent from the trusted directories is also absent from PATH in + // the test, so both return None either way). + // + // What it does not catch: a `which` crate, or `Command::new(bare_name)` + // letting execvp resolve in the child. Neither reads the environment in this + // process. Those are visible in review as a new dependency or a changed + // spawn, which is the level they belong at. + // + // Asserted over the source because the obvious alternative is worse. An + // earlier revision set PATH and observed the result, which mutates + // process-global state while other tests in this binary call + // `std::env::vars()` concurrently (`subprocess.rs`). That is a data race, + // the reason the function is `unsafe` from edition 2024, and it risked + // flaking unrelated tests that spawn bare commands. + let production = production_source(); + + assert!( + !production.contains("std::env"), + "system_commands must not read process environment state; PATH resolution \ + is exactly what this module exists to avoid" + ); + assert!( + !production.contains("var_os") && !production.contains("env::var"), + "environment lookup found in production source" + ); + } + + #[test] + fn a_command_present_only_outside_the_trusted_list_is_not_found() { + // The behavioural companion to the assertion above: a real executable that + // exists, is executable, and is simply not in a searched directory must not + // resolve by any route. + let dir = dir_with_executable("openjd-test-path-cmd"); + assert!( + dir.path().join("openjd-test-path-cmd").exists(), + "precondition: the executable really exists" + ); + + assert_eq!(find_system_command("openjd-test-path-cmd"), None); + } + + #[test] + fn rejects_names_containing_a_path_component() { + // Pins the guard that stops this module becoming the injection point: + // joining a trusted directory with "../.." escapes it entirely. + let dir = dir_with_executable("openjd-test-cmd"); + let parent = dir.path().to_str().expect("utf8"); + + for name in ["../openjd-test-cmd", "a/b", "a\\b", "", ".", ".."] { + assert_eq!( + find_system_command_in(name, &[parent]), + None, + "accepted {name:?} as a bare command name" + ); + } + } + + #[test] + fn escaping_name_is_rejected_even_though_the_target_exists() { + // The traversal guard has to be about the name, not about whether the + // resolved file happens to exist -- so prove the target is reachable by + // the same join before asserting the name is refused. + let dir = dir_with_executable("openjd-test-cmd"); + let nested = dir.path().join("nested"); + fs::create_dir(&nested).expect("mkdir"); + let nested_s = nested.to_str().expect("utf8"); + + assert!( + dir.path().join("openjd-test-cmd").exists(), + "precondition: the traversal target exists" + ); + assert_eq!( + find_system_command_in("../openjd-test-cmd", &[nested_s]), + None + ); + } + + #[test] + fn ignores_a_non_executable_file() { + let dir = tempfile::tempdir().expect("temp dir"); + fs::write(dir.path().join("openjd-test-cmd"), "not executable").expect("write"); + let dir_s = dir.path().to_str().expect("utf8"); + + assert_eq!(find_system_command_in("openjd-test-cmd", &[dir_s]), None); + } + + #[test] + fn missing_command_is_an_error_and_not_the_bare_name() { + // The silent-fallback failure mode: returning Ok("sudo") here would look + // fixed and behave exactly as the vulnerability did. + let error = system_command_path("openjd-definitely-not-installed") + .expect_err("missing command must not resolve"); + + assert_eq!(error.kind(), io::ErrorKind::NotFound); + let message = error.to_string(); + assert!( + message.contains("openjd-definitely-not-installed"), + "message did not name the command: {message}" + ); + assert!( + message.contains("PATH is deliberately not searched"), + "message did not explain why PATH was not used: {message}" + ); + } + + #[test] + fn every_trusted_directory_is_absolute() { + // A relative entry would be resolved against the process working + // directory, which a session changes. + for directory in TRUSTED_SYSTEM_DIRECTORIES { + assert!( + Path::new(directory).is_absolute(), + "{directory} is not absolute" + ); + } + } + + #[test] + fn the_list_resolves_real_commands_on_this_host() { + // The positive control the shape assertions cannot provide. + // + // Every other test here either patches the directory list or asserts that a + // literal appears in the literal declared above, so all of them pass on a + // list whose entries are misspelled, or empty, or point somewhere that does + // not exist. This one fails in that case, because it asks the real + // filesystem for commands the crate actually resolves. + // + // `sh` rather than `sudo` as the required one: `sudo` is absent from minimal + // containers, and a test that skips on the host it is meant to protect is + // not a control. + let sh = find_system_command("sh").expect("sh must resolve from the trusted list"); + assert!(sh.is_absolute()); + assert!( + TRUSTED_SYSTEM_DIRECTORIES + .iter() + .any(|d| sh.starts_with(Path::new(d))), + "{} is not under any trusted directory", + sh.display() + ); + + // rm is resolved by session cleanup, so its absence is a real breakage + // rather than a curiosity. + assert!( + find_system_command("rm").is_some(), + "rm must resolve: session cleanup uses it" + ); + } + + #[test] + fn platform_specific_directories_are_not_silently_dropped() { + // These entries exist for hosts CI does not run on, so nothing else in this + // file would notice their removal. Kept deliberately narrow: each assertion + // names the platform and the command that needs it, so a future reader can + // judge whether the entry is still earning its place rather than treating + // the list as untouchable. + for (directory, why) in [ + ("/run/wrappers/bin", "NixOS: the only setuid sudo"), + // rm only. This crate resolves exactly two commands, sudo and rm: + // setsid is called in-process via nix::libc::setsid() from pre_exec, and + // pgrep is not used here at all. Naming commands the crate never + // resolves would undercut the point of the assertion. + ("/run/current-system/sw/bin", "NixOS: rm"), + ("/usr/local/bin", "FreeBSD and other BSDs: sudo from ports"), + ] { + assert!( + TRUSTED_SYSTEM_DIRECTORIES.contains(&directory), + "{directory} was dropped, needed for {why}" + ); + } + } + + #[test] + fn setuid_wrapper_precedes_the_nixos_system_profile() { + // The ordering that decides which `sudo` NixOS resolves. + // + // Not /usr/bin. On NixOS /usr/bin holds only `env`, so there is no + // /usr/bin/sudo for the wrapper to beat and an assertion against it holds + // vacuously -- it would stay green under a reordering that breaks the + // invariant it is named for. + // + // The real competitor is /run/current-system/sw/bin. With the default + // security.sudo.enable, both exist: a mode 0755 store symlink in the system + // profile that is *not* setuid, and the setuid wrapper. Only the wrapper + // works. If the profile came first, resolution would succeed and the spawn + // would then fail with "sudo: must be owned by uid 0 and have the setuid bit + // set" -- worse than not finding it, because system_command_path never gets + // to produce its explanatory error. CI runs Linux and macOS, so nothing else + // here would catch it. + let wrapper = TRUSTED_SYSTEM_DIRECTORIES + .iter() + .position(|d| *d == "/run/wrappers/bin") + .expect("/run/wrappers/bin is searched"); + let profile = TRUSTED_SYSTEM_DIRECTORIES + .iter() + .position(|d| *d == "/run/current-system/sw/bin") + .expect("/run/current-system/sw/bin is searched"); + + assert!( + wrapper < profile, + "the setuid wrapper must precede the NixOS system profile, which also \ + contains a non-setuid sudo" + ); + } +} diff --git a/specs/sessions/architecture.md b/specs/sessions/architecture.md index 948dcf6e..bacfc373 100644 --- a/specs/sessions/architecture.md +++ b/specs/sessions/architecture.md @@ -42,6 +42,9 @@ src/ ├── embedded_files.rs # Two-phase file materialization ├── let_bindings.rs # Re-exports evaluate_let_bindings from openjd_model ├── session_user.rs # SessionUser trait, PosixSessionUser +├── system_commands.rs # Unix only. Resolves system command names (sudo, rm) +│ # against a fixed list of trusted absolute +│ # directories, never via PATH. See cross-user.md. ├── tempdir.rs # Secure temp directory creation ├── logging.rs # LogContent bitflags, session_log! macro, banners └── error.rs # SessionError enum diff --git a/specs/sessions/cross-user.md b/specs/sessions/cross-user.md index 01c3f7fd..57f96348 100644 --- a/specs/sessions/cross-user.md +++ b/specs/sessions/cross-user.md @@ -60,12 +60,40 @@ cross-user machinery is bypassed — no sudo, no chown, no shell script wrapper. When `SubprocessConfig.user` is set and `!user.is_process_user()`, subprocess launch routes through the embedded cross-user helper (see `embedded-cross-user-helper.md`). The helper process is spawned once per session -via `sudo -u -i /path/to/helper --auth-token ` and persists for +via ` -u -i /path/to/helper --auth-token ` and persists for the session lifetime. The `` value is a random 22-character ASCII string (128 bits of entropy, drawn character-by-character from a 64-char URL-safe alphabet using the OS CSPRNG) generated by the session and echoed in every command sent over the helper's stdin; the helper rejects any command -whose `"token"` field does not match. See the +whose `"token"` field does not match. + +`` above is written as a placeholder deliberately: it is an absolute path +resolved by `system_commands.rs` from a fixed list of trusted absolute +directories, never located through `PATH`. Session cleanup resolves `rm` the same +way, and skips the cross-user removal with a warning if either binary is absent +rather than falling back to a bare name. + +A bare command name resolves through whatever `PATH` the spawning process holds, +so it is worth stating why that is not a live exploit here, rather than leaving an +auditor to re-derive it. + +Two separate reasons. `CrossUserHelper::spawn` sets no environment, so the helper +inherits the session process's own and `Command::new` resolves against the +parent's `PATH`. And `sudo` runs once at helper startup, before any job action +exists; per-action environments arrive later over the helper's stdin protocol, so +they cannot influence the argv that located `sudo`. + +It is *not* the case that job environments are only applied after an +`env_clear()`. That holds on the same-user path in `subprocess.rs`, but the +helper's runner layers the action environment onto whatever it inherited, and no +`env_clear()` exists under `src/helper/`. The conclusion rests on ordering and +channel, not on clearing. + +Resolving from trusted directories is therefore hardening in this crate rather +than the fix for a live exploit: it removes an unstated invariant -- that no +caller ever adds an environment to that `Command`, and that the agent's own +`PATH` is trustworthy -- which an edit to an unrelated line could violate +silently. See the [Authentication Token](embedded-cross-user-helper.md#authentication-token) section of the helper spec for rationale and verification details. diff --git a/specs/sessions/logging.md b/specs/sessions/logging.md index 70c2b8dd..87948902 100644 --- a/specs/sessions/logging.md +++ b/specs/sessions/logging.md @@ -103,6 +103,7 @@ to provide visual structure in log output. | `session.rs` | `HOST_INFO` | Version, platform, architecture at init | | `session.rs` | `FILE_PATH` | Working directory, files directory paths | | `session.rs` | `BANNER` | Section banners for enter/exit/run/cleanup | +| `session.rs` | `FILE_PATH \| PROCESS_CONTROL` | Cross-user cleanup failures: command not found, nonzero exit, spawn error | | `subprocess.rs` | `PROCESS_CONTROL` | PID start, SIGTERM, SIGKILL, exit code, spawn failures | | `subprocess.rs` | `COMMAND_OUTPUT` | Stdout/stderr lines from the subprocess | | `subprocess.rs` | `BANNER` | Output header banner | @@ -111,6 +112,13 @@ to provide visual structure in log output. | `embedded_files.rs` | `FILE_PATH` | File write paths | | `embedded_files.rs` | `FILE_CONTENTS` | File data content (debug level only) | +The cross-user cleanup rows carry two flags together, and the pairing is deliberate: +`PROCESS_CONTROL` is one of the three content types that reach the worker agent log, +so a `FILE_PATH`-only record would go to the session stream alone and the operator who +needs it would not see it. The path in those records is the session working directory, +which makes them the first records in this crate that route a filesystem path into the +worker log rather than only the session stream. + ## Consumer Integration Log consumers (e.g., the worker agent) inspect the `openjd_log_content` key-value pair