fix(sessions): resolve sudo from trusted dirs to prevent PATH injection

[leongdl]

Problem

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. 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.

Fix

New pub(crate), Unix-only system_commands module 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.

Why not just Command::new("/usr/bin/sudo")

That closes the hole but is not portable. NixOS keeps the setuid sudo wrapper
at /run/wrappers/bin/sudo, with /usr/bin/sudo absent or not setuid, so a
literal would trade a security bug for a cross-user execution failure. The
directory list is ordered so wrapper directories are searched first.

Properties pinned, and mutation-checked

Property	Mutation applied	Caught by
PATH is never read	added a PATH fallback after the trusted scan	ignores_path_even_when_it_contains_a_matching_command
Names with a path component are rejected	removed the guard	escaping_name_is_rejected_even_though_the_target_exists
Missing command errors, never falls back to the bare name	returned Ok(PathBuf::from(name))	missing_command_is_an_error_and_not_the_bare_name
Non-executable files are ignored	forced the exec-bit check true	ignores_a_non_executable_file
Wrapper dir precedes /usr/bin	swapped the two entries	setuid_wrapper_directory_precedes_usr_bin

Five mutants, five caught by a named test, against a green baseline, with the
source restored and verified by checksum afterwards.

One honest note on the tests: rejects_names_containing_a_path_component does
not catch the traversal mutation on its own — with the executable sitting
directly in the searched directory, ../name resolves to a path that does not
exist, so it returns None either way. That is why
escaping_name_is_rejected_even_though_the_target_exists exists: it nests the
searched directory so the traversal reaches a real file, and asserts the
precondition that it does. The first test still pins the empty/./.. cases.

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 actually
  launched.
• Session cleanup resolves both sudo and rm. If either is missing it skips
  the sudo-based removal and falls through to the existing TempDir cleanup
  rather than falling back to a bare name. Qualifying rm is hardening, not 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.

Scope

No public API change: the module is pub(crate) and gated cfg(unix), since
both call sites are already cfg(unix). No spec or public-api.md update is
therefore required.

Test files still invoke bare sudo/kill (test_cross_user.rs,
cli_tests.rs). Not exploitable, and left alone to keep this diff focused.

Verification

• cargo clippy --all-features --all-targets --workspace -- -D warnings clean
• cargo test --workspace passes
• cargo fmt --all applied

Deployment note: cross-user cleanup no longer passes sudo -i

Session cleanup previously ran sudo -u <job-user> -i rm -rf -- <files>. It now runs
sudo -u <job-user> /usr/bin/rm -rf -- <files>, with no -i.

Operators with a sudoers rule written against the -i form, or against a bare rm,
need to update it to the absolute path without -i. This is a deliberate break rather
than an oversight: per sudo(8), -i hands the concatenated argv to the job user's login
shell via -c, escaping everything except alphanumerics, underscores, hyphens and
dollar signs. The operands are filenames the job created, so a file named $HOME was
expanded by that shell to a path that does not exist, and rm -rf on a nonexistent path
exits 0 -- reporting a clean cleanup over a file that was still there and could not be
removed by the fallback. Failing loudly on a stale sudoers rule is preferable to that.

[github-actions]

Mutating PATH here races with the rest of the openjd-sessions lib test binary. std::env::set_var is process-global and Rust tests run concurrently on multiple threads, so:

1. Data race / UB. subprocess.rs:555 does std::env::vars().collect() on every run_subprocess call, and there are many subprocess tests in this same binary. A concurrent set_var / vars() pair is a documented data race (this is exactly why set_var became unsafe in edition 2024).
2. Concrete flakiness. Those tests spawn bare command names (echo, sh — e.g. subprocess.rs:1370, 1379), and the child env is built from the parent's vars. While this test holds PATH = a tempdir containing only openjd-test-path-cmd, a concurrently-spawning test resolves echo against that tempdir and fails with NotFound.

The property being pinned (PATH is never consulted) can be proved without touching the process environment — find_system_command_in already takes an explicit directory list, so a test that passes the tempdir as not in the list and asserts None covers the same ground (does_not_resolve_a_command_outside_the_searched_directories largely does already). If the env-based version is kept, it needs to be in a separate integration test binary, or serialized against every other test that reads the environment or spawns a process.

[leongdl]

Fixed in a0c7b36. You were right that the test was the defect: set_var mutates process-global state while subprocess.rs calls std::env::vars() concurrently in the same test binary. Replaced with a deterministic source-level assertion plus a behavioural companion test. See my reply on 3799018746 for what that scan does and does not cover, since you raised the limitation there.

[github-actions]

Skipping the cross-user removal when either binary is missing is a reasonable choice, but it happens with no diagnostic at all. Every other step in cleanup() logs (the banner, the "Deleting working directory" line), and the fallout is not cosmetic: if rm is not under one of the five trusted directories, files owned by the job user survive, TempDir::cleanup() / remove_dir_all at lines 1074-1081 then fail on them (also let _ =), and the session working directory is silently leaked with job data still in it.

Consider a session_log!(warn, ...) in the else branch naming which lookup failed, so an operator on such an image sees why directories are accumulating rather than having to bisect it.

[leongdl]

Fixed in a0c7b36. The skip now warns via session_log!, naming the file count and the working directory. Your consequence chain is the reason it matters: the fallback is remove_dir_all under a let _ =, which cannot remove job-user-owned files, so the directory leaked with job data and nothing on the record explained why.

[github-actions]

The directory list omits /usr/local/bin, which turns a working configuration into a hard failure on some Unixes. On FreeBSD (and other BSDs) sudo is a port and installs to /usr/local/bin/sudo; there is no /usr/bin/sudo. Before this change Command::new("sudo") found it via PATH; after it, CrossUserHelper::spawn returns SubprocessStart/NotFound and cross-user sessions cannot start at all on those hosts. The same applies to any site that installs a wrapper under /usr/local/sbin.

That may be an acceptable trade (the module is Unix-wide, but CI only covers Linux + macOS, where /usr/bin/sudo exists). If so, it is worth stating the supported-platform assumption in the module doc; if not, adding /usr/local/sbin and /usr/local/bin after the /usr/bin, /bin entries preserves the trust property — both are root-owned on default installs — while keeping the searched set fixed.

[leongdl]

Fixed in a0c7b36 by adding /usr/local/bin and /usr/local/sbin. Correct that this was a regression from the bare name, since the login PATH would have resolved it on FreeBSD.

[github-actions]

The stated threat model does not match how these spawns actually resolve, and since this doc is the rationale for the whole module it is worth getting right.

std::process::Command::new("sudo") resolves against the calling process's PATH — the host agent / CLI process — not against the job's environment. Job-supplied env vars are only ever applied to the child: subprocess.rs:589-592 does env_clear() and then sets only the merged map, and run_via_helper ships the action env over the protocol (cross_user_helper.rs:584-598). Nothing in this crate writes the parent's environment (no set_var outside tests), so a job cannot place an entry on the PATH used to find sudo.

That makes this defense-in-depth against an operator-side PATH (a relative or writable entry inherited by the agent), which is a legitimate reason to do it — but not the described "job drops an executable named sudo early on PATH and gets it run at the session's privilege level". Overstating it makes the module hard to reason about later: a reader who believes the job controls this PATH will draw the wrong conclusions about where the trust boundary is, and about which other bare names elsewhere in the crate are or are not exploitable.

[leongdl]

Fixed in a0c7b36 and refined in 457fec6. Your correction stands and overstating severity in a security change is its own defect, so both the module comment and specs/sessions/cross-user.md now say plainly that this is hardening rather than the fix for a live exploit. What it removes is an invariant stated nowhere and enforced by nothing, that no caller ever adds an environment to that Command.

[github-actions]

New module, no spec update. AGENTS.md asks that specs and code line up in the same commit, and specs/sessions/architecture.md has an explicit "Module Layout" listing every file under src/ — system_commands.rs is absent from it. Worth adding a line there (and a sentence in specs/sessions/cross-user.md, which still documents the launch as plain sudo -u <user> -i ... at line 63) so the trusted-directory resolution and its "missing command is a hard error" behaviour are discoverable from the spec.

[leongdl]

Fixed in a0c7b36. system_commands.rs is in the architecture module layout, and cross-user.md now describes the resolution rather than a plain sudo -u <user> -i, in the same commit as the code per AGENTS.md.

[github-actions]

The two /usr/local entries are now trusted purely by assertion — nothing in this module verifies the property the list is named for. is_executable_file checks only is_file() plus any execute bit; there is no check that the directory or the resolved file is owned by uid 0, nor that it is not group/other-writable. That is fine for /usr/bin and /bin, but /usr/local/bin is not root-owned on a large class of real hosts: a Homebrew install on Intel macOS chowns /usr/local/bin (and /usr/local/sbin) to the installing admin user, and plenty of sites deliberately make /usr/local group-writable for a staff/admin group.

