fix: resolve system commands from trusted dirs to prevent PATH injection

[leongdl]

Problem

When a session runs an action as a different OS user, the argv it builds for its
own privileged helpers used bare command names:

command.extend(["sudo", "-u", user.user, "-i", "setsid", "-w"])

That argv is launched with an environment the job controls:

env = dict(os.environ)
env.update(**self._os_env_vars)   # job-supplied, includes PATH
popen_args["env"] = {k: v for k, v in env.items() if v is not None}

So sudo is resolved through a PATH the job wrote. A job that drops an
executable named sudo into a directory it prepends to PATH gets that
executable run at the session's privilege level rather than the job user's.

Fix

Commands are resolved by _system_commands.py against a fixed, ordered list of
trusted absolute directories. PATH is never consulted.

shutil.which() and command -v are deliberately not used — both resolve
through PATH, so either would reintroduce the vulnerability while appearing to
fix it. A trusted-directory scan is not the same operation as which.

Why a resolver rather than absolute-path literals

The first commit on this branch used literals (/usr/bin/sudo). That closes the
hole but is not portable — NixOS keeps the setuid sudo wrapper at
/run/wrappers/bin/sudo, so a literal would convert a security bug into a
cross-user execution failure. The directory list is ordered so wrapper
directories win where they exist.

Sites changed

sudo (3), setsid, kill, pgrep, and the pstree/ps debug dump. The debug
dump uses the non-raising variant so a developer aid does not fail on a host
without pstree.

pgrep was missed by the first pass

_linux/_sudo.py invoked pgrep unqualified. This is the macOS lookup of
sudo's child process, used to pick the target for cancelation signals — so a
substituted pgrep does not need to execute anything interesting, it only has to
return a false PID to redirect a SIGKILL.

Severity is lower than the reported sudo sites, and the reason is worth being
precise about: that call passes no env=, so the job's PATH does not reach it.
Real defect, same class, but not reachable by the reported vector.

Properties pinned, and mutation-checked

The failure mode of this fix is silence — a resolver that quietly falls back to
PATH looks identical to a working one from the outside, and the code still reads
as fixed. So every guarantee is falsifiable. Eight mutants, all caught, against a
green baseline with the source restored and verified by checksum:

#	Mutation	Caught by
M1	resolve via shutil.which	test_ignores_path_even_when_it_contains_a_matching_command ("PATH was consulted") + 2
M2	drop the path-separator guard	test_rejects_traversal_even_though_the_target_is_reachable + 3
M3	fall back to the bare name	test_raises_rather_than_returning_the_bare_name
M4	subclass FileNotFoundError	test_is_not_a_filenotfounderror
M5	search /usr/bin before the wrapper dir	test_searches_the_setuid_wrapper_directory_before_usr_bin
M6	drop /sbin	test_searches_both_sbin_locations
M7	ignore the execute bit	test_ignores_a_non_executable_file
M8	empty the directory list	the TestRealCommandsResolve controls

Three of those shaped the suite, and are worth flagging to a reviewer:

• M8 is why TestRealCommandsResolve exists. Every other test patches
  TRUSTED_SYSTEM_DIRECTORIES to a temp directory, so all of them would pass on a
  host where the genuine entries were empty or misspelled. Those controls assert
  against the real filesystem.
• M2 is why there are two traversal tests. The parametrized one does not catch
  the mutation alone: with the executable directly in the searched directory,
  ../name resolves to nothing either way, so it passes with the guard deleted.
  The nested-directory test reaches a real file through the traversal and asserts
  that precondition.
• M4 asserts the error is not an OSError subclass. The subprocess machinery
  catches OSError to mean "the thing I tried to launch is missing, carry on
  degraded"; an unavailable privileged helper must not be absorbed by that.

find_system_command is lru_cached, so an autouse fixture clears the cache
around each test — otherwise a result computed under one patched directory list
leaks into the next.

Verification

• 966 passed, 40 skipped, 16 xfailed, no failures
• black and mypy clean

The existing macOS argv test now patches the resolver rather than asserting
/usr/bin/sudo, which makes it host-independent and additionally proves the argv
goes through the resolver — a literal in the source would no longer match.

Companion PRs

The same defect class was fixed in the sibling codebases:

• openjd-rs — fix(sessions): resolve sudo from trusted dirs to prevent PATH injection openjd-rs#326
• deadline-cloud-worker-agent — fix: resolve privileged commands from trusted dirs to prevent PATH injection aws-deadline/deadline-cloud-worker-agent#1058
• deadline-cloud-job-attachments — fix: resolve sudo and findmnt from trusted dirs to prevent PATH injection aws-deadline/deadline-cloud-job-attachments#70

