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

[leongdl]

Problem

The agent invoked sudo, pkill and shutdown by bare name, so each was
resolved through PATH. Where any part of that search path is influenced by
less-trusted input, the resolution itself is the vulnerability (CWE-426,
Untrusted Search Path) — and the agent runs these as a privileged user.

Companion to HackerOne 3942741, reported against openjd-sessions-for-python.

Fix

New _system_commands module resolves a bare command name against a fixed,
ordered list of trusted absolute directories. PATH is never consulted, and
neither is shutil.which — it resolves through PATH, so it would reintroduce
the problem while appearing to fix it.

This also fixes two defects in the obvious alternative

Hardcoding absolute literals closes the hole but breaks hosts:

• /usr/sbin/shutdown is wrong on non-usr-merged Debian, where shutdown
  exists only at /sbin/shutdown. A literal there trades a security bug for a
  host that will not shut down on stop. Both sbin directories are now searched.
• The Windows branch still invoked a bare shutdown and is now resolved from
  System32 under SystemRoot.

/usr/bin/sudo is likewise not universal — NixOS keeps the setuid wrapper at
/run/wrappers/bin/sudo, so that directory is searched first.

Properties pinned, and mutation-checked

Each mutation was applied to the production source, the suite run, and the source
restored and verified by checksum. Baseline green before each.

#	Mutation	Caught by
M1	resolve via shutil.which	test_ignores_path_even_when_it_contains_a_matching_command + 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_system_command_path_raises_rather_than_returning_the_bare_name
M4	subclass FileNotFoundError	test_is_not_a_filenotfounderror
M5	search /usr/bin before the wrapper dir	test_posix_searches_the_setuid_wrapper_directory_before_usr_bin
M6	drop /sbin	test_posix_searches_both_sbin_locations

M4 is worth calling out: callers around subprocess catch FileNotFoundError to
mean "optional tool absent, carry on degraded". An unavailable privileged helper
must not be absorbed by that handling, so SystemCommandNotFoundError
deliberately does not inherit from it.

M2 is why there are two traversal tests. The straightforward one does not catch
the mutation on its own — with the executable directly in the searched directory,
../name resolves to nothing either way — so
test_rejects_traversal_even_though_the_target_is_reachable nests the directory
so the traversal reaches a real file, and asserts that precondition.

Test changes

Existing tests that asserted command locations now stub the resolver. Those cases
simulate a platform via sys.platform, so a real lookup would resolve against
whatever platform the suite is actually running on. Stubbing also means a literal
reappearing in the source fails the assertion.

Verification

• hatch run fmt clean
• hatch run lint clean (ruff, ruff format, mypy — 200 source files)
• hatch run test: 3077 passed, 48 skipped

One caveat, stated plainly: across four full-suite runs, one run had
test/unit/test_session_events.py::test_log_before_call[DeleteWorkerBeforeCallTest]
fail. It passes in isolation (27/27, repeatedly) and the other three full runs
were clean. The suite uses pytest-randomly, and that test has no relationship
to the files changed here, so I believe it is a pre-existing order-dependent
flake — but I was not able to prove it pre-existing, so I am flagging rather than
asserting it.

The Windows shutdown path is unverified. I have no Windows host; that branch
is covered by a unit test of the directory layout only, not by execution.

Scope note: pkill and shutdown were not part of the reported defect

Raised in review and worth stating plainly.

session_cleanup.py already used the absolute literal /usr/bin/pkill, so routing
it through the resolver removes no exposure. It is here for consistency, so that one
mechanism owns every privileged command name in the package, and so a later edit
cannot reintroduce a bare name unnoticed.

shutdown is the opposite case: it is now deliberately not resolved. Its path is
a contract with the sudoers rule the installer writes, and a trusted-directory search
can legitimately return a path that rule does not grant. A unit test reads the rule
out of each installer and compares the full granted argv against what the agent
builds, so drift on either side fails the build instead of producing a password
prompt at shutdown time.

The only site in this package where a bare name was resolved through PATH at
privilege is sudo.