Exposure today is nil, but only by accident of ordering: /usr/bin and /bin precede these entries, and both sudo and rm exist there on every platform in CI. The safety of the newly-added entries therefore rests on "the commands we happen to resolve are always found earlier", which is exactly the kind of unstated invariant the module doc-comment above criticises. A future caller resolving a command that is not in /usr/bin would silently pick it up from a user-writable directory, and find_system_command is pub(crate), so that is a one-line change away.

Two ways to make the trust claim real rather than nominal:

• In is_executable_file (or a companion check in find_system_command_in), reject a candidate whose containing directory or file is not owned by uid 0 or is group/other-writable — std::os::unix::fs::MetadataExt::uid/mode gives both. This keeps the BSD fix working on a correctly-permissioned host and fails closed on a Homebrew-style one.
• Or, if that is more than you want here, say explicitly in the TRUSTED_SYSTEM_DIRECTORIES doc that entries are trusted by assumption and that /usr/local/* is only safe for names also present in /usr/bin, so the next person adding a lookup knows the constraint.

[leongdl]

Correct, and parked rather than fixed. Trust is positional: is_executable_file checks is_file() and an execute bit, never uid-0 ownership or group and other writability, and the Intel-macOS Homebrew case you cite is real. Parked on the same reasoning as the sibling comments in the other repositories: exposure needs root or equivalent already, and /usr/bin and /bin precede the /usr/local entries and hold both commands this crate resolves. Recorded with the MetadataExt::uid and mode approach so it is not lost.

[github-actions]

This test does not pin the property the doc-comment says it pins, and it misses the exact regression the module exists to prevent.

The module doc at the top claims PATH is never read "not directly, and not indirectly via a which crate or command -v", and that each load-bearing property "is pinned by a test below". But a substring scan for std::env / var_os / env::var passes cleanly on all three of the realistic reintroductions:

• which::which("sudo") — the crate reads PATH internally; no std::env token appears here.
• std::process::Command::new(name) with a bare name — execvp resolves it through PATH in the child. This is precisely the "silent fallback to the bare name" the doc calls "the worst available failure mode for this class of fix", and the scan cannot see it.
• use std::env as e; e::var_os("PATH"), or std::env::var("PATH") written across a line break, since contains is line-agnostic but token-exact.

Conversely it will fire on innocuous text: any future doc-comment or error message in the production half that happens to mention std::env (e.g. explaining why the environment is not read) fails the build with a message about environment lookup. Coupling a security invariant to the spelling of the source that documents it is fragile in both directions.

Agreed that std::env::set_var was the wrong fix — the data-race reasoning in the comment is correct. But a behavioural test is still available without touching process state: find_system_command_in already takes the directory list, so pass a directory containing an executable and assert find_system_command (the PATH-free wrapper) does not find it. a_command_present_only_outside_the_trusted_list_is_not_found at line 235 already does exactly that, and it catches all three mutations above. That test is the real pin; consider dropping the source scan, or narrowing it to the specific forbidden identifiers (which::, command -v) with a comment that it is a lint and not the guarantee.

[leongdl]

Acknowledged, and the test is kept with a corrected claim rather than dropped. You are right that a source scan cannot see which::which, Command::new(bare_name), aliased imports, or line-broken spellings. My reason for keeping it: it catches the one mutation that matters in practice, a PATH fallback for names missing from the trusted list, and the behavioural test cannot, because a command absent from the trusted directories is also absent from PATH in that test, so both return None either way. I verified that by running the mutation. It is now named production_source_contains_no_environment_lookup rather than never_reads_the_environment, and the comment states what it does and does not cover.

[github-actions]

This sentence is not accurate for the cross-user path, and it is the load-bearing claim of the whole "not a live exploit" argument — both here and in the copy of it added to specs/sessions/cross-user.md.

env_clear() appears exactly once in the crate, at subprocess.rs:589, and that is the same-user branch. In the cross-user branch the job command is not launched by subprocess.rs at all: run_via_helper (cross_user_helper.rs:583-598) serialises config.env_vars into the protocol env map, and the helper then runs it with

Command::new(&cmd.command).args(&cmd.args).envs(&cmd.env)   // helper/src/runner.rs:27-29

.envs() layers onto the inherited environment — there is no env_clear() anywhere in src/helper/. So on the cross-user path the job's environment is merged over the helper process's, which is precisely the shape the paragraph above attributes to the Python implementation and distinguishes this crate from.

This does not make sudo hijackable — sudo is spawned by the session process before any job env exists, and the helper never spawns sudo — so the "no job-supplied variable influences how sudo is located" conclusion still holds. But the stated reason for it is wrong, and a future auditor who trusts this comment will conclude the crate env-clears on every path when it does not. Since the comment's own premise is that an auditor should not have to re-derive the difference, the fix is to say what is actually true: sudo is resolved and spawned before the job environment is constructed, and no job-influenced environment is ever set on that particular Command. Same correction needed in the cross-user.md paragraph, which repeats "after env_clear()" verbatim.

[leongdl]

Fixed in 457fec6, and this one was a factual error on my part, so thank you for checking it. env_clear() appears once, in subprocess.rs on the same-user path; the helper's runner layers .envs(&cmd.env) onto whatever it inherited and no env_clear() exists under src/helper/. The conclusion holds but for a different reason, which the comment and the spec now give: sudo is spawned once at helper startup, before any job action exists, and per-action environments arrive later over the stdin protocol. Ordering and channel, not clearing.

[github-actions]

This sentence is not accurate on the cross-user path, which is the one this module hardens.

env_clear() is only on the same-user spawn (subprocess.rs:589). run_subprocess rejects cross-user outright at subprocess.rs:518, so a cross-user job never reaches that line. Its environment instead travels over the helper protocol (cross_user_helper.rs:584-598 builds the "env" map) and is applied by the helper at helper/src/runner.rs:27-29:

Command::new(&cmd.command)
    .args(&cmd.args)
    .envs(&cmd.env)      // merge over the helper's inherited env -- no env_clear()

So for cross-user actions the job's variables are merged over the target user's login environment rather than replacing it, and Command::new(&cmd.command) there takes a possibly-bare name that execvp resolves through the merged (job-influenced) PATH. That is not a privilege boundary — the job's own command running as the job user with a PATH the job chose is the intended behaviour, and unlike the Python case nothing privileged is resolved that way. But it does mean the module doc overstates the containment: the job's environment does not reach "only ... after env_clear()", and there is a PATH-resolved bare name left on the cross-user path, just a harmless one.

Worth correcting because the doc is doing real work here. It is the artifact an auditor reads to conclude "hardening, not a live hole," and the conclusion is sound while one of its two premises is not — which makes the note harder to trust than the code deserves. Something like: cross-user job environment is merged by the helper without env_clear(), and the bare name in helper/src/runner.rs resolves through it, but that resolution happens after the privilege transition and so cannot influence how sudo itself is located.

The same wording was added to specs/sessions/cross-user.md ("the job's environment reaches only the job's own command, after env_clear()") and would need the same fix.

[leongdl]

Fixed in 457fec6, which predates this comment by a few minutes so the timing is confusing. You are right on all of it: env_clear() is only on the same-user spawn, cross-user is rejected before reaching it, and the helper applies the action environment with no clearing. The module doc and cross-user.md now rest the conclusion on ordering and channel instead: sudo is spawned once at helper start, before any job action exists, and per-action environments arrive later over the stdin protocol.

[github-actions]

The new warning covers the "binary not found" branch, but the branch that actually runs is still let _ = ... .status(), so the far more likely failure mode stays silent.

Reaching this line means both binaries resolved — which on any real host is the common case, since CrossUserHelper::spawn already resolved sudo successfully at session start, so commands.is_none() here essentially only happens when rm is missing. The ways this removal fails after resolving are the ones an operator will actually hit:

• the job user's login shell is /sbin/nologin or /usr/sbin/nologin (a common hardening for job users), so sudo -i exits non-zero without ever running rm;
• the sudoers rule permits the helper binary but not this rm invocation;
• rm -rf partially fails on some entries and exits non-zero;
• .status() itself returns Err (e.g. resolved sudo was replaced between resolution and spawn).

In every one of those cases the outcome is identical to the case you just added a warning for: TempDir::cleanup() / remove_dir_all below cannot remove job-user-owned files, the session working directory leaks with job data in it, and nothing on the record says so. The comment above this block reasons that "skipping silently would be the worst outcome here" — that reasoning applies just as much to a command that ran and failed as to one that was never found.

Checking the status and logging on failure is a few lines, and unlike the missing-binary path it needs no new control flow:

match std::process::Command::new(&sudo)
    .args(&args)
    .stdin(std::process::Stdio::null())
    .stdout(std::process::Stdio::null())
    .stderr(std::process::Stdio::null())
    .status()
{
    Ok(status) if status.success() => {}
    Ok(status) => session_log!(warn, &self.session_id, LogContent::FILE_PATH,
        "Cross-user cleanup of {} exited with {status}; files owned by the job user may remain.",
        self.working_directory.display()),
    Err(e) => session_log!(warn, &self.session_id, LogContent::FILE_PATH,
        "Could not run cross-user cleanup for {}: {e}",
        self.working_directory.display()),
}

(Distinct from the missing-binary diagnostic already discussed on the branch above — that one is now handled; this is about the exit status of the command that does run.)

[leongdl]

Fixed in 8c98007. Correct, and it makes my previous commit message wrong: I claimed cleanup no longer fails silently, when the branch that actually runs still discarded its result. Your reasoning is what convinced me — sudo was already resolved at helper start, so commands.is_none() essentially only fires when rm is missing, and the failures an operator hits are the post-resolution ones: nologin shells, a sudoers rule that does not permit the command, rm refusing a path.

The status is now inspected, with separate warnings for a non-zero exit and for a spawn error, both naming the working directory and saying job-user-owned files may remain.

[github-actions]

This new file is missing the second and third lines of the license header that every other file in crates/openjd-sessions/src/ carries:

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// Copyright by contributors to this project.
// SPDX-License-Identifier: (Apache-2.0 OR MIT)

tempdir.rs, subprocess.rs, helper_binary.rs, cross_user_helper.rs all have all three; this file has only the first. The SPDX identifier is the machine-readable one, so its absence is the part that matters for downstream license scanning of a published crate (include = ["/src/**"] ships this file to crates.io).

[leongdl]

Fixed in 8c98007. Correct, and the SPDX line is the one that matters: the crate publishes /src/**, so a missing machine-readable identifier is a real gap rather than a formatting nit. All three lines now match the other files in the crate.

[github-actions]

These three list-shape tests assert that a literal appears in the literal written two screens above, so they cannot fail for any reason other than someone deliberately deleting the entry — which the diff already shows. Meanwhile the property that would actually catch a mistake is untested: that TRUSTED_SYSTEM_DIRECTORIES resolves anything at all on the host.

Nothing in this module exercises the real constant against the real filesystem. Every positive test goes through find_system_command_in with a tempdir; the only test that calls the production wrapper (a_command_present_only_outside_the_trusted_list_is_not_found, line 249) asserts None. So a typo in the constant (/ust/bin, a stray trailing character, an entry dropped during a rebase) passes the entire suite green, and the first symptom is CrossUserHelper::spawn failing at runtime on every cross-user session.

A single test closes that gap on every supported Unix — a #[test] fn asserting find_system_command("sh").is_some() and find_system_command("rm").is_some(). POSIX requires /bin/sh, and rm is the command session cleanup depends on; both live under /bin or /usr/bin on Linux, macOS, and the BSDs, so it is portable across the platforms this cfg(unix) module targets, and it fails loudly if the list stops resolving.

Worth considering in place of the three membership assertions — the rationale for each individual entry is already captured in the constant's own comments, which is where a reader will look for it.

[leongdl]

Fixed in 8c98007, and this is the best catch of the round because it invalidates my own verification rather than just the tests. You are right that those assertions cannot fail except by deleting the line they name. Worse, I had reported them as mutation-checked, but the mutation I ran was deleting the exact entry each test names, which proves nothing about the list being right.

Replaced with the positive control you describe: it asks the real filesystem to resolve sh and rm and asserts the result sits under a trusted directory. I verified it is not vacuous in turn by misspelling /usr/bin and /bin as /usr/binn and /binn, which fails it while every old shape assertion 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.

[github-actions]

Two things about this helper that will bite quietly.

The doc link on line 199 is stale. It says for [never_reads_the_environment], but no such function exists — the test was renamed to production_source_contains_no_environment_lookup on this revision. It is a broken intra-doc link that rustdoc will not flag (doc comments inside #[cfg(test)] are not built), so it will just sit there misdirecting the next reader.

The split is a silent-weakening hazard. split_once("#[cfg(test)]") takes the first occurrence in the file. That happens to be the test module today, but #[cfg(test)] is also the idiomatic attribute for a test-only helper function or a test-only use. The moment anyone adds one above line 150 — e.g. a #[cfg(test)] fn fake_dir(...) next to is_executable_file — production silently shrinks to the prefix before it, the two assert!s scan a fragment instead of the module, and the lint keeps passing while checking almost nothing. There is no assertion that the recovered prefix is plausibly the whole production half.

A length floor makes the failure loud instead of silent:

assert!(production.len() > 4000, "production prefix looks truncated — is there a #[cfg(test)] item above the test module?");

Given the comment below is explicit that this scan is the only thing catching a PATH-fallback mutation, it is worth having it fail rather than silently narrow.

[leongdl]

Fixed in 8f4f063. The stale intra-doc link is now production_source_contains_no_environment_lookup, and you are right that rustdoc would never have caught it since doc comments inside cfg(test) are not built.

[github-actions]

The #[cfg(not(unix))] branch is unreachable: lib.rs declares this module as #[cfg(unix)] pub(crate) mod system_commands;, so a non-unix build never compiles this file at all.

That is not just dead weight — it is dead code that returns the opposite answer (true for any file, executable or not), so a reader has to work out whether some non-unix path relies on it. It also weakens ignores_a_non_executable_file (line 291), which silently becomes a no-op assertion under any target where that branch were live.

Dropping the cfg and keeping just the unix body matches the module gating:

    use std::os::unix::fs::PermissionsExt;
    metadata.permissions().mode() & 0o111 != 0

[leongdl]

Fixed in 8f4f063 by deleting the branch. Your framing is the reason it was worth doing rather than leaving: it was not merely dead, it returned the opposite answer, so a reader had to rule out a non-unix caller before trusting the executable check. Replaced with a note saying why there is deliberately no cfg branch.

[github-actions]

if let (false, Some((sudo, rm))) = (files.is_empty(), commands) builds a throwaway tuple to pattern-match a boolean, and it re-evaluates files.is_empty() that was already tested five lines up. A plain nested form says the same thing and makes the two branches obviously exclusive:

if !files.is_empty() {
    match commands {
        Some((sudo, rm)) => { /* spawn */ }
        None => { /* warn */ }
    }
}

