From 8ff72290b6eeaae8aca71ae8704be83381bd6e6b Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:18:22 -0700 Subject: [PATCH 1/7] fix(sessions): resolve sudo from trusted dirs, not PATH A session runs job-supplied actions and the job influences the environment they run with, including PATH. Command::new("sudo") therefore resolved a bare name through a search path the job may control, so a job that places an executable named sudo early on PATH gets it run at the session's privilege level rather than the job user's. Add system_commands, which resolves a bare command name against a fixed, ordered list of trusted absolute directories. PATH is never consulted, and neither is any which-style lookup -- those resolve through PATH and so would reintroduce the vulnerability while appearing to fix it. The directory order matters: on NixOS the setuid sudo wrapper lives in /run/wrappers/bin and the /usr/bin copy is absent or not setuid, so hardcoding /usr/bin/sudo would trade a security bug for a cross-user execution failure. Three properties are load-bearing, and each is pinned by a test: * PATH is never read. Mutation-checked by adding a PATH fallback, caught by ignores_path_even_when_it_contains_a_matching_command. * Only paths under TRUSTED_SYSTEM_DIRECTORIES are returned, and names with a path component are rejected so that joining "/usr/bin" with "../../tmp/evil" cannot make this module the injection point. Mutation-checked by removing the guard, caught by escaping_name_is_rejected_even_though_the_target_exists. * A missing command is an error, never a fallback to the bare name. A silent fallback would restore the vulnerability while looking fixed. Mutation- checked, caught by missing_command_is_an_error_and_not_the_bare_name. Five mutants were run in total; all five were caught by a named test, against a green baseline, with the source restored and verified by checksum. Call sites: * cross_user_helper::spawn resolves sudo once and reuses the binding in both the Command and its error message, so the message names the binary that was actually launched. * Session cleanup resolves both sudo and rm, and skips the sudo-based removal if either is missing rather than falling back to a bare name, falling through to the existing TempDir cleanup. Qualifying rm is hardening rather than a privilege boundary -- it runs as the job user either way -- but it stops the removal depending on that user's own login-shell PATH. No public API changes: the module is pub(crate) and Unix-only, since both call sites are already cfg(unix). cargo clippy --all-features --all-targets --workspace -- -D warnings is clean and cargo test --workspace passes. Refs: HackerOne 3942741, CWE-426 Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../openjd-sessions/src/cross_user_helper.rs | 14 +- crates/openjd-sessions/src/lib.rs | 4 + crates/openjd-sessions/src/session.rs | 19 +- crates/openjd-sessions/src/system_commands.rs | 270 ++++++++++++++++++ 4 files changed, 302 insertions(+), 5 deletions(-) create mode 100644 crates/openjd-sessions/src/system_commands.rs diff --git a/crates/openjd-sessions/src/cross_user_helper.rs b/crates/openjd-sessions/src/cross_user_helper.rs index c4b9b959..dbd9d003 100644 --- a/crates/openjd-sessions/src/cross_user_helper.rs +++ b/crates/openjd-sessions/src/cross_user_helper.rs @@ -272,7 +272,16 @@ 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, which the job influences. 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 +297,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..1ea0e91a 100644 --- a/crates/openjd-sessions/src/session.rs +++ b/crates/openjd-sessions/src/session.rs @@ -1036,17 +1036,30 @@ impl Session { .filter_map(|e| e.ok()) .map(|e| e.path().to_string_lossy().to_string()) .collect(); - if !files.is_empty() { + // 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. + let commands = crate::system_commands::find_system_command("sudo") + .zip(crate::system_commands::find_system_command("rm")); + if let (false, Some((sudo, rm))) = (files.is_empty(), commands) { let mut args = vec![ "-u".to_string(), user.user().to_string(), "-i".to_string(), - "rm".to_string(), + rm.to_string_lossy().to_string(), "-rf".to_string(), "--".to_string(), ]; args.extend(files); - let _ = std::process::Command::new("sudo") + let _ = std::process::Command::new(&sudo) .args(&args) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) diff --git a/crates/openjd-sessions/src/system_commands.rs b/crates/openjd-sessions/src/system_commands.rs new file mode 100644 index 00000000..765620c9 --- /dev/null +++ b/crates/openjd-sessions/src/system_commands.rs @@ -0,0 +1,270 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +//! Resolution of system command names to absolute paths, without consulting `PATH`. +//! +//! This module exists because of CWE-426 (Untrusted Search Path). A session runs +//! job-supplied actions, and the job influences the environment those actions run +//! with -- including `PATH`. A bare command name in the argv of one of our *own* +//! privileged helpers (`sudo`) is therefore resolved through a search path the +//! job may control, so a job that drops an executable named `sudo` early on +//! `PATH` gets it run at the session's privilege level. +//! +//! Every such name is resolved here instead, by scanning a fixed list of trusted +//! absolute directories. +//! +//! Three properties are load-bearing, and each is pinned by a test below: +//! +//! * **`PATH` is never read.** Not directly, and not indirectly via a `which` +//! crate or `command -v`, both of which resolve through `PATH` and so would +//! reintroduce the vulnerability while appearing to fix it. +//! * **Only paths under [`TRUSTED_SYSTEM_DIRECTORIES`] are returned**, and a name +//! containing a path separator is rejected -- otherwise joining +//! `"/usr/bin"` with `"../../tmp/evil"` would make this module the injection +//! point it exists to remove. +//! * **A missing command is an error, never a fallback to the bare name.** A +//! silent fallback would restore the vulnerability while looking fixed, which +//! is the worst available failure mode for this class of fix. + +use std::io; +use std::path::{Path, PathBuf}; + +/// Absolute directories searched for system commands, in order. +/// +/// The order is deliberate. On NixOS the setuid `sudo` wrapper lives in +/// `/run/wrappers/bin` and the `/usr/bin` copy is either absent or not setuid, so +/// the wrapper directory must be consulted first. Everywhere else that directory +/// does not exist and costs one `stat`. +/// +/// `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", + "/usr/bin", + "/bin", + "/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; + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + #[cfg(not(unix))] + { + true + } +} + +/// 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()); + } + + #[test] + fn ignores_path_even_when_it_contains_a_matching_command() { + // Pins "PATH is never read". Without this, adding a PATH fallback for + // commands missing from the trusted list would go unnoticed. + // + // PATH is process-global, so this test saves and restores it. It is the + // only test here that touches the environment. + let dir = dir_with_executable("openjd-test-path-cmd"); + let original = std::env::var_os("PATH"); + + std::env::set_var("PATH", dir.path()); + let found = find_system_command("openjd-test-path-cmd"); + match original { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + + assert_eq!(found, None, "PATH was consulted"); + } + + #[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 setuid_wrapper_directory_precedes_usr_bin() { + // On NixOS /usr/bin/sudo is absent or not setuid; the wrapper must win. + let wrapper = TRUSTED_SYSTEM_DIRECTORIES + .iter() + .position(|d| *d == "/run/wrappers/bin") + .expect("/run/wrappers/bin is searched"); + let usr_bin = TRUSTED_SYSTEM_DIRECTORIES + .iter() + .position(|d| *d == "/usr/bin") + .expect("/usr/bin is searched"); + + assert!( + wrapper < usr_bin, + "wrapper directory must be searched first" + ); + } +} From a0c7b363dceb045a0e7244f57dbdba06a68767be Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:22:33 -0700 Subject: [PATCH 2/7] fix(sessions): correct the threat model, widen trusted dirs, drop a test data race Five review findings. The first is a correctness problem in what this change claimed rather than in what it did, and is the most important of them. 1. The stated threat model was wrong for this crate. The module said the job influences the environment `sudo` is resolved with, so a bare name is hijackable. That is true of the Python reference implementation -- there the job's environment is merged over the parent's and handed to the Popen call that launches sudo -- and it is the reported vulnerability. It is not true here. CrossUserHelper::spawn sets no environment on its Command, so the helper inherits the session process's own, and Command::new resolves the program against the parent's PATH. The job's environment reaches only the job's own command, in subprocess.rs, and reaches it after env_clear(). Overstating severity in a security change is its own defect, so the docstring and specs/sessions/cross-user.md now say plainly what this is: hardening, not the fix for a live exploit. It removes an invariant that was stated nowhere and enforced by nothing -- that no caller ever adds an environment to that Command -- whose accidental violation would silently reproduce the Python bug. The other two reasons (the session process's own PATH is an assumption about how the agent is launched, and parity so the same audit reaches the same conclusion in both implementations) are recorded alongside it. 2. /usr/local/bin and /usr/local/sbin added. FreeBSD and the other BSDs install sudo from ports into /usr/local/bin and have no /usr/bin/sudo at all, so the list made cross-user sessions impossible to start there. That is a regression from the bare Command::new("sudo") this replaced, which the login PATH would have resolved. 3. /run/current-system/sw/bin added. /run/wrappers/bin holds only the setuid wrappers, so on NixOS it resolves sudo and nothing else; rm is in the sw/bin symlink farm. The entry the ordering comment justifies supported no complete code path without it. 4. The PATH test was a data race; replaced with a deterministic one. It called std::env::set_var while other tests in the same binary call std::env::vars() concurrently (subprocess.rs) -- the reason that function is unsafe from edition 2024 -- and it could have flaked unrelated tests that spawn bare commands. "PATH is never read" is now asserted against the module's own source via include_str!, splitting off the test module so the assertions do not match themselves. It needs no isolation, cannot race, and still fails on the realistic mutation, since a PATH fallback has to read the environment to work. A behavioural companion test keeps the observable half: an executable that exists and is simply not in a searched directory must not resolve by any route. 5. Session cleanup no longer skips silently. If sudo or rm is missing the cross-user removal is skipped, and the fallback is std::fs::remove_dir_all under a `let _ =`, which cannot remove job-user-owned files. The session directory leaked with job data in it and nothing explained why. Now warns via session_log!, naming the file count and directory. Mutation-checked against a green baseline, source restored and verified by checksum: N1 add a PATH fallback -> caught by never_reads_the_environment N2 drop the BSD local dirs -> caught by bsd_local_directories_are_searched N3 drop the NixOS sw/bin dir -> caught by the_two_nixos_directories_are_present_as_a_pair Specs updated in this commit per AGENTS.md: system_commands.rs added to the module layout, and cross-user.md now describes the resolution and the threat-model difference rather than a plain `sudo -u -i`. Not changed, with a reason: the ":" drive-specifier guard added to the Python siblings is deliberately omitted here. This module is cfg(unix), where ":" is a legal filename character and Path::join gives it no special meaning, so the check would be cargo-culted rather than load-bearing. cargo clippy --all-features --all-targets --workspace -- -D warnings clean; cargo test --workspace green. Refs: HackerOne 3942741, CWE-426 Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- crates/openjd-sessions/src/session.rs | 17 +++ crates/openjd-sessions/src/system_commands.rs | 126 +++++++++++++++--- specs/sessions/architecture.md | 3 + specs/sessions/cross-user.md | 22 ++- 4 files changed, 146 insertions(+), 22 deletions(-) diff --git a/crates/openjd-sessions/src/session.rs b/crates/openjd-sessions/src/session.rs index 1ea0e91a..ee62d5ae 100644 --- a/crates/openjd-sessions/src/session.rs +++ b/crates/openjd-sessions/src/session.rs @@ -1049,6 +1049,23 @@ impl Session { // PATH. let commands = crate::system_commands::find_system_command("sudo") .zip(crate::system_commands::find_system_command("rm")); + if !files.is_empty() && commands.is_none() { + // 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 on the record explains why. + session_log!( + warn, + &self.session_id, + LogContent::FILE_PATH, + "Could not locate sudo and rm in a trusted directory; \ + skipping cross-user cleanup of {} file(s) in {}. Files \ + owned by the job user may remain.", + files.len(), + self.working_directory.display() + ); + } if let (false, Some((sudo, rm))) = (files.is_empty(), commands) { let mut args = vec![ "-u".to_string(), diff --git a/crates/openjd-sessions/src/system_commands.rs b/crates/openjd-sessions/src/system_commands.rs index 765620c9..394332d9 100644 --- a/crates/openjd-sessions/src/system_commands.rs +++ b/crates/openjd-sessions/src/system_commands.rs @@ -2,12 +2,33 @@ //! Resolution of system command names to absolute paths, without consulting `PATH`. //! -//! This module exists because of CWE-426 (Untrusted Search Path). A session runs -//! job-supplied actions, and the job influences the environment those actions run -//! with -- including `PATH`. A bare command name in the argv of one of our *own* -//! privileged helpers (`sudo`) is therefore resolved through a search path the -//! job may control, so a job that drops an executable named `sudo` early on -//! `PATH` gets it run at the session's privilege level. +//! This addresses CWE-426 (Untrusted Search Path). Be precise about the exposure +//! in *this* implementation, because an earlier revision of this comment was not: +//! +//! In the Python reference implementation the hole is directly exploitable. There, +//! the job's environment is merged over the parent's and handed to the `Popen` +//! call that launches `sudo`, so a bare `sudo` is resolved through a `PATH` the job +//! wrote, and a job that drops an executable named `sudo` early on `PATH` gets it +//! run at the session's privilege level. That is the reported vulnerability. +//! +//! **This crate is not in that position today.** `CrossUserHelper::spawn` sets no +//! environment on its `Command`, so the child inherits the session process's own +//! environment, and `Command::new` resolves the program against the *parent's* +//! `PATH`. The job's environment reaches only the job's own command, in +//! `subprocess.rs`, and reaches it after `env_clear()`. So no job-supplied variable +//! influences how `sudo` is located. +//! +//! What remains is a latent hazard rather than a live exploit, which is still worth +//! closing: +//! +//! * The safety of the bare name rests on an invariant stated nowhere and enforced +//! by nothing -- that no caller ever sets an environment on the helper's +//! `Command`. Adding `.env()` or `.envs()` there, for any reason, would silently +//! make it the Python bug. +//! * It presumes the session process's own `PATH` is trustworthy. That is an +//! assumption about how the agent is launched, not a property of this crate. +//! * Parity with the reference implementation: the same audit should reach the same +//! conclusion in both, without a reader having to re-derive the difference. //! //! Every such name is resolved here instead, by scanning a fixed list of trusted //! absolute directories. @@ -39,8 +60,21 @@ use std::path::{Path, PathBuf}; /// 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", ]; @@ -154,24 +188,56 @@ mod tests { assert!(find_system_command_in("openjd-test-cmd", &[dir_s]).is_some()); } + /// This module's own source, minus its tests, for [`never_reads_the_environment`]. + 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 ignores_path_even_when_it_contains_a_matching_command() { - // Pins "PATH is never read". Without this, adding a PATH fallback for - // commands missing from the trusted list would go unnoticed. + fn never_reads_the_environment() { + // Pins "PATH is never read", which is the property the whole module exists + // for and the one a `which`-style rewrite would silently undo. // - // PATH is process-global, so this test saves and restores it. It is the - // only test here that touches the environment. - let dir = dir_with_executable("openjd-test-path-cmd"); - let original = std::env::var_os("PATH"); + // Asserted against the source rather than by setting PATH and observing the + // result. An earlier revision did the latter, and it was wrong twice over: + // `std::env::set_var` mutates process-global state while other tests in this + // same binary call `std::env::vars()` concurrently (`subprocess.rs`), which + // is a data race -- the reason the function is `unsafe` from edition 2024 -- + // and it could flake unrelated tests that spawn bare commands. + // + // This form is deterministic, needs no isolation, and still fails on the + // realistic mutation: a PATH fallback has to read the environment to work. + let production = production_source(); - std::env::set_var("PATH", dir.path()); - let found = find_system_command("openjd-test-path-cmd"); - match original { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } + 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!(found, None, "PATH was consulted"); + assert_eq!(find_system_command("openjd-test-path-cmd"), None); } #[test] @@ -250,6 +316,26 @@ mod tests { } } + #[test] + fn the_two_nixos_directories_are_present_as_a_pair() { + // /run/wrappers/bin alone supports no complete code path: it holds only the + // setuid wrappers, so on NixOS it resolves `sudo` and nothing else. `rm` + // lives in the sw/bin symlink farm, so without that entry the ordering + // resolves sudo and then skips the cleanup for want of rm. + assert!(TRUSTED_SYSTEM_DIRECTORIES.contains(&"/run/wrappers/bin")); + assert!(TRUSTED_SYSTEM_DIRECTORIES.contains(&"/run/current-system/sw/bin")); + } + + #[test] + fn bsd_local_directories_are_searched() { + // FreeBSD and the other BSDs install sudo from ports into /usr/local/bin and + // have no /usr/bin/sudo, so omitting this made cross-user sessions + // impossible to start there -- a regression from the bare name this module + // replaced, which the login PATH resolved. + assert!(TRUSTED_SYSTEM_DIRECTORIES.contains(&"/usr/local/bin")); + assert!(TRUSTED_SYSTEM_DIRECTORIES.contains(&"/usr/local/sbin")); + } + #[test] fn setuid_wrapper_directory_precedes_usr_bin() { // On NixOS /usr/bin/sudo is absent or not setuid; the wrapper must win. 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..31a04a3d 100644 --- a/specs/sessions/cross-user.md +++ b/specs/sessions/cross-user.md @@ -60,12 +60,30 @@ 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. + +The exposure being closed differs from the Python reference implementation, and +the difference is worth stating so an auditor does not have to re-derive it. +There, the job's environment is merged over the parent's and handed to the +`Popen` call that launches `sudo`, making a bare name directly hijackable. Here, +`CrossUserHelper::spawn` sets no environment, so the helper inherits the session +process's own and `Command::new` resolves against the parent's `PATH`; the job's +environment reaches only the job's own command, after `env_clear()`. 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`) whose accidental violation would reintroduce the +Python bug silently. See the [Authentication Token](embedded-cross-user-helper.md#authentication-token) section of the helper spec for rationale and verification details. From 1b8ec6bb750afccbf8d33460b3de2408e87f6471 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:08:12 -0700 Subject: [PATCH 3/7] docs: Reframe resolver comments around problem and solution Two changes to comments only. No behaviour change. Dropped the vulnerability-classification references. They named a taxonomy without telling a reader anything actionable about this code, and the comment reads better stating what goes wrong and what the module does about it. Dropped the "each is pinned by a test in " bookkeeping. It told the reader where tests live rather than why the code is shaped this way, and it goes stale the moment a test file moves. The properties themselves are still listed, now with the reason each one is easy to undo, which is the part that helps someone editing this later. The module docstrings now open with the problem, then the approach, then the three properties and what breaks if each is lost. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- crates/openjd-sessions/src/system_commands.rs | 67 +++++++++---------- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/crates/openjd-sessions/src/system_commands.rs b/crates/openjd-sessions/src/system_commands.rs index 394332d9..635d2fe8 100644 --- a/crates/openjd-sessions/src/system_commands.rs +++ b/crates/openjd-sessions/src/system_commands.rs @@ -2,49 +2,46 @@ //! Resolution of system command names to absolute paths, without consulting `PATH`. //! -//! This addresses CWE-426 (Untrusted Search Path). Be precise about the exposure -//! in *this* implementation, because an earlier revision of this comment was not: +//! The problem: `Command::new("sudo")` resolves a bare name through a `PATH`, +//! which makes correctness depend on whose `PATH` that is. Be precise about the +//! answer here, because it differs from the Python implementation. //! -//! In the Python reference implementation the hole is directly exploitable. There, -//! the job's environment is merged over the parent's and handed to the `Popen` -//! call that launches `sudo`, so a bare `sudo` is resolved through a `PATH` the job -//! wrote, and a job that drops an executable named `sudo` early on `PATH` gets it -//! run at the session's privilege level. That is the reported vulnerability. +//! In `openjd-sessions-for-python`, the job's environment is merged over the +//! parent's and handed to the `Popen` call that launches `sudo`. A bare name there +//! resolves through a `PATH` the job wrote, so a job that supplies its own `sudo` +//! has it run at the session's privilege level. //! -//! **This crate is not in that position today.** `CrossUserHelper::spawn` sets no -//! environment on its `Command`, so the child inherits the session process's own -//! environment, and `Command::new` resolves the program against the *parent's* -//! `PATH`. The job's environment reaches only the job's own command, in -//! `subprocess.rs`, and reaches it after `env_clear()`. So no job-supplied variable -//! influences how `sudo` is located. +//! This crate is not in that position. `CrossUserHelper::spawn` sets no environment +//! on its `Command`, so the helper inherits the session process's own environment +//! and `Command::new` resolves against the parent's `PATH`. The job's environment +//! reaches only the job's own command, in `subprocess.rs`, and reaches it after +//! `env_clear()`. No job-supplied variable influences how `sudo` is located. //! -//! What remains is a latent hazard rather than a live exploit, which is still worth -//! closing: +//! Resolving here is therefore about removing assumptions rather than closing a +//! reachable hole: //! -//! * The safety of the bare name rests on an invariant stated nowhere and enforced -//! by nothing -- that no caller ever sets an environment on the helper's -//! `Command`. Adding `.env()` or `.envs()` there, for any reason, would silently -//! make it the Python bug. -//! * It presumes the session process's own `PATH` is trustworthy. That is an -//! assumption about how the agent is launched, not a property of this crate. -//! * Parity with the reference implementation: the same audit should reach the same -//! conclusion in both, without a reader having to re-derive the difference. +//! * 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 the two implementations answering the same question the same way, so +//! a reader does not have to derive the difference to audit either one. //! -//! Every such name is resolved here instead, by scanning a fixed list of trusted +//! The solution: names are resolved here, by scanning a fixed list of trusted //! absolute directories. //! -//! Three properties are load-bearing, and each is pinned by a test below: +//! Three properties make that work, and all three are easy to undo by accident: //! -//! * **`PATH` is never read.** Not directly, and not indirectly via a `which` -//! crate or `command -v`, both of which resolve through `PATH` and so would -//! reintroduce the vulnerability while appearing to fix it. -//! * **Only paths under [`TRUSTED_SYSTEM_DIRECTORIES`] are returned**, and a name -//! containing a path separator is rejected -- otherwise joining -//! `"/usr/bin"` with `"../../tmp/evil"` would make this module the injection -//! point it exists to remove. -//! * **A missing command is an error, never a fallback to the bare name.** A -//! silent fallback would restore the vulnerability while looking fixed, which -//! is the worst available failure mode for this class of fix. +//! * `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}; From 457fec6697dc55768b9a9c738747d31daa589ad9 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:38:17 -0700 Subject: [PATCH 4/7] docs: Correct a false claim about when job environments are cleared Review finding. The module comment and cross-user.md both said the job's environment reaches the job's own command "after env_clear()", offering that as the reason no job variable can influence how sudo is located. That is wrong. env_clear() appears once in the crate, in subprocess.rs on the same-user path. The cross-user path goes through the helper's runner, which calls .envs(&cmd.env) onto whatever it inherited, and no env_clear() exists anywhere under src/helper/. The conclusion still holds, but for a different reason worth stating correctly: sudo is spawned once when the helper starts, before any job action exists, and per-action environments arrive later over the helper's stdin protocol. Ordering and channel, not clearing. Also reframed the source-scan test. It was named never_reads_the_environment, which claimed more than it delivers: it cannot see a which-style crate or a bare Command::new letting execvp resolve in the child, since neither reads this process's environment. It is a lint for the one mutation that matters in practice, a PATH fallback, which the behavioural test cannot catch because a command absent from the trusted directories is also absent from PATH in that test. Renamed to production_source_contains_no_environment_lookup and the comment now says what it does and does not cover. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- crates/openjd-sessions/src/system_commands.rs | 47 ++++++++++++------- specs/sessions/cross-user.md | 26 ++++++---- 2 files changed, 49 insertions(+), 24 deletions(-) diff --git a/crates/openjd-sessions/src/system_commands.rs b/crates/openjd-sessions/src/system_commands.rs index 635d2fe8..1b044415 100644 --- a/crates/openjd-sessions/src/system_commands.rs +++ b/crates/openjd-sessions/src/system_commands.rs @@ -11,11 +11,19 @@ //! resolves through a `PATH` the job wrote, so a job that supplies its own `sudo` //! has it run at the session's privilege level. //! -//! This crate is not in that position. `CrossUserHelper::spawn` sets no environment -//! on its `Command`, so the helper inherits the session process's own environment -//! and `Command::new` resolves against the parent's `PATH`. The job's environment -//! reaches only the job's own command, in `subprocess.rs`, and reaches it after -//! `env_clear()`. No job-supplied variable influences how `sudo` is located. +//! This crate is not in that position, for two reasons that are 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: @@ -197,19 +205,26 @@ mod tests { } #[test] - fn never_reads_the_environment() { - // Pins "PATH is never read", which is the property the whole module exists - // for and the one a `which`-style rewrite would silently undo. + 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). // - // Asserted against the source rather than by setting PATH and observing the - // result. An earlier revision did the latter, and it was wrong twice over: - // `std::env::set_var` mutates process-global state while other tests in this - // same binary call `std::env::vars()` concurrently (`subprocess.rs`), which - // is a data race -- the reason the function is `unsafe` from edition 2024 -- - // and it could flake unrelated tests that spawn bare commands. + // 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. // - // This form is deterministic, needs no isolation, and still fails on the - // realistic mutation: a PATH fallback has to read the environment to work. + // 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!( diff --git a/specs/sessions/cross-user.md b/specs/sessions/cross-user.md index 31a04a3d..c61ad0c3 100644 --- a/specs/sessions/cross-user.md +++ b/specs/sessions/cross-user.md @@ -76,14 +76,24 @@ rather than falling back to a bare name. The exposure being closed differs from the Python reference implementation, and the difference is worth stating so an auditor does not have to re-derive it. There, the job's environment is merged over the parent's and handed to the -`Popen` call that launches `sudo`, making a bare name directly hijackable. Here, -`CrossUserHelper::spawn` sets no environment, so the helper inherits the session -process's own and `Command::new` resolves against the parent's `PATH`; the job's -environment reaches only the job's own command, after `env_clear()`. 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`) whose accidental violation would reintroduce the -Python bug silently. See the +`Popen` call that launches `sudo`, making a bare name directly hijackable. + +Here it is not, for 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`) whose accidental violation would +reintroduce the Python behaviour silently. See the [Authentication Token](embedded-cross-user-helper.md#authentication-token) section of the helper spec for rationale and verification details. From 8c98007b03a950a30ba9fc9e9c4040a560a007ad Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:25:42 -0700 Subject: [PATCH 5/7] fix(sessions): add the license header, and replace tautological list tests Three review findings on the previous commit. 1. Missing license header lines. The new file carried only the copyright line; every other file under crates/openjd-sessions/src/ carries three, including the SPDX identifier. That is the machine-readable one, so its absence is what matters for license scanning of a published crate. Added. 2. The trusted-list tests asserted literals against literals. Correct, and this is the vacuous-test shape: `TRUSTED_SYSTEM_DIRECTORIES.contains("/usr/local/bin")` cannot fail except by someone deleting the line the assertion names, which the diff already shows. My earlier mutation testing "caught" those mutants only because the mutation was deleting the exact entry the test names, which proves nothing about the list being correct. Replaced with a positive control that asks the real filesystem to resolve `sh` and `rm`, and asserts the result sits under a trusted directory. Verified it is not itself vacuous: misspelling /usr/bin and /bin as /usr/binn and /binn fails it, where the old shape assertions all passed. The platform-specific entries keep a shape assertion, since CI runs on no host that can resolve them, but each now names the platform and the command that needs it so a reader can judge whether it still earns its place. 3. The cleanup warning covered the branch that rarely runs. Also correct. Reaching that line means both binaries resolved, and `sudo` was already resolved once at helper start, so `commands.is_none()` essentially only fires when `rm` is missing. The failures an operator hits happen after resolution: a job user whose login shell is nologin, a sudoers rule that does not permit the command, `rm` refusing a path. All of those were discarded by `let _ = ... .status()`. The status is now inspected and a non-zero exit or spawn error is warned about, which makes the previous commit's claim that cleanup no longer fails silently actually true. cargo clippy --all-features --all-targets --workspace -- -D warnings clean; cargo test --workspace green. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- crates/openjd-sessions/src/session.rs | 35 ++++++++++- crates/openjd-sessions/src/system_commands.rs | 60 ++++++++++++++----- 2 files changed, 79 insertions(+), 16 deletions(-) diff --git a/crates/openjd-sessions/src/session.rs b/crates/openjd-sessions/src/session.rs index ee62d5ae..1ff58535 100644 --- a/crates/openjd-sessions/src/session.rs +++ b/crates/openjd-sessions/src/session.rs @@ -1076,12 +1076,43 @@ impl Session { "--".to_string(), ]; args.extend(files); - let _ = std::process::Command::new(&sudo) + // 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 job user whose login shell is + // nologin, 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. + 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, + "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, + "Could not run cross-user cleanup of {}: {e}; files \ + owned by the job user may remain.", + self.working_directory.display() + ), + } } } } diff --git a/crates/openjd-sessions/src/system_commands.rs b/crates/openjd-sessions/src/system_commands.rs index 1b044415..1840f33a 100644 --- a/crates/openjd-sessions/src/system_commands.rs +++ b/crates/openjd-sessions/src/system_commands.rs @@ -1,4 +1,6 @@ // 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`. //! @@ -329,23 +331,53 @@ mod tests { } #[test] - fn the_two_nixos_directories_are_present_as_a_pair() { - // /run/wrappers/bin alone supports no complete code path: it holds only the - // setuid wrappers, so on NixOS it resolves `sudo` and nothing else. `rm` - // lives in the sw/bin symlink farm, so without that entry the ordering - // resolves sudo and then skips the cleanup for want of rm. - assert!(TRUSTED_SYSTEM_DIRECTORIES.contains(&"/run/wrappers/bin")); - assert!(TRUSTED_SYSTEM_DIRECTORIES.contains(&"/run/current-system/sw/bin")); + 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 bsd_local_directories_are_searched() { - // FreeBSD and the other BSDs install sudo from ports into /usr/local/bin and - // have no /usr/bin/sudo, so omitting this made cross-user sessions - // impossible to start there -- a regression from the bare name this module - // replaced, which the login PATH resolved. - assert!(TRUSTED_SYSTEM_DIRECTORIES.contains(&"/usr/local/bin")); - assert!(TRUSTED_SYSTEM_DIRECTORIES.contains(&"/usr/local/sbin")); + 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"), + ("/run/current-system/sw/bin", "NixOS: setsid, pgrep, rm"), + ("/usr/local/bin", "FreeBSD and other BSDs: sudo from ports"), + ] { + assert!( + TRUSTED_SYSTEM_DIRECTORIES.contains(&directory), + "{directory} was dropped, needed for {why}" + ); + } } #[test] From 8f4f063251164392249f5a5442433d8d6ba0ded4 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 21:10:48 -0700 Subject: [PATCH 6/7] fix(sessions): Correct contradictory and inaccurate comments, and fix log routing Six review findings. Five are comment or diagnostic defects in code this branch added; one changes where a warning lands. 1. cross_user_helper.rs contradicted the module doc on the security claim. The comment above the sudo lookup said the resolution avoids "PATH, which the job influences", while system_commands.rs says the opposite explicitly: the job does not influence the PATH used there, which is why this is hardening rather than a fix for a reachable hole. Contradicting the module doc on the one point an auditor needs settled is worse than saying nothing, so it now points at the module docs. 2. The cleanup warning could not say which binary was missing. `find_system_command("sudo").zip(find_system_command("rm"))` collapsed two independent lookups, and the message read "Could not locate sudo and rm", which describes the less likely case: sudo has already resolved once at helper start by the time cleanup runs. The lookups are now separate and the message names whichever is actually missing. 3. The three cleanup warnings did not reach the worker log. logging.rs states the routing contract: only EXCEPTION_INFO, PROCESS_CONTROL and HOST_INFO go to the worker log, while everything reaches CloudWatch via the session stream. All three warnings this branch added were tagged LogContent::FILE_PATH alone, so the operator who needs them would not have seen them, which defeats the point of adding them. They now carry FILE_PATH | PROCESS_CONTROL. 4. The cfg(not(unix)) branch in the executable check was unreachable. lib.rs declares this module #[cfg(unix)], so a non-unix build never compiles the file. The dead arm returned `true` for any file, executable or not, so it was dead code giving the opposite answer and a reader had to rule out a non-unix caller before trusting the check. Removed, with a note saying why there is no cfg branch. 5. A test justification named commands this crate never resolves. The NixOS entry was justified with "setsid, pgrep, rm". setsid is called in-process via nix::libc::setsid() from pre_exec, and pgrep does not appear in this crate at all. This crate resolves exactly two commands, sudo and rm, so the justification now says rm. 6. A doc link pointed at a test renamed in the previous commit. `never_reads_the_environment` became `production_source_contains_no_environment_lookup`. Doc comments inside cfg(test) are not built, so rustdoc would not have flagged the broken reference. cargo clippy --all-features --all-targets --workspace -- -D warnings clean; cargo test --workspace green. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../openjd-sessions/src/cross_user_helper.rs | 6 +- crates/openjd-sessions/src/session.rs | 62 ++++++++++++------- crates/openjd-sessions/src/system_commands.rs | 24 +++---- 3 files changed, 58 insertions(+), 34 deletions(-) diff --git a/crates/openjd-sessions/src/cross_user_helper.rs b/crates/openjd-sessions/src/cross_user_helper.rs index dbd9d003..98acf980 100644 --- a/crates/openjd-sessions/src/cross_user_helper.rs +++ b/crates/openjd-sessions/src/cross_user_helper.rs @@ -273,8 +273,10 @@ impl CrossUserHelper { ) -> Result<(Self, std::fs::File), SessionError> { let auth_token = generate_auth_token()?; // Resolved from a fixed list of trusted directories rather than through - // PATH, which the job influences. Bound once and reused below so the - // error message names the same binary that was actually launched. + // 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(), diff --git a/crates/openjd-sessions/src/session.rs b/crates/openjd-sessions/src/session.rs index 1ff58535..cb17aedb 100644 --- a/crates/openjd-sessions/src/session.rs +++ b/crates/openjd-sessions/src/session.rs @@ -1047,26 +1047,46 @@ impl Session { // hardening rather than a privilege boundary: it stops // the removal depending on that user's own login-shell // PATH. - let commands = crate::system_commands::find_system_command("sudo") - .zip(crate::system_commands::find_system_command("rm")); - if !files.is_empty() && commands.is_none() { - // 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 on the record explains why. - session_log!( - warn, - &self.session_id, - LogContent::FILE_PATH, - "Could not locate sudo and rm in a trusted directory; \ - skipping cross-user cleanup of {} file(s) in {}. Files \ - owned by the job user may remain.", - files.len(), - self.working_directory.display() - ); + // 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, and reporting "sudo and rm" would describe the less + // likely case of the two: `sudo` has already resolved once at + // helper start by the time cleanup runs. + let sudo = crate::system_commands::find_system_command("sudo"); + let rm = crate::system_commands::find_system_command("rm"); + if !files.is_empty() { + 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.", + missing.join(" or "), + files.len(), + self.working_directory.display() + ); + } } - if let (false, Some((sudo, rm))) = (files.is_empty(), commands) { + if let (false, (Some(sudo), Some(rm))) = (files.is_empty(), (sudo, rm)) { let mut args = vec![ "-u".to_string(), user.user().to_string(), @@ -1098,7 +1118,7 @@ impl Session { Ok(status) => session_log!( warn, &self.session_id, - LogContent::FILE_PATH, + LogContent::FILE_PATH | LogContent::PROCESS_CONTROL, "Cross-user cleanup of {} exited {}; files owned by \ the job user may remain.", self.working_directory.display(), @@ -1107,7 +1127,7 @@ impl Session { Err(e) => session_log!( warn, &self.session_id, - LogContent::FILE_PATH, + 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() diff --git a/crates/openjd-sessions/src/system_commands.rs b/crates/openjd-sessions/src/system_commands.rs index 1840f33a..c2ce3e83 100644 --- a/crates/openjd-sessions/src/system_commands.rs +++ b/crates/openjd-sessions/src/system_commands.rs @@ -99,15 +99,12 @@ fn is_executable_file(path: &Path) -> bool { if !metadata.is_file() { return false; } - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - metadata.permissions().mode() & 0o111 != 0 - } - #[cfg(not(unix))] - { - true - } + // 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. @@ -195,7 +192,8 @@ mod tests { assert!(find_system_command_in("openjd-test-cmd", &[dir_s]).is_some()); } - /// This module's own source, minus its tests, for [`never_reads_the_environment`]. + /// 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 @@ -370,7 +368,11 @@ mod tests { // the list as untouchable. for (directory, why) in [ ("/run/wrappers/bin", "NixOS: the only setuid sudo"), - ("/run/current-system/sw/bin", "NixOS: setsid, pgrep, rm"), + // 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!( From 135707501b66f70be65652e1d28a7ecd4a906670 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 21:44:54 -0700 Subject: [PATCH 7/7] test(sessions): Pin wrapper before the NixOS profile, not before /usr/bin Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- crates/openjd-sessions/src/session.rs | 162 +++++++++++++++--- crates/openjd-sessions/src/system_commands.rs | 69 +++++--- specs/sessions/cross-user.md | 24 +-- specs/sessions/logging.md | 8 + 4 files changed, 203 insertions(+), 60 deletions(-) diff --git a/crates/openjd-sessions/src/session.rs b/crates/openjd-sessions/src/session.rs index cb17aedb..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,9 +1068,31 @@ 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 @@ -1050,9 +1108,7 @@ impl Session { // 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, and reporting "sudo and rm" would describe the less - // likely case of the two: `sudo` has already resolved once at - // helper start by the time cleanup runs. + // fixes. let sudo = crate::system_commands::find_system_command("sudo"); let rm = crate::system_commands::find_system_command("rm"); if !files.is_empty() { @@ -1080,33 +1136,34 @@ impl Session { "Could not locate {} in a trusted directory; \ skipping cross-user cleanup of {} file(s) in {}. \ Files owned by the job user may remain.", - missing.join(" or "), + // " 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 mut args = vec![ - "-u".to_string(), - user.user().to_string(), - "-i".to_string(), - rm.to_string_lossy().to_string(), - "-rf".to_string(), - "--".to_string(), - ]; - args.extend(files); + 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 job user whose login shell is - // nologin, 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. + // 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()) @@ -3101,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 index c2ce3e83..8a2c5b30 100644 --- a/crates/openjd-sessions/src/system_commands.rs +++ b/crates/openjd-sessions/src/system_commands.rs @@ -5,21 +5,16 @@ //! 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 the -//! answer here, because it differs from the Python implementation. +//! 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. //! -//! In `openjd-sessions-for-python`, the job's environment is merged over the -//! parent's and handed to the `Popen` call that launches `sudo`. A bare name there -//! resolves through a `PATH` the job wrote, so a job that supplies its own `sudo` -//! has it run at the session's privilege level. -//! -//! This crate is not in that position, for two reasons that are 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`. +//! 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 @@ -35,8 +30,9 @@ //! 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 the two implementations answering the same question the same way, so -//! a reader does not have to derive the difference to audit either one. +//! * 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. @@ -58,10 +54,14 @@ use std::path::{Path, PathBuf}; /// Absolute directories searched for system commands, in order. /// -/// The order is deliberate. On NixOS the setuid `sudo` wrapper lives in -/// `/run/wrappers/bin` and the `/usr/bin` copy is either absent or not setuid, so -/// the wrapper directory must be consulted first. Everywhere else that directory -/// does not exist and costs one `stat`. +/// 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`. @@ -383,20 +383,35 @@ mod tests { } #[test] - fn setuid_wrapper_directory_precedes_usr_bin() { - // On NixOS /usr/bin/sudo is absent or not setuid; the wrapper must win. + 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 usr_bin = TRUSTED_SYSTEM_DIRECTORIES + let profile = TRUSTED_SYSTEM_DIRECTORIES .iter() - .position(|d| *d == "/usr/bin") - .expect("/usr/bin is searched"); + .position(|d| *d == "/run/current-system/sw/bin") + .expect("/run/current-system/sw/bin is searched"); assert!( - wrapper < usr_bin, - "wrapper directory must be searched first" + wrapper < profile, + "the setuid wrapper must precede the NixOS system profile, which also \ + contains a non-setuid sudo" ); } } diff --git a/specs/sessions/cross-user.md b/specs/sessions/cross-user.md index c61ad0c3..57f96348 100644 --- a/specs/sessions/cross-user.md +++ b/specs/sessions/cross-user.md @@ -73,16 +73,15 @@ directories, never located through `PATH`. Session cleanup resolves `rm` the sam way, and skips the cross-user removal with a warning if either binary is absent rather than falling back to a bare name. -The exposure being closed differs from the Python reference implementation, and -the difference is worth stating so an auditor does not have to re-derive it. -There, the job's environment is merged over the parent's and handed to the -`Popen` call that launches `sudo`, making a bare name directly hijackable. +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. -Here it is not, for 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`. +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 @@ -91,9 +90,10 @@ helper's runner layers the action environment onto whatever it inherited, and no 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`) whose accidental violation would -reintroduce the Python behaviour silently. See the +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