[github-actions]

SystemCommandNotFoundError not being an OSError regresses the cancelation path.

The docstring justifies the choice by saying callers "already catch OSError subclasses to mean the thing I tried to launch is missing, carry on degraded", and that this condition must not be absorbed. But at least one of those handlers is not a degrade-and-continue — it is the guard that keeps a cancel from unwinding:

_runner_base.py:1238-1249

try:
    if method == "notify":
        process.notify()
    else:
        process.terminate()
except OSError as err:  # pragma: nocover
    self._logger.warning(f"Cancelation could not send {method} signal ...")

Its own docstring states the intent explicitly: "a cancel path is the wrong place to raise: the caller is already unwinding a cancel, and losing the surrounding bookkeeping to a signal failure is worse than a warning."

terminate() -> _terminate_process() -> _posix_signal_subprocess(), which now calls system_command_path("sudo") (line 901) and system_command_path("kill") (line 956), and reaches find_child_process_id_pgrep -> system_command_path("pgrep") (_linux/_sudo.py:178). Before this PR each of those resolutions failed with FileNotFoundError out of run()/Popen — an OSError, so it was caught and logged. Now it is a SystemCommandNotFoundError, which escapes _notify_or_terminate and propagates into the cancel/timeout bookkeeping.

Same for the pgrep case in _linux/_sudo.py:178: find_sudo_child_process_group_id's retry loop only catches FindSignalTargetError, so the new exception escapes that function too.

If the intent is "cannot proceed safely, do not silently degrade", that still needs the raise to land somewhere that handles it — otherwise the failure mode moves from "cancel logged a warning" to "cancel aborted partway through". Worth either deriving from OSError, or adding an explicit handler in _notify_or_terminate / the _sudo.py retry loop.

[leongdl]

Fixed in 68265a3. You were right and my original reasoning was wrong: I asserted that an unavailable privileged helper must not be absorbed by OSError handlers without checking what those handlers actually do. _runner_base catches OSError around notify()/terminate() deliberately, and its own comment says a cancel path is the wrong place to raise. SystemCommandNotFoundError now derives from FileNotFoundError. The test that asserted the opposite has been inverted, since it was pinning the wrong decision.

[github-actions]

Eager resolution of sudo here aborts kills that would never have used it.

if not user.is_process_user():
    kill_cmd = [system_command_path("sudo"), "-u", user.user, "-i"]

This runs unconditionally, ~20 lines before the direct-signal attempt below:

with ctx_mgr as has_cap_kill:
    if has_cap_kill or not self._user or self._user.is_process_user():
        ...
        os.killpg(self._sudo_child_process_group_id, numeric_signal)
        ...
        else:
            return          # <-- sudo was never needed

On a Linux host that has CAP_KILL (the try_use_cap_kill() path), killpg succeeds and kill_cmd is discarded — but if sudo is not present in any trusted directory, system_command_path raises here first and the SIGKILL is never sent. Previously the same host got a successful killpg and returned, because the missing sudo only surfaced when run(kill_cmd) actually executed.

Combined with SystemCommandNotFoundError not being an OSError (see the other comment), this propagates out of terminate() rather than degrading.

Deferring the resolution to just before kill_cmd.extend([...]) at line ~955 — where kill is already resolved — keeps the previous ordering: only resolve the privileged helper on the path that actually uses it.

[leongdl]

Fixed in 68265a3. Resolution is now deferred to the branch that uses it, so the CAP_KILL direct-signal path no longer depends on sudo being present.

[github-actions]

/run/wrappers/bin is trusted first, and no candidate is checked for who owns it.

The trusted-directory list is the entire trust boundary of this module, but membership is decided purely by path string — _is_executable_file only checks isfile + X_OK. /usr/bin, /bin, /sbin are root-owned on any sane host, so that is fine for them. /run/wrappers/bin is different in kind: it is on a tmpfs that is populated at boot, and it is searched before /usr/bin for every name, including sudo.

On a NixOS host that is exactly right. On every other host the directory is expected not to exist — but nothing here enforces that expectation, so if anything on the host (a container image with a loose /run, a package, an operator script) creates /run/wrappers/bin/sudo, it silently takes precedence over /usr/bin/sudo and the module reports it as trusted. For a module whose stated purpose is removing an untrusted-search-path vulnerability, that is a search path with one entry whose trustworthiness is assumed rather than established.