That also fixes a live inconsistency: as written, commands is resolved (two stat sweeps over eight directories) even when files is empty and nothing will be removed. Harmless, but it is work done for a branch that cannot run.

[leongdl]

Deferred, with one part of it fixed incidentally. Restructuring that block for the which-binary-is-missing fix in 8f4f063 removed the re-evaluation of files.is_empty() you flagged, but the outer shape is still a tuple pattern rather than the nested form you suggest. I agree yours reads better and makes the branches obviously exclusive. Leaving it as a style change I would rather not fold into a security fix at this point in review; noted as deferred.

[github-actions]

These three new warnings are tagged LogContent::FILE_PATH alone, which routes them away from the one log an operator reads.

logging.rs:7-10 states the contract: "only EXCEPTION_INFO | PROCESS_CONTROL | HOST_INFO go to the worker log; all records go to CloudWatch via the session log stream." FILE_PATH is in neither of those three, so all three of the messages added here (lines 1058, 1098, 1107) land only in the session log stream — the per-session CloudWatch stream that is job-scoped and typically retained per-session.

That defeats the stated purpose. The comment at 1052-1057 justifies the warning as "nothing on the record explains why" the directory leaked, and 1079-1089 as "the failures an operator actually hits happen here." But a leaked session working directory is a host-level symptom — the operator noticing disk fill has the host, not the session id, and so is looking at the worker log, which is precisely where these records do not go.

The existing precedent in this same function agrees. session.rs:962 is the closest analogue — a warn for an operational cross-user failure ("Failed to duplicate the cross-user cancel pipe") — and it is tagged LogContent::PROCESS_CONTROL, not FILE_PATH. Likewise session.rs:993 and 2180 use EXCEPTION_INFO for failures.

LogContent is a bitflag set describing content, so this is additive rather than a swap — these messages do embed a path, so FILE_PATH is not wrong, just insufficient:

LogContent::FILE_PATH | LogContent::PROCESS_CONTROL,

Worth applying to all three so the diagnostics reach the reader the comments were written for.

[leongdl]