[github-actions]

Passing an agent-resolved absolute path to sudo couples this resolver to the NOPASSWD sudoers rule the installer writes — worth an end-to-end check on --allow-shutdown hosts.

Previously the argv was sudo shutdown now, and sudo resolved shutdown itself using secure_path, which is what matched the rule install.sh:342 writes:

${wa_user} ALL=(root) NOPASSWD: /usr/sbin/shutdown now

Now the agent picks the path, using the order /run/wrappers/bin, /usr/bin, /bin, /usr/sbin, /sbin. On usr-merged distributions where sbin has been folded into bin (Fedora 42+, Arch) /usr/bin/shutdown is found first, so the argv becomes sudo /usr/bin/shutdown now rather than the /usr/sbin/shutdown now in the rule.

To be fair to the change: sudo compares the device and inode of the submitted command against the sudoers command spec, so a /usr/bin vs /usr/sbin difference that is the same file via a symlinked directory should still match. The risk is narrower than the string difference suggests — but it is now a property of two independently maintained lists (this resolver's search order, and the literal in each installer script) rather than of sudo alone, and a silent non-match means sudo prompts for a password and the shutdown fails. On an autoscaled fleet that surfaces as a host that never terminates and keeps heartbeating in STOPPING.

Concretely worth doing:

• Verify a shutdown end-to-end on a host installed with --allow-shutdown, on at least one usr-merged and one non-usr-merged layout, since unit tests stub the resolver and cannot catch a sudoers mismatch.
• install_macos.sh:604-608 now documents the old contract and is stale: it states "sudo resolves shutdown to /sbin/shutdown via PATH ... The sudoers command MUST continue to match that argv exactly." That comment is the natural place for a future reader to check this invariant, so it should describe the new mechanism.

[leongdl]

Fixed in 10b6772, by removing the coupling rather than testing around it. shutdown is no longer resolved at all: its path is a contract with the sudoers rule the installer writes, so it is now the module constant LINUX_SHUTDOWN_PATH, and only sudo is resolved. Your point that a silent non-match leaves the host heartbeating in STOPPING is what made this a must-fix rather than a nit. The end-to-end check across both distribution layouts is parked, since I have no non-usr-merged host to run it on and an unexecuted test would be worse than none. The static half is covered: a unit test reads the rule and compares the full granted argv against what the agent builds.

[github-actions]

_validate_command_name blocks path separators but not a Windows drive-relative prefix, so the "only paths under the trusted directories are returned" invariant can be broken on win32.

os.path.join discards the left operand when the right operand carries a drive letter:

ntpath.join(r"C:\Windows\System32", "D:evil")  # -> "D:evil"
ntpath.join(r"C:\Windows\System32", "C:evil")  # -> "C:evil"

Neither D:evil nor C:evil contains / or \\, so both pass validation, and the resulting candidate resolves against that drive's per-drive current directory — outside every trusted directory. find_system_command would then return it.

Not currently reachable (every call site passes a literal), but this module's stated contract is that the returned path is always under a trusted directory, and the docstring calls that property "load-bearing". A cheap way to enforce it regardless of name shape is to verify the result after joining, e.g. reject when os.path.dirname(os.path.abspath(candidate)) is not the trusted directory, or reject names for which os.path.splitdrive(name)[0] is non-empty.

[leongdl]

Fixed in 10b6772. Verified your example before changing anything: ntpath.join(r"C:\Windows\System32", "D:evil") really does return "D:evil", so a drive-relative name escaped every trusted directory while containing no separator. The validator now rejects : as well, with D:evil and C:evil in the parametrized rejection cases.

[github-actions]

_is_executable_file checks X_OK as the agent user, but the commands resolved for sudo are executed as root — so a binary that is deliberately not agent-executable now resolves to None and the operation fails outright.

shutdown is the case that matters. On hardened hosts /sbin/shutdown (or /usr/sbin/shutdown) is not uncommonly mode 0750 root:root, and /usr/sbin itself is sometimes not traversable by non-root users. Before this change the agent never needed to see the binary: it passed the bare name and sudo — already root — did the resolution. Now find_system_command("shutdown") returns None for the unprivileged agent user, system_command_path raises SystemCommandNotFoundError, and _host_shutdown raises before reaching Popen.

That exception is not caught. Both call sites in startup/entrypoint.py (lines 275-276 and 560-561) invoke _host_shutdown inside a while _repeatedly_attempt_host_shutdown(): loop with no handler, so the error unwinds to the top-level except Exception in entrypoint, which logs and sys.exit(1)s. For a service-requested shutdown that means the agent exits instead of retrying, the host never powers off, and the loop's explicit design goal — keep heartbeating in STOPPING until the host goes down — is lost.

Two things worth separating here:

1. For a command that will be run via sudo, existence (os.path.exists) is the appropriate check, not the calling user's X_OK.
2. Regardless of the check, _host_shutdown should probably catch SystemCommandNotFoundError and log rather than let it escape the retry loop, since raising is strictly worse than the previous behaviour of letting sudo try.

[leongdl]

Partly fixed in 9f4a61b, and the part I did not fix is now moot. _host_shutdown no longer lets the error escape: it logs and returns, because callers treat it as best-effort and retry until the host goes down, so unwinding to the top-level handler ended the retrying and left the host up. The X_OK-as-agent-user concern was specifically about shutdown, which is no longer resolved, so the remaining resolved commands are sudo, pkill and shutdown.exe, all 0755 and exec'd by the agent itself.

[github-actions]

This one was already immune to the PATH problem — it was the absolute literal /usr/bin/pkill — so replacing it with the resolver does not remove a CWE-426 exposure, it only changes which absolute path is submitted.

The change is not neutral, though: the resolver searches /run/wrappers/bin first, so on NixOS this now yields /run/wrappers/bin/pkill where it previously yielded /usr/bin/pkill. Since this runs under sudo -u <job-user>, any operator sudoers rule that permits the old literal would stop matching, and sudo would prompt for a password — leaving job-user processes alive after a session ends.

Note also that system_command_path is evaluated inside the try at line 107, whose only handler is except subprocess.CalledProcessError. A SystemCommandNotFoundError therefore propagates past it. It is ultimately swallowed by the broad except Exception in _cleanup_session_user (line 81), which downgrades a "cannot clean up job-user processes at all" condition to a single logger.warn — the same treatment a transient failure gets.

If the goal is uniformity of style rather than a fix, that is reasonable, but it would be worth calling out in the PR description that pkill was not part of the vulnerability, so a reviewer can weigh the NixOS argv change against a benefit that is cosmetic here.

[leongdl]

Correct on both counts about scope, and now stated in the PR description: pkill was already the absolute literal /usr/bin/pkill, so routing it through the resolver removes no exposure. One part of the comment is wrong, though, and I want it on the record rather than silently ignored: the resolver will not yield /run/wrappers/bin/pkill on NixOS. That directory holds only setuid and setcap wrappers, and pkill is not among them. With /run/current-system/sw/bin in the list it resolves from the symlink farm as expected.

[github-actions]

system_command_path can raise SystemCommandNotFoundError, and nothing in install() catches it — the surrounding try (line 162) only wraps run(...) and only handles CalledProcessError. So on a host where sudo is not in a trusted directory, the installer dies with a raw traceback rather than the actionable message the rest of this function is careful to produce (print(f"ERROR: {e}") / sys.exit(1) on the Windows path).

install.sh already handles this case with a clear message ("ERROR: sudo is not installed but is a required dependency of the worker agent.", line 263-264), but that check runs inside the script this line is trying to launch, so it can no longer be reached. Catching the error here and printing the same style of message would keep the diagnostic.

[leongdl]

Fixed in 10b6772. Resolution happens before the argv is built and is now caught there, printing the actionable message and exiting 1, which matches what the Windows branch already does a few lines up. Your observation that install.sh's own "sudo is not installed" message is unreachable because it lives inside the script this line launches is what made the fix worth doing rather than deferring to the script.

[github-actions]

find_system_command and SystemCommandNotFoundError are exported in __all__ but no production code calls or catches either — the only callers of find_system_command are in test/unit/test_system_commands.py, and every call site in this PR uses system_command_path.

That leaves a public "the absence is tolerable" entry point with no user, and the carefully argued design decision in SystemCommandNotFoundError's docstring (deliberately not a FileNotFoundError so degraded-mode handlers cannot absorb it) has nothing to protect, because nothing catches it anywhere. Given the module is private (_system_commands), it may be simpler to keep only system_command_path and add find_system_command when a tolerant caller actually appears.

[leongdl]

Fixed in 9f4a61b by dropping find_system_command from __all__. You were right that every caller in this package needs the command it asks for, so none can do anything useful with None. It stays defined because the tests use it to exercise the search without asserting on an exception, but it no longer advertises a tolerate-absence entry point that nothing here wants.

[github-actions]

test_shutdown_is_not_looked_up_in_the_trusted_directories has no skipif guard, but it asserts on POSIX-only entries — so it raises ValueError on Windows, where the unit-test matrix runs.

trusted_directories() returns a one-element tuple on win32 (C:\Windows\System32), so:

directories.index("/usr/bin")   # ValueError: "/usr/bin" is not in list

Every other POSIX-layout assertion in this file is guarded (test_posix_searches_the_setuid_wrapper_directory_before_usr_bin, test_posix_has_the_two_nixos_entries_as_a_pair, test_posix_searches_both_sbin_locations all carry @pytest.mark.skipif(sys.platform == "win32", ...)), and this one makes the same kind of claim about the same list but is missing the marker. code_quality.yml runs UnitTest on windows-latest across all five Python versions, so this fails there rather than being a latent nit.

The second assertion on the next line (LINUX_SHUTDOWN_PATH.startswith("/usr/sbin/")) is platform-independent and would be fine to keep unguarded, so the minimal fix is either the same skipif on the whole test, or splitting the directories.index half out under a guard.

[leongdl]

Fixed in a37ad17. Correct that this was a real failure rather than a nit, and it did fail the windows-latest 3.13 leg before I got to it. Guarded with the same win32 skipif the sibling POSIX-layout tests carry.

[github-actions]

MACOS_SHUTDOWN_PATH is the same sudoers contract as LINUX_SHUTDOWN_PATH, but nothing pins it to the rule install_macos.sh writes.

install_macos.sh:618 grants:

${wa_user} ALL=(root) NOPASSWD: /sbin/shutdown -h now

and the agent now invokes [sudo, /sbin/shutdown, -h, now], so today they agree. But TestShutdownPathIsASudoersContract._granted_shutdown_path reads only installer/install.sh, and its regex (NOPASSWD:\s*(\S+)\s+now) is Linux-shaped — it would not match the macOS rule's -h now even if pointed at that file. The only macOS assertion is test_macos_shutdown_path_exists_on_this_host, which checks the file exists on the runner, not that it matches the grant.

So the class docstring's claim that the pairing "cannot drift apart silently" holds for Linux only. If someone edits the macOS rule (or the -h flag) the constant here silently stops matching, and the failure mode is the one the docstring describes: sudo prompts for a password and shutdown-on-stop fails — on the platform where the argv has two parts to keep in sync rather than one.

Extending the helper to take a script path plus an expected-args suffix, and adding a darwin case against install_macos.sh, would cover both with the same mechanism.

Separately, install_macos.sh:606-607 is now stale as a result of this change — it says the agent invokes sudo shutdown -h now and that "sudo resolves shutdown to /sbin/shutdown via PATH". After this PR the agent passes the absolute path itself, which is worth correcting since that comment is the thing a future reader would consult before editing the rule.

[leongdl]

Fixed in a37ad17 and 9f4a61b. You were right that the docstring's "cannot drift apart silently" held for Linux only: the regex was Linux-shaped and would not have matched install_macos.sh's -h now. The contract test is now parametrized over both installers and reads each rule from its own script. It also compares the full argv rather than only the path, since a rule losing or gaining -h would otherwise pass, which was raised separately in 3799155191. The stale comment at install_macos.sh:606-607 is corrected too, since it still described sudo resolving the name via PATH.

[github-actions]

Now that shutdown is no longer resolved, /usr/sbin and /sbin have no remaining user — and the test that pins them justifies them with exactly the case that was removed.

The only names passed to this resolver in production are sudo (entrypoint.py, session_cleanup.py, installer/init.py), pkill (session_cleanup.py), and shutdown.exe (Windows). None of those live under sbin on any supported platform, and the comment here says so outright: "this list is no longer used to locate shutdown". The "other commands can still live only under /sbin" hedge is about hypothetical future callers.

Meanwhile test_posix_searches_both_sbin_locations reads:

"""shutdown is at /usr/sbin/shutdown on usr-merged distributions but only
at /sbin/shutdown on some Debian releases. Hardcoding either one broke a
host; both must be searched."""

That rationale is now false — shutdown is deliberately hardcoded (LINUX_SHUTDOWN_PATH) and is not searched. A future reader trying to work out whether these two entries are load-bearing will find a test asserting they are, for a reason that no longer applies. Either drop the two entries (smallest search surface is the conservative default for a module whose whole purpose is constraining resolution) or restate the test's justification so it does not point at the one command that bypasses the resolver.

Related, the module docstring still argues against hardcoded literals on the same grounds — "on non-usr-merged Debian shutdown exists only at /sbin/shutdown. A hardcoded literal turns a security bug into an availability bug on those hosts" — while this revision hardcodes /usr/sbin/shutdown. That is defensible, because the sudoers rule hardcodes the same path so such hosts were already broken before this PR, but as written the docstring reads as an argument against what the code now does.

[leongdl]

Fixed in 46386e2. Both the module comment and the test now say the honest thing: no production caller resolves an sbin command any more, because shutdown is hardcoded and sudo and pkill are both in /usr/bin. The entries stay, since a system command can be sbin-only on a non-usr-merged distribution, but "no current caller" replaced the justification that cited the case I had removed.

[github-actions]

The new contract test pins the sudoers path but not the sudoers argv, and (?:\s+-h)? is what opens the hole.

sudo matches a NOPASSWD: rule against the whole command line, not just the program. The two installers grant different argv:

install.sh:342        ${wa_user} ALL=(root) NOPASSWD: /usr/sbin/shutdown now
install_macos.sh:618  ${wa_user} ALL=(root) NOPASSWD: /sbin/shutdown -h now

and _host_shutdown builds argv to match each ([sudo, LINUX_SHUTDOWN_PATH, "now"] / [sudo, MACOS_SHUTDOWN_PATH, "-h", "now"]). But _granted_shutdown_path returns match.group(1) only, so the flag is read and thrown away.

Because the -h group is optional, the regex matches either shape for either installer, and two silent drifts get through:

• macOS rule edited to /sbin/shutdown now (dropping -h) — regex matches, group(1) is unchanged, test passes; the agent still sends -h, so the rule no longer matches and sudo prompts for a password.
• Linux rule edited to /usr/sbin/shutdown -h now (someone normalising the two installers) — same thing in the other direction: test passes, the agent sends bare now, grant no longer matches.

Either one produces exactly the failure the class docstring says it exists to prevent, on the platform where the argv has two parts to keep in sync.

Since the helper is already parameterised over the installer, the cheap fix is to capture the tail as well and assert against the argv the module actually builds — e.g. return (group(1), flags) and compare to the platform's expected [path, *flags, "now"], or drop the (?:\s+-h)? optionality and parametrise the expected suffix per installer so an unexpected shape trips the assert match is not None message rather than passing quietly.

[leongdl]

Fixed in 9f4a61b. Correct: (?:\s+-h)? was optional and only group(1) was returned, so both drifts you describe passed. The test now drives _host_shutdown, strips the sudo prefix, and compares the whole remaining argv against the rule's full command line. Mutation-checked by removing -h from the macOS rule, which fails the macOS case and nothing else.