Worth considering an os.stat-based check on the resolved candidate and its directory before returning it: owned by uid 0, and not group- or world-writable. That makes "trusted" a verified property rather than a property of the literal list, and it costs one stat on a path that is already being stated by isfile.

[leongdl]

Correct, and parked rather than fixed. Membership is decided by path string alone: _is_executable_file checks only isfile plus an execute bit. Parked because reaching the exposure needs root or equivalent already, and on a host where /run/wrappers/bin is root-owned an attacker who can write there can replace /usr/bin/sudo directly, which no check here would stop. The reason I am wary of the fix rather than just applying it: the X_OK check in this module already caused one real regression by testing permissions as the wrong user, and an ownership check has the same failure shape, refusing a binary that works. Recorded with the concrete os.stat approach so it is not lost.

[github-actions]

Negative results are cached for the life of the process.

lru_cache caches None as readily as it caches a hit, so a single lookup that lands in a window where the binary is absent poisons that command permanently. The realistic window is a package upgrade: dpkg/rpm unlink-and-replace /usr/bin/sudo, and this library runs inside a long-lived agent daemon. One unlucky find_system_command("sudo") during that window means every subsequent cross-user launch and every SIGKILL for the rest of the process lifetime raises SystemCommandNotFoundError, on a host where sudo is present and working.

The docstring justifies caching with "the filesystem layout does not change underneath a running session" — true for a hit (the resolved absolute path stays valid), but the absence of a file is not the same kind of stable fact.

Caching only successful lookups would keep the stated performance benefit (the hot paths — process launch, signal delivery — are all hits) while making a miss retry:

_cache: dict[str, str] = {}

def find_system_command(name: str) -> Optional[str]:
    _validate_command_name(name)
    if (cached := _cache.get(name)) is not None:
        return cached
    for directory in TRUSTED_SYSTEM_DIRECTORIES:
        candidate = os.path.join(directory, name)
        if _is_executable_file(candidate):
            _cache[name] = candidate
            return candidate
    return None

[leongdl]

Fixed in 3fcbe09, and thank you for this one, it was a genuine availability bug rather than a style point. lru_cache cached None as readily as a path, so a lookup landing in a package manager's unlink-relink window made one unlucky moment permanent for the life of the agent. Replaced with a dict that stores successful lookups only. Both halves are pinned, because "do not cache misses" could otherwise be satisfied by caching nothing at all and losing the reason the cache exists.

[github-actions]

Coverage gap: rm/powershell in the session-cleanup argv are still bare names.

The new module states the invariant absolutely — "never let a command name reach execvp-style resolution" — but one privileged argv is not converted. _session.py:580-589 builds the cleanup command that runs as the job user:

if is_posix():
    recursive_delete_cmd = ["rm", "-rf"]
else:
    recursive_delete_cmd = ["powershell", "-Command", "Remove-Item", "-Recurse", "-Force"]
...
_subprocess = LoggingSubprocess(args=recursive_delete_cmd + files, user=self._user, ...)

That reaches _start_subprocess, so on POSIX the argv becomes [<resolved sudo>, -u, user, -i, <resolved setsid>, -w, "rm", "-rf", ...] — the two helpers this PR hardened, followed by a bare rm that setsid resolves through execvp. In the same-user branch rm is argv[0] of the Popen itself. This is an rm -rf over the session directory, which makes it the highest-consequence bare name left in the tree.