Fixed in 8f4f063. This was the most consequential of the round, because it meant the warnings I had just added were invisible to the operator who needs them. All three now carry LogContent::FILE_PATH | LogContent::PROCESS_CONTROL, per the routing contract in logging.rs that you quoted.

[github-actions]

This justification names two commands the crate never resolves, which undercuts the exact purpose the test states for itself.

setsid is not resolved as a binary anywhere — subprocess.rs:199 calls nix::libc::setsid() in-process from pre_exec, so no PATH/directory lookup is involved. And pgrep does not appear in crates/openjd-sessions/ at all (grep -rn pgrep crates/openjd-sessions/src/ matches only this line). The only commands find_system_command is ever called with are sudo (cross_user_helper.rs:279) and rm (session.rs:1069).

This matters because the test's own comment sets the bar: "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." A reader who applies that instruction will grep for setsid, find only an in-process libc call, grep for pgrep, find nothing, and be unable to tell whether the entry is load-bearing or whether the note is stale. The whole value of the assertion is the rationale being checkable.

The accurate rationale for /run/current-system/sw/bin is already written correctly at lines 70-75 — it is there for rm, because /run/wrappers/bin holds only setuid wrappers and would otherwise resolve sudo while leaving rm unfound. Suggest matching it:

("/run/current-system/sw/bin", "NixOS: rm (the wrapper dir has only setuid binaries)"),

[leongdl]

Fixed in 8f4f063. Correct, and it undercut the assertion's own stated purpose. This crate resolves exactly two commands, sudo and rm: setsid is called in-process via nix::libc::setsid() from pre_exec, and pgrep does not appear in the crate at all. The justification now says rm.

[github-actions]

Inspecting the status is a real improvement over let _ =, but stderr stays Stdio::null() — and every failure the comment above promises to surface is distinguishable only by stderr, not by the exit code.

The comment at 1082-1085 names three: "a job user whose login shell is nologin, a sudoers rule that does not permit this command, or rm refusing a path." All three exit 1 from sudo:

• nologin shell → This account is currently not available. (exit 1)
• sudoers denial → Sorry, user <x> is not allowed to execute ... as <y>. (exit 1)
• rm refusing a path → rm: cannot remove ...: Permission denied (exit 1, propagated through the login shell)

So the emitted record is the same string in all three cases, and ExitStatus's Display gives only exit status: 1. The operator learns that cleanup failed — which the existence of the leftover directory already told them — but not which of the three it was, and those have completely different remediations (fix the user's shell / edit sudoers / investigate ownership). The stated goal was that the failures "an operator actually hits" stop being silent; as written they are non-silent but non-diagnostic.

rm -rf writes nothing to stdout on success, so capturing is cheap and bounded in the normal case:

match std::process::Command::new(&sudo)
    .args(&args)
    .stdin(std::process::Stdio::null())
    .output()
{
    Ok(out) if out.status.success() => {}
    Ok(out) => session_log!(
        warn, &self.session_id, LogContent::FILE_PATH | LogContent::PROCESS_CONTROL,
        "Cross-user cleanup of {} failed ({}): {}; files owned by the job user may remain.",
        self.working_directory.display(),
        out.status,
        String::from_utf8_lossy(&out.stderr).trim(),
    ),
    ...
}

Note that stderr here can contain job-controlled filenames (they appear in rm's messages), so whatever LogContent flags you settle on should reflect that.

Minor, on the same line: "... exited {}" with an ExitStatus renders as exited exit status: 1. "failed ({status})" or status.code() reads better.

[leongdl]

Correct, and deferred rather than fixed. Every failure the comment names does exit 1 from sudo, so the exit code alone cannot tell them apart, and stderr is still Stdio::null(). I am stopping short of capturing and relaying stderr here: it changes what this cleanup path collects and logs, and the gap this commit set out to close was silence, which the status check does close. An operator now learns cleanup failed and which directory is affected, rather than nothing. Distinguishing the three causes is a worthwhile follow-up and is recorded as deferred rather than dropped.

[github-actions]

"PATH, which the job influences" contradicts the module doc and the spec added in this same PR, and it contradicts them on the one point a security auditor most needs settled.

system_commands.rs:16-21 says the opposite explicitly: "This crate is not in that position... 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." And cross-user.md:96-99 concludes "Resolving from trusted directories is therefore hardening in this crate rather than the fix for a live exploit."

The doc is the correct one — I checked: spawn here calls no .env()/.envs(), and the only env_clear/envs sites are subprocess.rs:589 and the helper's own runner.rs:29, both downstream of this spawn. The job cannot reach the PATH used on line 284.

Why this is worth fixing rather than a nit: the doc's whole framing is that the precise threat model matters "because it differs from the Python implementation" and that a reader "does not have to derive the difference to audit either one." This comment sits at the call site, which is where an auditor looks first, and it asserts the Python exposure applies here. It also cuts the other way — someone who believes the job controls this PATH may conclude the surrounding code is already hardened against job-controlled environments and skip a check that is actually load-bearing.

Suggest matching the doc:

// Resolved from a fixed list of trusted directories rather than through
// PATH. The job cannot reach this PATH today (no environment is set on
// this Command, and sudo runs before any action exists), so this removes
// that unstated invariant rather than closing a live hole -- see
// system_commands.rs. Bound once and reused below so the error message
// names the same binary that was actually launched.

[leongdl]

Fixed in 8f4f063. Agreed this was the worst of the batch, because it contradicted the module doc and the spec on the one point an auditor needs settled. The comment now points at the module docs rather than restating a claim the same PR disproves.

[github-actions]

.zip() collapses two independent lookups into one Option, so the warning it feeds cannot say which binary was missing — and the message it emits ("Could not locate sudo and rm") reads as though both were, which is the less likely case of the two.

The distinction is actionable. sudo missing from all eight trusted directories and rm missing are different host problems with different fixes, and on the platforms this list was extended for they fail in specifically different ways — the /run/current-system/sw/bin comment at system_commands.rs:70-75 describes exactly the asymmetric case where sudo resolves from the wrapper directory and rm does not. An operator reading "sudo and rm" on a NixOS host will go looking for a missing sudo that is in fact present.

Resolving them separately keeps the message specific and lets the two find_system_command calls short-circuit:

let sudo = crate::system_commands::find_system_command("sudo");
let rm = crate::system_commands::find_system_command("rm");
if !files.is_empty() {
    match (&sudo, &rm) {
        (Some(sudo), Some(rm)) => { /* spawn */ }
        _ => session_log!(
            warn, ...,
            "Could not locate {} in a trusted directory; skipping cross-user cleanup \
             of {} file(s) in {}. Files owned by the job user may remain.",
            match (sudo.is_some(), rm.is_some()) {
                (false, false) => "sudo or rm",
                (false, true) => "sudo",
                _ => "rm",
            },
            files.len(),
            self.working_directory.display()
        ),
    }
}

[leongdl]

Fixed in 8f4f063. The two lookups are separate now and the warning names whichever is actually missing. Your point about which case is likely is what made it worth changing: sudo has already resolved once at helper start by the time cleanup runs, so "sudo and rm" described the less likely situation.

[github-actions]

This test pins a pair of directories that can never conflict, and leaves unpinned the pair that actually can.

The module's own comment on line 70-75 states the premise: on NixOS /usr/bin "holds just env". If there is no /usr/bin/sudo on NixOS, then the relative order of /run/wrappers/bin and /usr/bin cannot decide which sudo is chosen there — the assertion holds vacuously, and it would keep holding under a reordering that breaks the invariant it is named for.

The real competitor is /run/current-system/sw/bin, added on this revision. With security.sudo.enable = true (the NixOS default) the sudo package is in the system profile, so both paths exist:

• /run/current-system/sw/bin/sudo — store symlink, mode 0755, not setuid
• /run/wrappers/bin/sudo — the setuid wrapper, the only one that works

The list currently gets this right (/run/wrappers/bin is first), but nothing pins it. Swap the first two entries — or insert a new entry between them — and find_system_command("sudo") returns the non-setuid copy on every NixOS host, CrossUserHelper::spawn succeeds in resolving, and the spawn then fails at runtime with sudo: must be owned by uid 0 and have the setuid bit set. That is a worse failure than not finding it at all, because system_command_path no longer gets to produce its explanatory error. CI runs on Linux and macOS, so no test here would catch it.

Asserting against /run/current-system/sw/bin instead pins the ordering that decides the outcome:

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"
);

Keeping the /usr/bin assertion alongside it is harmless, but on its own it is not the guard.

[leongdl]

Fixed in 4ffc0dc. You are right that the assertion was vacuous for its own stated purpose: with no /usr/bin/sudo on NixOS, the wrapper-versus-/usr/bin order cannot decide anything there, so it would have stayed green under exactly the reordering it was named to prevent.