Two mitigations apply, and both are worth stating explicitly rather than leaving implicit: sudo -i resets the environment (so the job's PATH does not survive into the login shell), and this LoggingSubprocess is constructed without os_env_vars, so the same-user branch inherits the agent's own PATH rather than the job's. So this is not the same directly-job-controlled path the module's docstring describes.

Still worth either resolving these two names too, or narrowing the module docstring's claim to the argv positions actually covered — as written, a future reader will reasonably conclude no bare command names remain on any privileged path.

[leongdl]

Correct, and parked as out of scope for this change. _session.py is not a file this PR touches, and your own two mitigations are the reason I am comfortable deferring: sudo -i resets the environment before the login shell resolves rm, and that LoggingSubprocess is constructed without os_env_vars. It should be routed through the same resolver in a separate change against _session.py, where it can be reviewed on its own terms.

[github-actions]

The docstring is candid that the three load-bearing properties are unpinned, which is right — but it means the diff ships a security fix whose central guarantee has no falsifiable coverage, and the only new test is the argv-shape assertion in test_subprocess.py:1311. That one pins that _start_subprocess calls some resolver (it patches system_command_path out), not that the resolver ignores PATH.

Three cheap tests would cover all of it, no privileged host needed:

1. PATH is ignored — drop a fake sudo in tmp_path, monkeypatch.setenv("PATH", str(tmp_path)), cache_clear(), assert the result is not the fake.
2. Only trusted dirs are returned — patch TRUSTED_SYSTEM_DIRECTORIES to (str(tmp_path),), assert the returned path is under it.
3. A miss raises — patch to an empty tuple, assert SystemCommandNotFoundError, and assert it is not an OSError if that non-inheritance is intentional (see my other comment).

Also: the warning points at "§5.5 of the PATH-injection analysis", which is not an artifact in this repo. An external-only pointer will be dead weight to anyone reading this file later — worth inlining the property table or dropping the reference.

[leongdl]

Fixed in 1839df4, which added test/openjd/sessions_v0/test_system_commands.py. Your read of the earlier state was right: patching system_command_path out pinned argv shape and nothing about PATH-independence. The suite now covers all three of your suggestions, and every property is mutation-checked, including the empty-directory-list case which survived the first pass and is why real-filesystem positive controls exist. The cross-repository pointer you flagged has also been removed from the docstring.

[github-actions]

TestRealCommandsResolve (and test_ignores_a_non_executable_file) will fail on the windows-latest CI leg.

.github/workflows/code_quality.yml:20 runs the matrix over [ubuntu-latest, windows-latest, macos-latest], and this file has no pytestmark / skipif guard. The module docstring itself states the expected Windows behaviour — "On Windows nothing will be found and the lookups raise" — which is precisely what breaks these two:

• test_a_universally_present_command_resolves[sh] / [ls]: system_command_path("sh") scans /run/wrappers/bin, /usr/bin, ... none of which exist on Windows, so it raises SystemCommandNotFoundError instead of returning a path.
• test_sudo_resolves_on_this_host is fine (it skips via find_system_command("sudo") is None), but the two above have no such escape hatch.
• test_ignores_a_non_executable_file fails for a different reason: on Windows os.access(path, os.X_OK) returns True for any existing readable file, so _is_executable_file reports the non-executable target-cmd as executable and find_system_command returns it rather than None.

The sibling POSIX-only suite already establishes the convention — test_linux_sudo.py:167 uses @pytest.mark.skipif(not is_posix(), reason="pgrep and process groups are posix-only"). Applying the same guard to TestRealCommandsResolve and to test_ignores_a_non_executable_file (the latter because the X_OK semantics, not the trusted directories, are what differ) would keep the positive controls doing their job on the platforms where they mean something.

[leongdl]

Fixed in 68265a3, with the POSIX guard following the test_linux_sudo.py convention you cited. Worth noting the same two tests failed exactly as you predicted on the sibling job-attachments PR before I got to them, so this was a prediction rather than a hypothetical.

[github-actions]

Resolving kill turns a shell builtin into a hard dependency on kill(1) being installed.

In the cross-user branch this argv is executed as sudo -u <user> -i kill -s kill -- -<pgid>. sudo -i simulates an initial login: it runs the target user's login shell and, when a command is given, passes it to that shell via -c. So kill was resolved by the shell, and kill is a builtin in bash/dash/sh — no kill(1) binary was ever required for this path, and there was nothing for a job-controlled PATH to hijack either (-i resets the environment, and a builtin is not looked up on PATH at all).

After this change the same operation requires a real executable at /run/wrappers/bin, /usr/bin, /bin, /usr/sbin, or /sbin. On Debian-family images kill(1) ships in procps, which the -slim base images do not install — that is a common base for containerized workers. On such a host every cross-user SIGKILL fallback now raises SystemCommandNotFoundError from this line instead of succeeding via the builtin. The same-user branch is a smaller change in kind (it was already an execvp of a bare name from run()), but it gains the same new failure mode.

Two options, depending on which property you want to keep:

• Leave kill as a bare name in the cross-user branch and note in a comment that sudo -i resolves it as a shell builtin with a reset environment, so CWE-426 does not apply to that position; resolve it only in the same-user branch, where Popen really does execvp.
• Or keep the resolution and use find_system_command("kill"), falling back to the bare name specifically for the sudo -i case where the builtin is guaranteed.

Either way it is worth confirming kill(1) is present on the base images the agent ships on before this lands, since the failure surfaces only on the cancel/terminate path.

[leongdl]

Fixed in 68265a3. This was the sharpest catch in the batch. sudo -i hands the command to the target user's login shell, where kill is a builtin, so nothing was ever PATH-resolved there and resolving it invented a hard dependency on procps that Debian -slim images do not carry. The cross-user branch leaves it bare with a comment explaining why; the same-user branch still resolves it, because there run() really does execvp a bare name. Both directions are now pinned, since either is easy to tidy into a bug.

[github-actions]

The NixOS case the ordering comment is written for still cannot launch a cross-user session, because setsid is not reachable from this list.

The comment justifies putting /run/wrappers/bin first specifically so that NixOS's setuid sudo wrapper wins. But on NixOS the trusted list resolves only sudo:

• /run/wrappers/bin contains just the setuid/setcap wrappers (sudo, su, ping, mount, ...) — not setsid, kill, or pgrep.
• /usr/bin contains a single file, /usr/bin/env.
• /bin contains a single file, /bin/sh.
• /usr/sbin and /sbin do not exist.

Everything else lives under /run/current-system/sw/bin (a symlink farm into /nix/store), which is not in TRUSTED_SYSTEM_DIRECTORIES. So on the exact platform this ordering exists to support, _subprocess.py:610 — system_command_path("setsid"), on the non-macOS cross-user branch immediately after the resolved sudo — raises SystemCommandNotFoundError, and _start_subprocess swallows it into "Process failed to start". Before this PR, setsid was a bare name resolved by the login shell that sudo -i starts, whose PATH includes /run/current-system/sw/bin, so it worked.

Adding /run/current-system/sw/bin to the list would make the NixOS support the comment claims actually hold. It is a root-owned symlink farm managed by nixos-rebuild, so it is trust-equivalent to /usr/bin on that platform, and it costs one stat on hosts where it does not exist — the same argument the comment already makes for /run/wrappers/bin. Worth also adding a TestTrustedDirectories assertion for it so the pairing is pinned: the wrapper entry without the sw entry supports no complete code path.

[leongdl]

Fixed in 68265a3 by adding /run/current-system/sw/bin. You were right that the ordering comment described support that did not exist: the wrapper directory resolved sudo and nothing else, so a cross-user session failed on setsid one line later. A test asserts the two entries as a pair so the combination cannot be half-removed.

[github-actions]

The ordering invariant that matters for the NixOS sudo wrapper is now unpinned, and /run/current-system/sw/bin is the reason.

Adding /run/current-system/sw/bin is the right call — without it setsid/pgrep were unreachable on NixOS. But it also contains a sudo, and that changes which comparison the ordering test needs to make.

On NixOS both of these exist:

• /run/wrappers/bin/sudo — the setuid wrapper, the only one that works.
• /run/current-system/sw/bin/sudo — a symlink into /nix/store. Store paths cannot carry the setuid bit, so this binary is mode 0555, uid 0, not setuid. Running it fails with sudo: must be owned by uid 0 and have the setuid bit set.

_is_executable_file only checks isfile + X_OK, both of which the non-setuid copy satisfies, so whichever of the two directories comes first in the tuple is what system_command_path("sudo") returns. The current order is correct. What is not covered is that it stays correct: TestTrustedDirectories.test_searches_the_setuid_wrapper_directory_before_usr_bin (test file line 227) asserts only that /run/wrappers/bin precedes /usr/bin — and /run/current-system/sw/bin now sits between those two. Swapping the two /run entries is a plausible edit, since the comment presents them as "a pair" and no test distinguishes them; it keeps that assertion green while making every cross-user session on NixOS fail at launch with the setuid error. test_the_two_nixos_entries_are_present_as_a_pair only asserts membership, not order.

The comment on line 58 also under-describes the entry: it explains why the directory is needed (setsid, pgrep) but not that it also shadows the one command whose resolution has a setuid precondition.

Suggest tightening the assertion to the invariant that has a failure mode — assert /run/wrappers/bin precedes both /run/current-system/sw/bin and /usr/bin, with the store-path/setuid reason in the docstring so a future reader does not reorder them.

(This is also the concrete case for the ownership/mode check raised on the earlier revision: sudo is a command where "executable file in a trusted directory" is genuinely not sufficient, and here the insufficiency lands on a supported platform rather than a hypothetical one.)

[leongdl]

Fixed in a144881. Correct that the existing assertion did not constrain the new entry, because sw/bin sits between the wrapper directory and /usr/bin. There is now a direct assertion that the wrapper precedes the symlink farm, mutation-checked by swapping the two entries, which fails that test and nothing else.