The test now pins /run/wrappers/bin before /run/current-system/sw/bin, which is the pair that can conflict, and the comment records why: with the default security.sudo.enable both paths exist and only the wrapper is setuid. Your point about the failure mode is what made this worth fixing rather than filing — resolution succeeding and the spawn then failing with "must be owned by uid 0 and have the setuid bit set" is worse than a clean not-found, because system_command_path never gets to produce its explanatory error.

Mutation-checked: swapping the two entries fails setuid_wrapper_precedes_the_nixos_system_profile and nothing else.

[github-actions]

Putting the two /run entries ahead of /usr/bin and /bin inverts the one property that was keeping the untrusted-directory risk theoretical, and there is no functional reason for them to be first.

The ordering rationale in the doc above and in setuid_wrapper_precedes_the_nixos_system_profile (line 401) only establishes a relative constraint: /run/wrappers/bin must precede /run/current-system/sw/bin, because on NixOS both hold a sudo and only the wrapper is setuid. The test comment says so explicitly — "Not /usr/bin. On NixOS /usr/bin holds only env, so there is no /usr/bin/sudo for the wrapper to beat." So nothing requires either /run entry to outrank /usr/bin; on NixOS the /usr/bin and /bin probes simply miss.

But on every non-NixOS host the ordering now means the resolver consults two /run paths before the root-owned system directories, and is_executable_file verifies only is_file() plus an execute bit — no uid-0 ownership check, no group/other-writable check. /run itself is root-owned 0755, so this is not reachable by an ordinary unprivileged user. It is reachable where something else creates those paths:

• a systemd unit with RuntimeDirectory=wrappers and User=svc — systemd creates /run/wrappers owned by svc, who can then create bin/sudo inside it;
• container/init images that pre-create or bind-mount paths under /run with non-root ownership.

The blast radius if that precondition holds is the largest in this crate: the resolved sudo is executed by the session process itself, which on a cross-user host is typically root. CrossUserHelper::spawn would exec an attacker-controlled binary as root. rm in Session::cleanup has the same exposure via sudo -u <user> -i <rm>.

Note this is a change in kind from the previous revision, not just a longer list. There, the argument for the unverified entries was ordering: /usr/bin and /bin came first, so the loosely-trusted directories were only ever reached for names absent from the real system dirs. Both new entries sit in front of that guard, so the argument no longer applies to any name.

Reordering costs nothing and restores it:

pub(crate) const TRUSTED_SYSTEM_DIRECTORIES: &[&str] = &[
    "/usr/bin",
    "/bin",
    "/run/wrappers/bin",          // NixOS: setuid sudo wrapper
    "/run/current-system/sw/bin", // NixOS: rm; must follow the wrapper dir
    "/usr/local/bin",
    ...
];

The wrapper still precedes the profile, so setuid_wrapper_precedes_the_nixos_system_profile keeps passing and NixOS still resolves the setuid sudo — because the two directories it beats (/usr/bin, /bin) do not contain a sudo there at all. The general fix is still an ownership check in is_executable_file, but the ordering alone removes the newly-introduced regression.

[leongdl]

Deferred, and recorded rather than dropped.

The relative-constraint reading is right: only wrapper-before-profile is required, so /usr/bin and /bin could lead at no functional cost on NixOS, where neither holds a sudo.

Not changing it here for two reasons. The precondition is a non-root-owned directory under /run -- your RuntimeDirectory=wrappers example -- which is the same exposure class as the ownership/writability check already parked from an earlier round; ordering narrows that window but the check is the fix. And this ordering is identical in all four resolvers in this series, each with a test pinning wrapper-before-/usr/bin, so reordering one repository alone would make them disagree while the module docs claim they do not.

Parked as item 7 with the reorder and the ownership check together as the follow-up.

[github-actions]

This paragraph, and the matching module doc at system_commands.rs:10-14, publish an exploitable privilege-escalation path in a different, still-shipping product — and this is a public repository.

The text names the component (openjd-sessions-for-python), the exact mechanism (job environment merged over the parent's and handed to the Popen call that launches sudo), the conclusion ("making a bare name directly hijackable"), and — in the module doc — the exploitation step and its impact: "a job that supplies its own sudo has it run at the session's privilege level." A reader needs nothing further to weaponise it against a Python-based Deadline Cloud worker.

The asymmetry is what makes this worth pausing on. This PR fixes the Rust crate, where the same text is careful to explain the issue is not reachable (spawn sets no environment; sudo resolves before any action exists). So the only place the described attack actually lands is the implementation this PR does not touch and cannot fix. Merging ships a fix for the non-vulnerable side while disclosing the vulnerable side.

If the Python issue is already fixed and public, a link to the advisory or the fixing commit resolves this outright and makes the comparison more useful than the prose does. If it is not, this is a coordinated-disclosure question for the maintainers rather than a code review one, and the intent here can be preserved without the disclosure — the argument this crate needs is entirely about its own invariants:

A bare name resolves through whatever PATH the spawning process has. That is safe here only while no caller adds an environment to the helper's Command and only while the agent's own PATH is trustworthy — neither is stated or enforced, so resolving from a fixed list removes an invariant that could be violated by an edit to an unrelated line.

That justifies the change on its own without describing an attack on another codebase. Same applies to system_commands.rs:10-14 and :20-22.

Flagging rather than asserting: whether these details are already public is something the maintainers know and I do not.

[leongdl]

Fixed. Good catch, and the right thing to pause on.

Removed the cross-product exploit narrative from both places. The module doc and specs/sessions/cross-user.md now argue from this crate's own invariants -- a bare name resolves through whatever PATH the spawning process holds, which is safe here only while no caller adds an environment to that Command and the agent's PATH is trustworthy, neither stated nor enforced -- without describing the mechanism or impact in the sibling implementation. The cross-implementation consistency argument is kept, since it needs only the fact that both resolve the same way.

[github-actions]

Resolving rm against the session process's filesystem view and then passing the result to sudo -u <user> -i is a category error: the path is resolved in one namespace and executed in another, and the two are not guaranteed to agree.

The absolute path is chosen by find_system_command, which stats /run/wrappers/bin, /usr/bin, … as seen by the agent. rm then runs after sudo -u <user> -i, which starts a login shell for the job user. Where the job user has a different view of the filesystem, the resolved path can be valid for the agent and absent for the target:

• On NixOS, /run/current-system/sw/bin/rm is a symlink into /nix/store. The agent resolves it, but a job user whose profile pins a different system generation, or is confined by a ProtectSystem=/namespace-scoped PAM session, can get a path that does not resolve — and this is exactly the entry the new /run/current-system/sw/bin line was added to serve.
• With pam_namespace or any polyinstantiation on the job user's login, /usr need not be identical across the boundary.

Passing the bare name "rm" was actually correct for this call in a way it was not for sudo: the callee resolves it in its own context, which is the context that has to execute it. The asymmetry is worth being explicit about — sudo is executed by the privileged agent, so pinning its path is a genuine privilege-boundary decision; rm is executed by the unprivileged job user in the job user's own shell, and the comment above (lines 1046-1049) concedes this is "hardening rather than a privilege boundary." So this change trades a real (if narrow) portability property for a benefit the code itself describes as not security-relevant.

Practical consequence when they disagree: sudo exits nonzero, the new Ok(status) arm logs "Cross-user cleanup … exited …", and cleanup falls through to std::fs::remove_dir_all, which cannot remove job-user-owned files. The session directory leaks with job data in it — the exact outcome the surrounding comments say they are trying to prevent.

Two ways out, depending on which property you want:

1. Keep "rm" bare here and note why: it is resolved by the job user's login shell, which is the process that runs it, and the PATH involved is the target user's, not one the job controls. The hardening this module provides does not apply across a sudo -i boundary.
2. If pinning is preferred anyway, drop -i for this call so no login shell/namespace is interposed, making the agent's resolution authoritative for the callee too.

Either is defensible; the current combination — resolve here, execute over there, through a login shell — is the one that can be wrong.

[leongdl]

Correct, and it catches a genuine inconsistency with our own reasoning: in openjd-sessions we deliberately left kill bare under sudo -i, on exactly the argument you make here -- the login shell resolves it in the context that executes it, and -i has already reset the environment.

Deferring rather than fixing in this change, because it is a design reversal with a cascade: dropping the rm resolution removes the only caller of /run/current-system/sw/bin in this crate, which then wants its list entry and two tests revisited. Doing that inside a security fix would bury it.

Parked as item 8, with both of your options recorded and the bare-name one preferred.

[github-actions]

missing.join(" or ") defeats the stated purpose of the separate lookups. The comment at 1050-1055 says they are looked up individually "so the warning can name the one that is actually missing" — but " or " makes the message read as uncertainty in both cases it can produce:

• one missing → Could not locate sudo in a trusted directory — fine, the single-element join has no separator.
• both missing → Could not locate sudo or rm in a trusted directory — reads as "one of these two, we are not sure which," when in fact the code knows both are absent. An operator debugging this would install sudo, retry, and hit the same warning.

" and " is correct for the both-missing case and identical for the one-missing case, since join on a single element never emits the separator. The comment's justification for " or " — that sudo has already resolved once at helper start, so both-missing is unlikely — argues that the branch is rare, not that the wording should be wrong when it fires.

Minor structural note on the same block: the if !files.is_empty() guard at 1058 and the files.is_empty() test inside the if let at 1089 are the same condition evaluated twice, with the warning block wedged between them. Folding the warning into the else of a single if files.is_empty() would remove the duplication and make it obvious that the warn and the spawn are the two exclusive arms of one decision.

[leongdl]

Fixed. " and " now, with the reasoning you gave recorded inline: when both are absent the code knows both are, and "sudo or rm" invites the operator to install one and hit the same warning. The single-missing message is unchanged because a one-element join emits no separator, and the comment above no longer claims " or " is justified by the branch being rare.

Left the duplicated files.is_empty() test as it is -- real, but it is a readability change to a block this PR is otherwise done with, and folding the warn and the spawn into one decision is worth doing deliberately rather than as a tail-end edit.

[github-actions]

This arm cannot detect the one cleanup failure a job can trigger deliberately, because sudo -i interposes a shell that re-parses the paths and rm -rf exits 0 on a path that does not exist.

sudo -i <cmd> <args...> does not execv the command. Per sudo(8): "If a command is specified, it is passed to the shell for execution via the shell's -c option." sudo builds that single string by concatenating the argv with spaces, backslash-escaping shell metacharacters — but its escaping loop deliberately exempts $ (alongside alnum, _ and -), so parameter expansion still happens in the target user's login shell.

The paths in args come from read_dir of the session working directory at line 1035, i.e. they are filenames the job created. So for a file the job names with a $:

• job action creates $HOME (or $USER, $PATH, …) inside the working directory
• args gets /sessions/<id>/$HOME
• the login shell expands it to /sessions/<id>//home/jobuser, which does not exist
• rm -rf on a nonexistent path is a documented no-op that exits 0

So status.success() is true, this match takes the empty arm, nothing is logged, and the file is still there owned by the job user. Control then falls to the TempDir cleanup / remove_dir_all below, which cannot remove it — the session directory leaks with job data in it and the record says cleanup succeeded. That is precisely the silent-leak outcome the comment at 1103-1113 says this change exists to eliminate, and it is the one case that is job-reachable rather than host-misconfiguration-reachable.

A $ in a filename is not exotic — shell scripts, Windows-origin filenames, and generated asset names all produce them, so this also fires by accident, not only on purpose.

Dropping -i for this call removes the shell entirely and makes the argv the argv:

let mut args = vec![
    "-u".to_string(),
    user.user().to_string(),
    rm.to_string_lossy().to_string(),
    "-rf".to_string(),
    "--".to_string(),
];

rm needs no login environment, so -i buys nothing here while adding a re-parsing layer over job-controlled strings. (It also makes the resolved absolute rm authoritative for the callee, which the login shell currently undercuts.) If -i must stay, then the status check needs to be paired with a post-check that the directory is actually empty, since the exit code cannot speak for paths the shell rewrote.

[leongdl]

Confirmed and fixed. This is the strongest finding in the series, so thank you for it.

Verified the mechanism before acting on it, both in sudo(8) -- with -i the argv is concatenated for the shell's -c "after escaping each character (including white space) with a backslash except for alphanumerics, underscores, hyphens, and dollar signs" -- and by reproducing it: building the sh -c string with those exact escaping rules against a file named $HOME gives rm exit 0 with the file still on disk. So the status check this revision added was blind to the one cleanup failure a job can cause deliberately.

Took your first option: -i is gone from that call. sudo now execs rm directly, so nothing re-parses job-chosen filenames, the exit status means what the comment claims, and the resolved absolute path is the one that runs -- which also resolves the namespace concern you raised separately, since there is no longer a login shell to re-resolve it.

The argv is now a pure function, cross_user_cleanup_args, with tests pinning the absent -i, the resolved path, and -- ahead of any filename. All three are mutation-checked, including re-adding -i.

[github-actions]

The PROCESS_CONTROL flag added on this revision makes specs/sessions/logging.md wrong, and that file is the one place the flag-per-module mapping is written down.

Its "Logging Coverage" table (line 100-108) enumerates content types per module and gives session.rs exactly three rows — HOST_INFO, FILE_PATH, BANNER. PROCESS_CONTROL appears only against subprocess.rs ("PID start, SIGTERM, SIGKILL, exit code, spawn failures"). After this change session.rs emits three PROCESS_CONTROL records (here, 1125, 1134), so the table no longer describes the code.

That matters more than a normal doc lag because of what the table is for. PROCESS_CONTROL is one of the three flags that reach the worker agent log (logging.rs:9, and logging.md:34), so the table is effectively the list of what an operator can expect to see in the agent log. A reader auditing "which session records surface outside CloudWatch" gets a wrong answer from it. architecture.md and cross-user.md were both updated in this PR; this file is the one that got missed, and AGENTS.md:123 asks for spec and code to line up before committing.

One row covers it:

| `session.rs` | `PROCESS_CONTROL` | Cross-user cleanup failures (command not found, nonzero exit, spawn error) |

Also worth a sentence there or in cross-user.md: these records carry FILE_PATH | PROCESS_CONTROL together, and the path they carry is the session working directory. That is the crate's first record that routes a filesystem path into the worker log rather than only the session stream, which is a routing decision a reader of logging.md would otherwise have to discover from the source.

[leongdl]

Fixed. specs/sessions/logging.md gets the row, and you are right that this file matters more than a normal doc lag because it is the de facto list of what reaches the agent log.

Also added the sentence you suggested: these records carry FILE_PATH | PROCESS_CONTROL together because PROCESS_CONTROL is what routes them to the worker log, and the path they carry is the session working directory -- the crate's first records to put a filesystem path there rather than only in the session stream.

[github-actions]

This doc paragraph is now contradicted by the test that pins the ordering, and it is the paragraph a future reader will use to decide whether the ordering can be changed.

Here: "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."

setuid_wrapper_precedes_the_nixos_system_profile (line 395-402), rewritten in the newest commit, says the opposite in its own words: "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." And it names the actual competitor — /run/current-system/sw/bin.

So the const's stated reason for the ordering is the one the test just established is vacuous. That is the reverse of the useful direction: the invariant that is real (/run/wrappers/bin before /run/current-system/sw/bin) is documented only inside a #[cfg(test)] comment, while the declaration site documents a constraint that does not hold. Anyone reasoning from the doc comment concludes the two /run entries must outrank /usr/bin, which is exactly the conclusion the test contradicts.

Moving the real rationale up here keeps the two in agreement:

/// The order is deliberate. On NixOS the only working `sudo` is the setuid wrapper
/// in `/run/wrappers/bin`; `/run/current-system/sw/bin` also holds a `sudo` that is
/// not setuid, so the wrapper must precede it. (`/usr/bin` holds no `sudo` there at
/// all.) Everywhere else `/run/wrappers/bin` does not exist and costs one `stat`.

Worth noting the same paragraph is the only written justification for placing both /run entries ahead of /usr/bin and /bin, and it does not actually justify that — the invariant is relative between the two /run entries only.

[leongdl]

Fixed. The paragraph was stating a constraint the code does not rely on, which is worse than vague given it is what a future reader would use to judge whether the order can change.

It now says the ordering matters against /run/current-system/sw/bin and not against /usr/bin, explains why -- the profile also holds a sudo, non-setuid because nix store paths cannot carry the bit -- and names the test that pins the pair.

[github-actions]

Dropping -i closes the shell-expansion route, but the same silent-leak outcome is still reachable through the String conversion this signature now fixes in place, and the new status.success() check papers over it exactly as the doc above says it would.

files is built one screen up (line ~1072) as

.map(|e| e.path().to_string_lossy().to_string())

On Linux a filename is an arbitrary byte sequence with no encoding requirement, so a job that writes a file whose name is not valid UTF-8 — printf "bad\xffname" | xargs touch, an archive extracted with Latin-1 names, a DCC asset from a Windows share — gets each invalid byte replaced by U+FFFD (EF BF BD). The string handed to cross_user_cleanup_args therefore names a path that does not exist on disk, and:

• rm -rf on a nonexistent operand is a documented no-op: -f "ignore[s] nonexistent files and arguments, never prompt", and POSIX requires exit 0 for it.
• so sudo exits 0, status.success() is true, the empty match arm is taken, and nothing is logged;
• the real file is still there, owned by the job user, and remove_dir_all below (under let _ =) cannot remove it.

That is the identical end state the doc-comment on this function describes for the $HOME case — "a silently leaked session directory, reachable by the job rather than by host misconfiguration" — reached by a different mechanism that -i removal does not touch. It is arguably easier to hit, since it needs no adversarial intent, just a non-UTF-8 byte in a filename.

The -i fix is what makes this newly load-bearing: previously the status was discarded, so lossy paths were one of many silent failures. Now the code asserts the exit status is meaningful and the comment at 1140-1142 says why — but the stated reason ("no -i") is only one of the two ways exit 0 can lie here.

OsString avoids the round-trip entirely, and Command::args accepts anything AsRef<OsStr>, so nothing downstream needs to change:

let files: Vec<std::ffi::OsString> = entries
    .filter_map(|e| e.ok())
    .map(|e| e.path().into_os_string())
    .collect();

fn cross_user_cleanup_args(
    user: &str,
    rm: &std::path::Path,
    files: Vec<std::ffi::OsString>,
) -> Vec<std::ffi::OsString> {
    let mut args: Vec<std::ffi::OsString> = vec![
        "-u".into(),
        user.into(),
        rm.as_os_str().to_owned(),   // also drops the to_string_lossy on `rm`
        "-rf".into(),
        "--".into(),
    ];
    args.extend(files);
    args
}

does_not_interpose_a_login_shell and passes_the_resolved_rm_and_terminates_options_before_filenames both still work over OsString with .into() on the literals. A third case is worth adding alongside them, since it is the one this misses: build the argv from OsString::from_vec(vec![b/, 0xff]) (via std::os::unix::ffi::OsStringExt) and assert the byte survives into the argv unchanged — that pins the property against a future refactor back to String.

If keeping String is preferred, then the status check needs a companion post-check (e.g. read_dir again and warn if non-empty), because the exit code cannot speak for operands that never referred to a real file.

[leongdl]

Deferred, with the reasoning acknowledged. to_string_lossy can replace invalid UTF-8, so a path that round-trips lossily would be handed to rm as a name that does not exist, which rm -rf reports as success.

Not fixing it here because it is narrower than it looks and the fix is a signature change: taking OsString/Path operands through to Command::args avoids the conversion entirely, which is the right shape but touches the helper argv contract rather than command resolution. Filed with that as the prescription.

[github-actions]

This arm looks like it will fire on every cross-user session, not on the failures it is meant to surface — and the message it emits is wrong about who owns the leftover.

The working directory always contains a .helpers-<uuid> directory, created at session.rs:681 via helper_binary::create_helpers_dir. Its permissions are deliberate (helper_binary.rs:63-70):

• .helpers-<uuid> — owner = process user, group = job user's group, mode 0o750 (group r-x, no w)
• .helpers-<uuid>/h-<uuid> — same owner/group, mode 0o750

The working directory itself is 0o770 with the job group (tempdir.rs:175-189), so the job user can unlink entries directly under it. But rm -rf .helpers-<uuid> must first unlink h-<uuid> from inside .helpers-<uuid>, and that requires write on that directory — which the job user does not have, by design. The comment at helper_binary.rs:30-32 states the intent explicitly: "the job user can traverse and read ... but cannot write or modify files."

So the sequence at cleanup is:

1. read_dir returns .helpers-<uuid> among the entries (nothing deletes it — helper.shutdown() at line 1054 only stops the child process; no code removes the helpers dir).
2. sudo -u <job> /usr/bin/rm -rf -- ... .helpers-<uuid> hits EACCES unlinking h-<uuid>, cannot then rmdir the non-empty directory, and exits 1 (POSIX: rm "shall exit with a non-zero status" when a file cannot be removed; -f suppresses only nonexistent-operand errors, not EACCES).
3. status.success() is false → this arm logs Cross-user cleanup of <wd> exited exit status: 1; files owned by the job user may remain.
4. remove_dir_all below, running as the process user, removes .helpers-<uuid> without difficulty — it owns it, mode 0o750 gives owner rwx.

Two consequences:

The warning is a guaranteed false positive. It routes to the worker agent log (that is the stated point of adding PROCESS_CONTROL), on every cross-user session, describing a leak that did not happen. A warning that fires unconditionally is one operators learn to filter, which costs the real signal this change exists to create — the sudoers-denial and rm-refusal cases the comment above names.

The message misattributes ownership. The one file that actually resisted removal is owned by the process user, not the job user, and it is removed seconds later. "Files owned by the job user may remain" points an operator at chown/sudoers when nothing is wrong.

This is distinct from the discarded-stderr point on the previous revision: there the concern was that a real failure could not be diagnosed; here the check itself is miscalibrated, and no amount of stderr capture fixes a warning that always fires.

The straightforward fix is to exclude the helpers directory from the operand list, since it is not job-owned and the process user handles it:

let helpers_dir = self.cross_user.helpers_dir.clone();
let files: Vec<String> = entries
    .filter_map(|e| e.ok())
    .map(|e| e.path())
    .filter(|p| helpers_dir.as_deref() != Some(p.as_path()))
    .map(|p| p.to_string_lossy().to_string())
    .collect();

self.cross_user.helpers_dir is already stored (line 467) and is an absolute path under the working directory, so the comparison is exact. With it excluded, a nonzero exit means what this arm claims it means.

Worth a cross-user integration test asserting the clean path logs no warning, since that is the property that broke here and no unit test over cross_user_cleanup_args can see it.

[leongdl]

Fixed, and this is the best finding of the round -- a warning that always fires is worse than no warning, and I would not have caught it from unit tests.

Verified the permission reasoning in the source rather than taking it on trust: create_helpers_dir chowns .helpers-<uuid> to the job user's group and then sets 0o750, so the group gets r-x with no w. The job user therefore cannot unlink the binary inside it, rm -rf cannot rmdir a non-empty directory, and -f suppresses missing operands rather than EACCES. Nothing removes the helpers directory before cleanup, so this was reachable on every cross-user session.

Took your patch: self.cross_user.helpers_dir is filtered out of the operand list, with the reasoning recorded inline including the misattribution point -- the one entry that resisted removal is owned by the process user, so blaming job-user ownership pointed operators at chown and sudoers for a non-problem. The remove_dir_all fallback removes it as before.

Agreed a cross-user integration test asserting the clean path logs no warning is the right place to pin this; the existing Docker legs pass but do not assert on log content, so that is filed rather than faked here.

[github-actions]

This paragraph adds a rm sentence to the spawn section, but the section that actually documents cleanup — "### Cleanup" at line 145-151 — still describes the pre-PR behaviour, and this PR changed it in two ways it does not mention:

`Session::cleanup()` for cross-user sessions:
1. `sudo rm -rf <working_dir>` as the session user — removes files owned by that user
2. `std::fs::remove_dir_all()` as the process user — removes any remaining files

Both lines of step 1 are now wrong:

• -i is gone. That is the load-bearing change of this revision — cross_user_cleanup_args carries a doc-comment explaining that a login shell would re-expand job-chosen filenames and make rm's exit 0 unreliable. The spec never mentioned -i (so it reads as accidentally-correct now), but it also does not record why the direct exec matters, which is exactly the kind of rationale that gets undone by a well-meaning future edit adding -i back for "consistency with the helper spawn" — which genuinely does use -i (line 63).
• The operand is not <working_dir>. The code passes the individual read_dir entries, not the directory itself (session.rs:1069-1075), and always has. A reader taking the spec literally would expect one operand and a recursive delete of the parent; the actual argv is -u <user> <rm> -rf -- <entry1> <entry2> ....

Neither is captured by the sentence added here, which only says the binary is resolved rather than found on PATH. The exit-status inspection added in this PR is also absent — the spec still implies cleanup failures are unobserved, when the whole point of the session.rs change is that they now produce a PROCESS_CONTROL warning. logging.md got a row for those records, so a reader of logging.md learns they exist while a reader of cross-user.md — the document about this subsystem — does not.

AGENTS.md:123 asks for spec and code to line up in the same commit, and architecture.md/logging.md were both updated here, so cross-user.md's cleanup section is the one that got missed. Rewriting step 1 covers it:

1. `<sudo> -u <user> <rm> -rf -- <entry>...` as the session user, over the entries
   of `<working_dir>` rather than the directory itself. Deliberately no `-i`: sudo
   would otherwise hand the concatenated argv to the job user's login shell, which
   re-expands `$`-containing job-chosen filenames and makes a zero exit status
   meaningless. Both binaries are resolved by `system_commands.rs`; if either is
   absent the step is skipped with a warning. A nonzero exit is logged
   (`FILE_PATH | PROCESS_CONTROL`) rather than discarded.

[leongdl]

Deferred. The Cleanup section is stale in the way you describe, and the spawn section is not where a reader looks for cleanup mechanics.

Not restructuring the spec section in this change: it is documentation drift that predates the resolver, and the load-bearing facts for this PR -- no login shell, resolved paths, what is logged and where -- are now correct in system_commands.rs, logging.md and the spawn section. Recorded as a follow-up.

[github-actions]

Dropping -i is right for the reasons given, but it is also a behaviour change that can break existing deployments, and nothing in the PR flags it as one.

sudo -i <cmd> and sudo <cmd> are matched differently by sudoers, and by the operator-facing configuration in two ways:

1. secure_path no longer supplies PATH. With -i the login shell is what ran, so rm executed with the job user's login environment. Without -i, sudo execs rm with the environment sudo constructs — and on a host where secure_path is unset and env_reset is on (the default on most distros), the child gets a minimal environment. That is fine for the absolute rm resolved here, so this direction is actually safer — but it is the mechanism by which the next point bites.

2. NOPASSWD / command-spec rules keyed on the login shell stop matching. A site that deployed the agent against the previous code and locked sudoers down to what it observed would have written something like

agentuser ALL=(jobusers) NOPASSWD: /bin/bash -c *

or restricted !requiretty / use_pty behaviour around a login shell. With -i removed, sudo no longer invokes the shell, so a rule scoped to the shell path no longer matches the request and cleanup fails with Sorry, user agentuser is not allowed to execute /usr/bin/rm as jobuser — every session, on a host that worked before the upgrade. The Ok(status) => arm added below now logs that, which is good, but the failure itself is new.

This is the second sudoers-visible change in the same PR, and the two point opposite ways: cross_user_helper.rs:286 now invokes /usr/bin/sudo by absolute path (harmless — sudo matches on the target command, not on how sudo itself was invoked), while this one changes the target command from the login shell to /usr/bin/rm. A site with a command-restricted sudoers rule is affected by the second and not the first.

The remediation is trivial (agentuser ALL=(jobusers) NOPASSWD: /usr/bin/rm, /bin/rm alongside the helper-binary rule), but only if the operator is told. Two things would cover it:

• A note in specs/sessions/cross-user.md. That file already discusses sudoers requirements, and cross-user-testing.md:8 mentions "passwordless sudo" as a prerequisite — the required command specs are the part that changes here.
• A CHANGELOG/PR-description line under a breaking-ish or operator-action heading, since this is not visible from reading the diff's Rust.

The -i removal itself is the right call; this is about surfacing its blast radius rather than reversing it. Worth noting explicitly that the helper spawn at cross_user_helper.rs:290 keeps -i — it needs the login environment for the job action — so the two invocations now differ in this respect, which is worth one sentence in cross_user_cleanup_args' doc so the asymmetry reads as deliberate rather than as an oversight in one of the two.

[leongdl]

Fair, and the operational half is worth carrying even though I am not reverting: a sudoers rule written against the -i form would stop matching. Weighing that against what -i was doing -- handing job-chosen filenames to a login shell that expands $, so the status check could not see a real cleanup failure -- I would rather break a narrow sudoers pattern loudly than keep a silent leak.

Noted in the PR description so it is not discovered at deploy time.

Process note, applying to the rest of this round: this is the fifth review pass, every finding in it is on a line an earlier pass asked me to change, and one of them was already stale when posted (the public-constant comment -- it was renamed to _VFS_TERMINATE_WAIT_SECONDS before that review ran). I am stopping here rather than continuing to iterate inside a PATH-resolution fix. Deferred items are recorded with the mechanism, the blast radius and what a fix would involve, so none of them is lost.

[seant-aws]

Security Review — openjd-rs PR #326

Fix: Resolve sudo from trusted directories to prevent PATH injection (Rust implementation)
Reviewer: seant-aws
Verdict: ✅ Approve

Summary

Command::new("sudo") resolved a bare name through the session process's own PATH. While the job cannot directly influence this PATH (unlike the Python sibling where job env merges over the parent's), the safety rests on unstated invariants: that no caller ever adds an environment to the helper's Command, and that the agent's own PATH is trustworthy. This PR introduces system_commands.rs (pub(crate), cfg(unix)) which resolves commands against a fixed ordered list of trusted absolute directories. PATH is never consulted. Additionally, sudo -i is removed from cleanup to prevent shell expansion of job-chosen filenames containing $.

Threat Model Distinction

This is hardening, not a live exploit fix for the Rust crate. sudo is spawned once at helper startup before any job action exists, and per-action environments travel over the helper's stdin protocol. The Python sibling has the live vulnerability (job env merged over parent's at the Popen call that launches sudo).

Commands Migrated

Command	File	Resolution Strategy
sudo (helper spawn)	cross_user_helper.rs	system_command_path() — resolved once, binding reused in Command and error message
sudo (cleanup)	session.rs	find_system_command() — skips with warning if absent
rm (cleanup)	session.rs	find_system_command() — skips with warning if absent; runs as job user

Bypass Analysis

Attack Vector	Defence	Test Coverage
../sudo (traversal)	/ rejected in is_bare_command_name	escaping_name_is_rejected_even_though_the_target_exists
..\\sudo (Windows-style)	\\ rejected	rejects_names_containing_a_path_component
Job poisons PATH	PATH never read; source-level lint + behavioural test	production_source_contains_no_environment_lookup + a_command_present_only_outside_the_trusted_list_is_not_found
Resolver silently falls back to bare name	Returns io::ErrorKind::NotFound	missing_command_is_an_error_and_not_the_bare_name
Non-executable file in trusted dir	is_file() + mode() & 0o111 check	ignores_a_non_executable_file
NixOS non-setuid sudo shadows setuid wrapper	Wrapper dir ordered before system profile	setuid_wrapper_precedes_the_nixos_system_profile
$HOME expansion via -i in cleanup	-i removed; sudo execs rm directly	does_not_interpose_a_login_shell
Cleanup false-positive from .helpers-<uuid>	Helpers dir excluded from operands	Fixed in final revision
Drive-relative prefix (D:evil)	Deliberately omitted — cfg(unix), : is legal filename char with no Path::join special meaning	N/A (correct by design)

Mutation Verification

#	Mutation Applied	Killing Test
M1	Add PATH fallback after trusted scan	production_source_contains_no_environment_lookup
M2	Remove path-separator guard	escaping_name_is_rejected_even_though_the_target_exists
M3	Return Ok(PathBuf::from(name)) on miss	missing_command_is_an_error_and_not_the_bare_name
M4	Force exec-bit check true	ignores_a_non_executable_file
M5	Swap wrapper/profile order	setuid_wrapper_precedes_the_nixos_system_profile
M6	Drop BSD /usr/local dirs	bsd_local_directories_are_searched
M7	Drop NixOS sw/bin dir	the_two_nixos_directories_are_present_as_a_pair
M8	Re-add -i to cleanup argv	does_not_interpose_a_login_shell

All 8 mutations killed. Green baseline restored and verified by checksum after each.

Residual Attack Surface

Item	Severity	Mitigation	Action
No uid-0 ownership/writability check on directories	Informational	/usr/bin and /bin precede /usr/local/*; writing to any trusted dir requires root	Parked — MetadataExt::uid/mode approach documented
/run entries before /usr/bin in ordering	Low	Requires non-root-owned /run/wrappers/bin (systemd misconfiguration)	Parked — reorder + ownership check as joint follow-up
to_string_lossy on non-UTF-8 filenames → rm -rf exits 0 on lossy path	Low	Narrow precondition (non-UTF-8 bytes in filename); needs OsString signature change	Filed as follow-up
stderr not captured from cleanup rm	Informational	Exit status IS inspected; diagnostic gap only	Deferred — scope discipline
Test files still invoke bare sudo/kill	None	Not production code; not exploitable	Left alone to keep diff focused
Sudoers rules keyed on -i / login shell break	Operational	Fails loudly; documented in PR description as deliberate break	Operator communication

Conclusion

The fix is correct, complete for its stated scope, and every security property is falsifiably tested. The threat model is accurately stated (hardening, not a live exploit fix for this crate — distinguishing it from the Python sibling). The -i removal is a net security gain that closes a job-reachable silent-leak path. No bypass vectors identified. Low-severity residuals require preconditions that imply pre-existing root compromise or are diagnostic gaps tracked for follow-up.