From 231c9512b2e9670a1e4f15c26f1d26fee73207b4 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 11:06:25 -0700 Subject: [PATCH 1/9] fix: use absolute paths for system commands to prevent PATH injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace unqualified shell commands with absolute paths to prevent a job from hijacking command resolution via PATH environment variable manipulation. A malicious job could set PATH to include a directory containing a fake 'sudo' script, which would then execute as the worker agent when OpenJD runs a cross-user action. This allows privilege escalation from the sandboxed job user to the worker agent. Changes: - sudo → /usr/bin/sudo (3 locations) - setsid → /usr/bin/setsid (Linux cross-user execution) - kill → /usr/bin/kill (signal sending) - pstree → /usr/bin/pstree (debug logging) - ps → /bin/ps (debug logging) Security: CWE-426 (Untrusted Search Path) Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_subprocess.py | 16 ++++++++++------ test/openjd/sessions_v0/test_subprocess.py | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/openjd/sessions/_subprocess.py b/src/openjd/sessions/_subprocess.py index 9d0929e6..fe57beb2 100644 --- a/src/openjd/sessions/_subprocess.py +++ b/src/openjd/sessions/_subprocess.py @@ -594,7 +594,7 @@ def _start_subprocess(self) -> Optional[Popen]: ) command.extend( [ - "sudo", + "/usr/bin/sudo", "-u", user.user, "-i", @@ -605,7 +605,9 @@ def _start_subprocess(self) -> Optional[Popen]: ] ) else: - command.extend(["sudo", "-u", user.user, "-i", "setsid", "-w"]) + command.extend( + ["/usr/bin/sudo", "-u", user.user, "-i", "/usr/bin/setsid", "-w"] + ) elif is_windows(): user = cast(WindowsSessionUser, self._user) # type: ignore @@ -888,7 +890,7 @@ def _posix_signal_subprocess( user = cast(PosixSessionUser, self._user) # Only sudo if the user to run as is not the same as the current user. if not user.is_process_user(): - kill_cmd = ["sudo", "-u", user.user, "-i"] + kill_cmd = ["/usr/bin/sudo", "-u", user.user, "-i"] # If we were unable to detect sudo's child process PID after launching the # subprocess, we try again now @@ -943,7 +945,7 @@ def _posix_signal_subprocess( kill_cmd.extend( [ - "kill", + "/usr/bin/kill", "-s", signal_name, "--", @@ -971,12 +973,14 @@ def _posix_signal_subprocess( def _log_process_tree(self) -> None: """A developer method to visualize the process tree including PIDs and PGIDs when debuging tests""" - pstree_result = run(["pstree", "-pg"], stdout=PIPE, stderr=STDOUT, stdin=DEVNULL, text=True) + pstree_result = run( + ["/usr/bin/pstree", "-pg"], stdout=PIPE, stderr=STDOUT, stdin=DEVNULL, text=True + ) self._logger.debug( f"pstree -pg output: {pstree_result.stdout}", extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL), ) - ps_result = run(["ps", "-ejH"], stdout=PIPE, stderr=STDOUT, stdin=DEVNULL, text=True) + ps_result = run(["/bin/ps", "-ejH"], stdout=PIPE, stderr=STDOUT, stdin=DEVNULL, text=True) self._logger.debug( f"ps -ejH output:\n{ps_result.stdout}", extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL), diff --git a/test/openjd/sessions_v0/test_subprocess.py b/test/openjd/sessions_v0/test_subprocess.py index 11231ed2..9fbe3455 100644 --- a/test/openjd/sessions_v0/test_subprocess.py +++ b/test/openjd/sessions_v0/test_subprocess.py @@ -1315,7 +1315,7 @@ def test_builds_setsid_shim_command_on_macos(self, queue_handler: QueueHandler) # THEN built_command = mock_popen.call_args.kwargs["args"] assert built_command == [ - "sudo", + "/usr/bin/sudo", "-u", "job-user", "-i", From d7b5b19c9a3fe8a9526bf2438d958762d28067e6 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:10:56 -0700 Subject: [PATCH 2/9] fix: resolve system commands from trusted dirs, not PATH The previous commit replaced unqualified command names with absolute path literals. That closes the PATH-injection hole but hardcodes locations that are not universal: NixOS keeps the setuid sudo wrapper in /run/wrappers/bin, so a literal /usr/bin/sudo turns a security bug into a cross-user execution failure. Introduce _system_commands.py, which resolves a bare command name against a fixed, ordered list of trusted absolute directories. PATH is never consulted, and neither is shutil.which -- which resolves through PATH and so would reintroduce the vulnerability while appearing to fix it. Three properties are load-bearing: * PATH is never read, directly or indirectly. * Only paths under TRUSTED_SYSTEM_DIRECTORIES are returned. Command names containing a path separator are rejected, so the resolver cannot itself become the injection point via os.path.join("/usr/bin", "../../tmp/evil"). * A missing command raises rather than falling back to the bare name. A silent fallback would restore the vulnerability while looking fixed, which is the worst available failure mode for this class of fix. Also fixes a site missed by the previous pass: _linux/_sudo.py invoked pgrep unqualified. That is the macOS lookup of sudo's child process, used to choose the target for cancelation signals, so a substituted pgrep need not execute anything -- returning a false PID is enough to redirect a SIGKILL. It is less severe than the reported sudo sites because the call passes no env=, so the job's PATH does not reach it, but it is the same defect class. Migrated: sudo (3 sites), setsid, kill, pgrep, and the pstree/ps debug dump. The debug dump uses the non-raising variant, since a developer aid must not fail on a host that has no pstree. Known gap: the three security properties above are not yet pinned by tests, and the module docstring says so rather than implying coverage that does not exist. That suite is the immediate follow-up. Refs: HackerOne 3942741, CWE-426 Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_linux/_sudo.py | 3 +- src/openjd/sessions/_subprocess.py | 44 +++--- src/openjd/sessions/_system_commands.py | 147 +++++++++++++++++++++ test/openjd/sessions_v0/test_subprocess.py | 11 +- 4 files changed, 187 insertions(+), 18 deletions(-) create mode 100644 src/openjd/sessions/_system_commands.py diff --git a/src/openjd/sessions/_linux/_sudo.py b/src/openjd/sessions/_linux/_sudo.py index fccfef88..186acf85 100644 --- a/src/openjd/sessions/_linux/_sudo.py +++ b/src/openjd/sessions/_linux/_sudo.py @@ -9,6 +9,7 @@ from .._logging import LoggerAdapter, LogContent, LogExtraInfo from .._os_checker import is_posix, is_linux +from .._system_commands import system_command_path PGREP_NO_MATCH = 1 """``pgrep``'s exit status for "no processes matched". @@ -174,7 +175,7 @@ def find_child_process_id_pgrep( sudo_pid: int, ) -> Optional[int]: pgrep_result = run( - ["pgrep", "-P", str(sudo_pid)], + [system_command_path("pgrep"), "-P", str(sudo_pid)], stdout=PIPE, stderr=STDOUT, stdin=DEVNULL, diff --git a/src/openjd/sessions/_subprocess.py b/src/openjd/sessions/_subprocess.py index fe57beb2..e345d762 100644 --- a/src/openjd/sessions/_subprocess.py +++ b/src/openjd/sessions/_subprocess.py @@ -19,6 +19,7 @@ from ._logging import LoggerAdapter, LogContent, LogExtraInfo from ._os_checker import is_linux, is_macos, is_posix, is_windows from ._session_user import PosixSessionUser, WindowsSessionUser, SessionUser +from ._system_commands import find_system_command, system_command_path from ._action_filter import redact_openjd_redacted_env_requests if is_windows(): # pragma: nocover @@ -594,7 +595,7 @@ def _start_subprocess(self) -> Optional[Popen]: ) command.extend( [ - "/usr/bin/sudo", + system_command_path("sudo"), "-u", user.user, "-i", @@ -606,7 +607,14 @@ def _start_subprocess(self) -> Optional[Popen]: ) else: command.extend( - ["/usr/bin/sudo", "-u", user.user, "-i", "/usr/bin/setsid", "-w"] + [ + system_command_path("sudo"), + "-u", + user.user, + "-i", + system_command_path("setsid"), + "-w", + ] ) elif is_windows(): user = cast(WindowsSessionUser, self._user) # type: ignore @@ -890,7 +898,7 @@ def _posix_signal_subprocess( user = cast(PosixSessionUser, self._user) # Only sudo if the user to run as is not the same as the current user. if not user.is_process_user(): - kill_cmd = ["/usr/bin/sudo", "-u", user.user, "-i"] + kill_cmd = [system_command_path("sudo"), "-u", user.user, "-i"] # If we were unable to detect sudo's child process PID after launching the # subprocess, we try again now @@ -945,7 +953,7 @@ def _posix_signal_subprocess( kill_cmd.extend( [ - "/usr/bin/kill", + system_command_path("kill"), "-s", signal_name, "--", @@ -973,18 +981,22 @@ def _posix_signal_subprocess( def _log_process_tree(self) -> None: """A developer method to visualize the process tree including PIDs and PGIDs when debuging tests""" - pstree_result = run( - ["/usr/bin/pstree", "-pg"], stdout=PIPE, stderr=STDOUT, stdin=DEVNULL, text=True - ) - self._logger.debug( - f"pstree -pg output: {pstree_result.stdout}", - extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL), - ) - ps_result = run(["/bin/ps", "-ejH"], stdout=PIPE, stderr=STDOUT, stdin=DEVNULL, text=True) - self._logger.debug( - f"ps -ejH output:\n{ps_result.stdout}", - extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL), - ) + # find_system_command rather than system_command_path: neither of these is + # required for correct operation, and a debug aid must not raise on a host + # that happens not to install pstree. + for name, args in (("pstree", ["-pg"]), ("ps", ["-ejH"])): + command = find_system_command(name) + if command is None: + self._logger.debug( + f"{name} is not installed; skipping its process tree dump", + extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL), + ) + continue + result = run([command, *args], stdout=PIPE, stderr=STDOUT, stdin=DEVNULL, text=True) + self._logger.debug( + f"{name} {' '.join(args)} output:\n{result.stdout}", + extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL), + ) def _windows_notify_subprocess(self, process: Popen) -> None: """Sends a CTRL_BREAK_EVENT signal to the subprocess. diff --git a/src/openjd/sessions/_system_commands.py b/src/openjd/sessions/_system_commands.py new file mode 100644 index 00000000..0d768f50 --- /dev/null +++ b/src/openjd/sessions/_system_commands.py @@ -0,0 +1,147 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +"""Resolution of system command names to absolute paths, without consulting PATH. + +This module exists because of CWE-426 (Untrusted Search Path). A session runs +job-supplied actions, and the job controls the environment those actions run +with -- including ``PATH``. That same environment is handed to the ``Popen`` call +that launches our *own* privileged helpers (``sudo``, ``setsid``, ``kill``), so a +bare command name in that argv is resolved through a search path the job wrote. +A job that drops an executable named ``sudo`` early on ``PATH`` gets it run at +the session's privilege level. + +The fix is to never let a command name reach ``execvp``-style resolution. Every +name is resolved here instead, by scanning a fixed list of trusted absolute +directories. + +Three properties are load-bearing: + +.. warning:: + These properties are **not yet pinned by tests.** The regression suite for + this module is outstanding work -- see §5.5 of the PATH-injection analysis for + the table of properties that need falsifiable coverage. Until those exist, + nothing here fails if the trusted-directory scan is replaced with + :func:`shutil.which`, so treat the guarantees below as intent rather than as + verified behaviour. + + +* **``PATH`` is never read.** Not directly, and not indirectly via + :func:`shutil.which` or ``command -v`` -- both of which resolve through + ``PATH`` and so would reintroduce the vulnerability while appearing to fix it. +* **Only paths under :data:`TRUSTED_SYSTEM_DIRECTORIES` are returned.** +* **A command that cannot be found raises.** Falling back to the bare name would + silently restore the vulnerability, which is the worst available failure mode + for this class of fix: the code would look fixed and behave as if it were not. + +This module is POSIX-oriented; :data:`TRUSTED_SYSTEM_DIRECTORIES` lists POSIX +locations. On Windows nothing will be found and the lookups raise, which is +correct for this library because the cross-user code paths that need these +commands are POSIX-only. +""" + +from __future__ import annotations + +import os +from functools import lru_cache +from typing import Optional, Tuple + +__all__ = [ + "SystemCommandNotFoundError", + "TRUSTED_SYSTEM_DIRECTORIES", + "find_system_command", + "system_command_path", +] + + +TRUSTED_SYSTEM_DIRECTORIES: Tuple[str, ...] = ( + # Ordered, and the order is deliberate. On NixOS the setuid `sudo` wrapper + # lives in /run/wrappers/bin and the /usr/bin copy is either absent or not + # setuid, so the wrapper directory has to be consulted first. Everywhere else + # this directory does not exist and costs one stat(). + "/run/wrappers/bin", + "/usr/bin", + "/bin", + # sbin entries are last: `shutdown` lives here, and on non-usr-merged + # distributions (some Debian releases) it is *only* at /sbin/shutdown. + "/usr/sbin", + "/sbin", +) + + +class SystemCommandNotFoundError(Exception): + """A required system command was not present in any trusted directory. + + Deliberately not a subclass of :class:`FileNotFoundError`. Callers around the + subprocess machinery already catch ``OSError`` subclasses to mean "the thing + I tried to launch is missing, carry on degraded", and this condition must not + be absorbed by that handling: it means a privileged helper is unavailable, so + the operation cannot proceed safely. + """ + + +def _validate_command_name(name: str) -> None: + """Reject anything that is not a bare command name. + + Without this the resolver would itself become the injection point it exists + to remove: ``os.path.join("/usr/bin", "../../tmp/evil")`` escapes the trusted + directory entirely, so a caller that passed attacker-influenced text would be + no better off than before. + """ + if not name: + raise ValueError("A system command name must not be empty.") + if name in (os.curdir, os.pardir): + raise ValueError(f"{name!r} is not a system command name.") + # Checking both separators regardless of platform. A backslash is a legal + # filename character on POSIX, but no command this module resolves contains + # one, and treating it as suspect keeps the check identical on both + # platforms rather than subtly weaker on one. + if "/" in name or "\\" in name: + raise ValueError( + f"A system command name must not contain a path separator, but got {name!r}." + ) + + +def _is_executable_file(path: str) -> bool: + return os.path.isfile(path) and os.access(path, os.X_OK) + + +@lru_cache(maxsize=None) +def find_system_command(name: str) -> Optional[str]: + """Return the absolute path to ``name``, or ``None`` if it is not installed. + + Searches :data:`TRUSTED_SYSTEM_DIRECTORIES` in order. ``PATH`` is not + consulted. Use this for commands whose absence is tolerable; use + :func:`system_command_path` when the command is required. + + The result is cached: the filesystem layout does not change underneath a + running session, and these lookups sit on process-launch and + signal-delivery paths. Tests that patch + :data:`TRUSTED_SYSTEM_DIRECTORIES` must call + ``find_system_command.cache_clear()``. + + Raises: + ValueError: if ``name`` is not a bare command name. + """ + _validate_command_name(name) + for directory in TRUSTED_SYSTEM_DIRECTORIES: + candidate = os.path.join(directory, name) + if _is_executable_file(candidate): + return candidate + return None + + +def system_command_path(name: str) -> str: + """Return the absolute path to ``name``. + + Raises: + ValueError: if ``name`` is not a bare command name. + SystemCommandNotFoundError: if ``name`` is not in any trusted directory. + """ + path = find_system_command(name) + if path is None: + raise SystemCommandNotFoundError( + f"Could not find the system command {name!r} in any trusted directory " + f"({', '.join(TRUSTED_SYSTEM_DIRECTORIES)}). PATH is deliberately not " + f"searched; see openjd.sessions._system_commands." + ) + return path diff --git a/test/openjd/sessions_v0/test_subprocess.py b/test/openjd/sessions_v0/test_subprocess.py index 9fbe3455..20e58366 100644 --- a/test/openjd/sessions_v0/test_subprocess.py +++ b/test/openjd/sessions_v0/test_subprocess.py @@ -1308,6 +1308,15 @@ def test_builds_setsid_shim_command_on_macos(self, queue_handler: QueueHandler) patch.object( subprocess_mod, "_macos_shim_interpreter", return_value="/usr/local/bin/python3" ), + # Stubbed so the assertion below pins the *shape* of the argv rather + # than this host's sudo location. It also makes the assertion prove + # the command went through the trusted-path resolver: a literal + # "sudo" in the source would no longer match. + patch.object( + subprocess_mod, + "system_command_path", + side_effect=lambda name: f"/trusted/bin/{name}", + ), patch.object(subprocess_mod, "Popen") as mock_popen, ): subproc._start_subprocess() @@ -1315,7 +1324,7 @@ def test_builds_setsid_shim_command_on_macos(self, queue_handler: QueueHandler) # THEN built_command = mock_popen.call_args.kwargs["args"] assert built_command == [ - "/usr/bin/sudo", + "/trusted/bin/sudo", "-u", "job-user", "-i", From 1839df40e2883e32cf29b04a4c63ba4f945f207c Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:39:42 -0700 Subject: [PATCH 3/9] test: pin the trusted-path resolver's security properties The previous commit introduced _system_commands but left its guarantees unfalsified, and said so in the module docstring. Nothing failed if the trusted-directory scan were replaced with shutil.which, which is the worst gap this fix could have: its failure mode is silence. A resolver that quietly falls back to PATH looks identical to a working one, and the code still reads as fixed. Add test_system_commands.py and remove the docstring warning. Every property is mutation-checked. Eight mutants, all caught, against a green baseline with the source restored and verified by checksum: M1 resolve via shutil.which -> test_ignores_path_even_when_it_contains_a_ matching_command ("PATH was consulted"), plus 2 others M2 drop the separator guard -> test_rejects_traversal_even_though_the_ target_is_reachable, plus 3 others M3 fall back to the bare name -> test_raises_rather_than_returning_the_bare_name M4 subclass FileNotFoundError -> test_is_not_a_filenotfounderror M5 reorder the wrapper dir -> test_searches_the_setuid_wrapper_directory_ before_usr_bin M6 drop /sbin -> test_searches_both_sbin_locations M7 ignore the exec bit -> test_ignores_a_non_executable_file M8 empty the directory list -> the TestRealCommandsResolve controls Three notes on why the suite is shaped as it is: M8 is the reason TestRealCommandsResolve exists. Every other test patches TRUSTED_SYSTEM_DIRECTORIES to a temporary 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 instead. M2 is the reason 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 would pass with the guard deleted. The nested-directory test reaches a real file through the traversal and asserts that precondition, so it fails when the guard goes. 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", and an unavailable privileged helper must not be absorbed by that handling. find_system_command is lru_cached, so an autouse fixture clears the cache around each test; without it a result computed under one patched directory list leaks into the next test. 966 passed, 40 skipped, 16 xfailed. black and mypy clean. Refs: HackerOne 3942741, CWE-426 Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_system_commands.py | 11 +- .../sessions_v0/test_system_commands.py | 230 ++++++++++++++++++ 2 files changed, 232 insertions(+), 9 deletions(-) create mode 100644 test/openjd/sessions_v0/test_system_commands.py diff --git a/src/openjd/sessions/_system_commands.py b/src/openjd/sessions/_system_commands.py index 0d768f50..ec9a7668 100644 --- a/src/openjd/sessions/_system_commands.py +++ b/src/openjd/sessions/_system_commands.py @@ -14,15 +14,8 @@ name is resolved here instead, by scanning a fixed list of trusted absolute directories. -Three properties are load-bearing: - -.. warning:: - These properties are **not yet pinned by tests.** The regression suite for - this module is outstanding work -- see §5.5 of the PATH-injection analysis for - the table of properties that need falsifiable coverage. Until those exist, - nothing here fails if the trusted-directory scan is replaced with - :func:`shutil.which`, so treat the guarantees below as intent rather than as - verified behaviour. +Three properties are load-bearing, and each is pinned by a mutation-checked test +in ``test/openjd/sessions_v0/test_system_commands.py``: * **``PATH`` is never read.** Not directly, and not indirectly via diff --git a/test/openjd/sessions_v0/test_system_commands.py b/test/openjd/sessions_v0/test_system_commands.py new file mode 100644 index 00000000..6e1118ee --- /dev/null +++ b/test/openjd/sessions_v0/test_system_commands.py @@ -0,0 +1,230 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +"""Tests for the trusted-path command resolver. + +These pin the security properties of ``_system_commands``, the fix for the PATH +injection reported as HackerOne 3942741. Each has been mutation-checked: +reverting the corresponding production behaviour makes a named test here fail. + +Why this matters more than usual: 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 reads as fixed. Without a test that fails when the +trusted-directory scan is removed, the fix is an unverified claim. +""" + +import os +import stat +from pathlib import Path +from unittest.mock import patch + +import pytest + +from openjd.sessions._system_commands import ( + SystemCommandNotFoundError, + TRUSTED_SYSTEM_DIRECTORIES, + find_system_command, + system_command_path, +) + +_MODULE = "openjd.sessions._system_commands" + + +@pytest.fixture(autouse=True) +def clear_resolver_cache(): + """``find_system_command`` is ``lru_cache``d, so a result computed under one + patched directory list would otherwise leak into the next test.""" + find_system_command.cache_clear() + yield + find_system_command.cache_clear() + + +@pytest.fixture +def executable_dir(tmp_path: Path) -> Path: + """A directory containing an executable file named ``target-cmd``.""" + target = tmp_path / "target-cmd" + target.write_text("#!/bin/sh\ntrue\n") + target.chmod(target.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + return tmp_path + + +class TestOnlySearchesTrustedDirectories: + def test_resolves_a_command_in_a_searched_directory(self, executable_dir: Path) -> None: + """The negative control. Without it, the "not found" assertions below would + be indistinguishable from a resolver that never finds anything.""" + # GIVEN + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", (str(executable_dir),)): + # WHEN + result = find_system_command("target-cmd") + + # THEN + assert result == str(executable_dir / "target-cmd") + + def test_does_not_resolve_a_command_outside_searched_directories( + self, executable_dir: Path + ) -> None: + # GIVEN + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", ("/usr/bin", "/bin")): + # WHEN + result = find_system_command("target-cmd") + + # THEN + assert result is None + + def test_ignores_path_even_when_it_contains_a_matching_command( + self, executable_dir: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Pins "PATH is never read". + + This is the property a ``shutil.which`` implementation would silently + violate, and the one a PATH fallback for missing commands would undo. It + is the closest thing this suite has to a direct test of the reported + vulnerability: the job-controlled PATH must not influence resolution. + """ + # GIVEN + monkeypatch.setenv("PATH", str(executable_dir)) + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", ("/usr/bin", "/bin")): + # WHEN + result = find_system_command("target-cmd") + + # THEN + assert result is None, "PATH was consulted" + + def test_returns_the_first_matching_directory(self, tmp_path: Path) -> None: + """Order is load-bearing: on NixOS the setuid sudo wrapper must win over a + non-setuid /usr/bin copy.""" + # GIVEN + first, second = tmp_path / "first", tmp_path / "second" + for directory in (first, second): + directory.mkdir() + target = directory / "target-cmd" + target.write_text("#!/bin/sh\ntrue\n") + target.chmod(target.stat().st_mode | stat.S_IXUSR) + + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", (str(first), str(second))): + # WHEN + result = find_system_command("target-cmd") + + # THEN + assert result == str(first / "target-cmd") + + def test_ignores_a_non_executable_file(self, tmp_path: Path) -> None: + # GIVEN + (tmp_path / "target-cmd").write_text("not executable") + + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", (str(tmp_path),)): + # WHEN + result = find_system_command("target-cmd") + + # THEN + assert result is None + + +class TestRejectsNonBareNames: + @pytest.mark.parametrize( + "name", + [ + pytest.param("a/b", id="forward-slash"), + pytest.param("a\\b", id="backslash"), + pytest.param("", id="empty"), + pytest.param(".", id="curdir"), + pytest.param("..", id="pardir"), + ], + ) + def test_rejects_name_with_a_path_component(self, name: str) -> None: + with pytest.raises(ValueError): + find_system_command(name) + + def test_rejects_traversal_even_though_the_target_is_reachable(self, tmp_path: Path) -> None: + """The guard must be about the name, not about whether the join happens to + land on a real file -- so prove the target IS reachable by that join before + asserting the name is refused. + + Without this, a test whose searched directory has nothing above it would + pass even with the guard deleted, pinning nothing. That was verified: the + parametrized test above does not catch the mutation on its own. + """ + # GIVEN + target = tmp_path / "target-cmd" + target.write_text("#!/bin/sh\ntrue\n") + target.chmod(target.stat().st_mode | stat.S_IXUSR) + nested = tmp_path / "nested" + nested.mkdir() + assert os.path.isfile( + os.path.join(str(nested), "../target-cmd") + ), "precondition: the traversal target is reachable by this join" + + # WHEN / THEN + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", (str(nested),)): + with pytest.raises(ValueError, match="path separator"): + find_system_command("../target-cmd") + + def test_rejection_is_valueerror_not_notfound(self) -> None: + """A bad name is a caller bug; a missing command is an environment problem. + Conflating them would let a caller mistake one for the other.""" + with pytest.raises(ValueError): + system_command_path("a/b") + + +class TestMissingCommandRaises: + def test_find_returns_none(self) -> None: + assert find_system_command("openjd-definitely-not-installed") is None + + def test_raises_rather_than_returning_the_bare_name(self) -> None: + """The silent-fallback failure mode: returning "sudo" here would look fixed + and behave exactly as the vulnerability did.""" + with pytest.raises(SystemCommandNotFoundError) as excinfo: + system_command_path("openjd-definitely-not-installed") + + message = str(excinfo.value) + assert "openjd-definitely-not-installed" in message + assert "PATH is deliberately not searched" in message + + def test_is_not_a_filenotfounderror(self) -> None: + """The subprocess machinery catches OSError subclasses to mean "the thing I + tried to launch is missing, carry on degraded". An unavailable privileged + helper must not be absorbed by that handling.""" + assert not issubclass(SystemCommandNotFoundError, OSError) + + +class TestTrustedDirectories: + def test_all_entries_are_absolute(self) -> None: + """A relative entry would resolve against the process working directory, + which a session changes.""" + for directory in TRUSTED_SYSTEM_DIRECTORIES: + assert os.path.isabs(directory), f"{directory} is not absolute" + + def test_searches_the_setuid_wrapper_directory_before_usr_bin(self) -> None: + assert TRUSTED_SYSTEM_DIRECTORIES.index( + "/run/wrappers/bin" + ) < TRUSTED_SYSTEM_DIRECTORIES.index("/usr/bin") + + def test_searches_both_sbin_locations(self) -> None: + """On non-usr-merged distributions some system commands exist only under + /sbin.""" + assert "/usr/sbin" in TRUSTED_SYSTEM_DIRECTORIES + assert "/sbin" in TRUSTED_SYSTEM_DIRECTORIES + + +class TestRealCommandsResolve: + """Positive controls against the real filesystem. + + The tests above use temporary directories, so they would all pass on a host + where the genuine trusted directories were empty or misspelled. These assert + that the commands the library actually launches are found where it looks. + """ + + @pytest.mark.parametrize("name", ["sh", "ls"]) + def test_a_universally_present_command_resolves(self, name: str) -> None: + resolved = system_command_path(name) + + assert os.path.isabs(resolved) + assert os.path.dirname(resolved) in TRUSTED_SYSTEM_DIRECTORIES + + def test_sudo_resolves_on_this_host(self) -> None: + """sudo is the command the reported vulnerability abused. If this fails, + cross-user sessions cannot start on this host -- which is exactly the + portability regression that hardcoded literals introduced.""" + if find_system_command("sudo") is None: + pytest.skip("sudo is not installed on this host") + + assert os.path.dirname(system_command_path("sudo")) in TRUSTED_SYSTEM_DIRECTORIES From 68265a3cf9743d4f3dba5855bb9e9cfe51019a7d Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:08:16 -0700 Subject: [PATCH 4/9] fix: address automated review findings on the trusted-path resolver Four defects from bot review, three of which overturn decisions this branch made deliberately. Each is verified against the code rather than taken on faith, and each fix is mutation-checked. 1. SystemCommandNotFoundError now derives from FileNotFoundError. The previous revision made it a plain Exception on the stated grounds that an unavailable privileged helper must not be absorbed by handlers that catch OSError to mean "carry on degraded". That reasoning assumed what those handlers do rather than checking. _runner_base's cancel path catches OSError around notify()/terminate() precisely so a failure to signal does not unwind an in-progress cancelation; its own comment says "a cancel path is the wrong place to raise". Signalling reaches system_command_path("sudo"), so a non-OSError escapes that guard on a live path and costs the cancel its bookkeeping. The prior test asserted `not issubclass(..., OSError)` -- it pinned the wrong decision, which is worse than no test. It is now inverted, with the reversal and its reason recorded in the test body. 2. `kill` stays a bare name in the cross-user branch. That argv runs as `sudo -u -i kill ...`. `sudo -i` simulates an initial login: it starts the target user's login shell and passes the command via -c, so `kill` is a shell builtin there. Nothing is resolved on PATH and -i has already reset the environment, so CWE-426 never reached this position. Resolving it invented a hard dependency on kill(1) from procps, which Debian -slim images -- a common worker base -- do not install, and would have broken every cross-user SIGKILL fallback on such a host. The same-user branch still resolves it, because there `run()` really does execvp a bare name against this process's PATH. Both branches are now pinned, since the difference is easy to "tidy" into a bug in either direction. 3. sudo is no longer resolved before the direct-signal attempt. kill_cmd was built at the top of _posix_signal_subprocess, ~20 lines before the os.killpg() path that returns on success. On a host with CAP_KILL that direct path is the only one taken, so a missing sudo aborted signals that previously worked -- a regression the bare-string version did not have. Resolution now happens only on the branch that uses it. 4. /run/current-system/sw/bin added to the trusted directories. The ordering comment justified putting /run/wrappers/bin first "so NixOS works", but that directory holds only the setuid wrappers. On NixOS it resolves sudo and nothing else: /usr/bin holds just env, /bin just sh, and the sbin directories are absent. setsid and pgrep live in the sw/bin symlink farm, so a cross-user session resolved sudo and then failed on setsid one line later -- the claimed support did not exist. The pairing is now asserted so it cannot be half-removed. Also: the name guard rejects ":" as well as separators. ntpath.join(r"C:\Windows\System32", "D:evil") == "D:evil", so a drive-relative name discards the trusted prefix while containing no separator at all. Harmless under posixpath, but the guard belongs in the validator rather than depending on which os.path is loaded. Also: TestRealCommandsResolve and test_ignores_a_non_executable_file are now POSIX-guarded. They would have failed the windows-latest CI leg -- confirmed, the equivalent tests did exactly that in the sibling job-attachments PR. On Windows os.access(X_OK) is true for any existing file, so "not executable" is not expressible there, and none of the trusted directories exist. Mutation-checked, all four caught against a green baseline with sources restored and verified by checksum: R1 revert to plain Exception -> test_is_an_oserror_so_the_cancel_path_ can_absorb_it R2 drop the colon from the guard -> test_rejects_name_with_a_path_ component[drive-relative] R3 drop the NixOS sw/bin entry -> test_the_two_nixos_entries_are_present_ as_a_pair R4 resolve kill under sudo -i -> test_cross_user_leaves_kill_bare_for_ the_login_shell_builtin R4 is worth flagging: it SURVIVED the first time it was run. The fix had no test protecting it, so the procps dependency could have been reintroduced freely. The test above was written in response and then confirmed to fail against the mutant. 972 passed, 40 skipped, 16 xfailed. black and mypy clean. Refs: HackerOne 3942741, CWE-426 Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_subprocess.py | 34 +++++-- src/openjd/sessions/_system_commands.py | 48 +++++++--- .../test_subprocess_process_group.py | 90 +++++++++++++++++++ .../sessions_v0/test_system_commands.py | 59 ++++++++++-- 4 files changed, 208 insertions(+), 23 deletions(-) diff --git a/src/openjd/sessions/_subprocess.py b/src/openjd/sessions/_subprocess.py index e345d762..dda295d0 100644 --- a/src/openjd/sessions/_subprocess.py +++ b/src/openjd/sessions/_subprocess.py @@ -892,13 +892,15 @@ def _posix_signal_subprocess( else: raise NotImplementedError(f"Unsupported signal: {signal_name}") - kill_cmd = list[str]() - - if self._user is not None: - user = cast(PosixSessionUser, self._user) - # Only sudo if the user to run as is not the same as the current user. - if not user.is_process_user(): - kill_cmd = [system_command_path("sudo"), "-u", user.user, "-i"] + # Whether this signal has to go through sudo. The argv itself is not built + # yet: the direct os.killpg() attempt below returns on success, and on a + # host with CAP_KILL that is the only path taken. Resolving sudo here would + # make a host without sudo unable to signal *even when it never needs + # sudo* -- a regression this method did not have when the name was a bare + # string in a list. + needs_sudo = ( + self._user is not None and not cast(PosixSessionUser, self._user).is_process_user() + ) # If we were unable to detect sudo's child process PID after launching the # subprocess, we try again now @@ -951,9 +953,25 @@ def _posix_signal_subprocess( # Uncomment to visualize process tree when debugging tests # self._log_process_tree() + # `kill` is resolved only for the same-user branch, where `run()` really + # does execvp() a bare name against this process's PATH. + # + # In the cross-user branch it must stay a bare name. `sudo -i` simulates an + # initial login: it starts the target user's login shell and hands it the + # command via -c, so `kill` is a *shell builtin* there. Nothing is looked up + # on PATH, and -i has already reset the environment, so CWE-426 does not + # reach this position. Resolving it would instead invent a hard dependency + # on kill(1) from procps, which Debian `-slim` images -- a common worker + # base -- do not install, and would break every cross-user SIGKILL fallback + # on such a host. + if needs_sudo: + user = cast(PosixSessionUser, self._user) + kill_cmd = [system_command_path("sudo"), "-u", user.user, "-i", "kill"] + else: + kill_cmd = [system_command_path("kill")] + kill_cmd.extend( [ - system_command_path("kill"), "-s", signal_name, "--", diff --git a/src/openjd/sessions/_system_commands.py b/src/openjd/sessions/_system_commands.py index ec9a7668..893da454 100644 --- a/src/openjd/sessions/_system_commands.py +++ b/src/openjd/sessions/_system_commands.py @@ -52,23 +52,44 @@ # setuid, so the wrapper directory has to be consulted first. Everywhere else # this directory does not exist and costs one stat(). "/run/wrappers/bin", + # ...and the two NixOS entries are a pair. /run/wrappers/bin holds *only* the + # setuid/setcap wrappers, so on NixOS it resolves `sudo` and nothing else: + # /usr/bin holds just `env`, /bin just `sh`, and the sbin directories do not + # exist. `setsid` and `pgrep` live in this symlink farm, which nixos-rebuild + # manages and root owns, making it trust-equivalent to /usr/bin there. + # + # Without this entry the ordering above buys nothing: a cross-user session + # would resolve `sudo` and then fail on `setsid` one line later. The pairing + # is asserted in TestTrustedDirectories so it cannot be half-removed. + "/run/current-system/sw/bin", "/usr/bin", "/bin", - # sbin entries are last: `shutdown` lives here, and on non-usr-merged - # distributions (some Debian releases) it is *only* at /sbin/shutdown. + # sbin entries are last: on non-usr-merged distributions (some Debian + # releases) some system commands exist only under /sbin. "/usr/sbin", "/sbin", ) -class SystemCommandNotFoundError(Exception): +class SystemCommandNotFoundError(FileNotFoundError): """A required system command was not present in any trusted directory. - Deliberately not a subclass of :class:`FileNotFoundError`. Callers around the - subprocess machinery already catch ``OSError`` subclasses to mean "the thing - I tried to launch is missing, carry on degraded", and this condition must not - be absorbed by that handling: it means a privileged helper is unavailable, so - the operation cannot proceed safely. + A :class:`FileNotFoundError`, and therefore an :class:`OSError`, on purpose. + + An earlier revision of this module deliberately made it a plain ``Exception``, + reasoning that "a privileged helper is unavailable" must not be absorbed by + handlers that catch ``OSError`` to mean "carry on degraded". That reasoning was + wrong here, because it assumed rather than checked what those handlers do. + ``_runner_base``'s cancel path catches ``OSError`` around + ``notify()``/``terminate()`` precisely so a failure to signal does not unwind an + in-progress cancelation -- its own comment says "a cancel path is the wrong + place to raise". Escaping that handler would lose the cancel's bookkeeping, + which is worse than the warning it logs. + + So the semantics this class wants are exactly ``FileNotFoundError``'s: the thing + we tried to launch is not there. Remaining a distinct type still lets a caller + that cares tell "not in any trusted directory" apart from "``exec`` failed", and + the message says which. """ @@ -88,9 +109,16 @@ def _validate_command_name(name: str) -> None: # filename character on POSIX, but no command this module resolves contains # one, and treating it as suspect keeps the check identical on both # platforms rather than subtly weaker on one. - if "/" in name or "\\" in name: + # + # The colon is rejected for the same reason, and it is not hypothetical: + # ntpath.join(r"C:\Windows\System32", "D:evil") == "D:evil". A drive-relative + # name discards the trusted prefix entirely while containing no separator at + # all, so a separator-only check lets it through. POSIX joins it harmlessly, + # but the guard belongs here rather than depending on which os.path is loaded. + if "/" in name or "\\" in name or ":" in name: raise ValueError( - f"A system command name must not contain a path separator, but got {name!r}." + f"A system command name must not contain a path separator or drive " + f"specifier, but got {name!r}." ) diff --git a/test/openjd/sessions_v0/test_subprocess_process_group.py b/test/openjd/sessions_v0/test_subprocess_process_group.py index b088be76..27b53b29 100644 --- a/test/openjd/sessions_v0/test_subprocess_process_group.py +++ b/test/openjd/sessions_v0/test_subprocess_process_group.py @@ -14,7 +14,9 @@ import pytest +from openjd.sessions import _subprocess as subprocess_mod from openjd.sessions._os_checker import is_posix +from openjd.sessions._session_user import PosixSessionUser from openjd.sessions._subprocess import LoggingSubprocess from .conftest import build_logger @@ -87,3 +89,91 @@ def test_sudo_helper_returns_unknown_when_sudo_is_already_gone(self) -> None: # THEN: the established "unknown" value, not an escaping ESRCH. assert result is None + + +@pytest.mark.skipif(not is_posix(), reason="posix signal delivery") +class TestSignalArgvCommandResolution: + """How `kill` is spelled in the signal argv, per branch. + + The two branches deliberately differ, and the difference is easy to "tidy" + into a bug in either direction, so both are pinned here. + + Cross-user goes through `sudo -u -i kill ...`. `sudo -i` simulates an + initial login: it starts the target user's login shell and passes the command + to it via -c, so `kill` is a **shell builtin** in that position. Nothing is + resolved on PATH and -i has already reset the environment, so CWE-426 does not + reach it. Resolving it to an absolute path would instead invent a hard + dependency on kill(1) from procps, which Debian `-slim` images do not install, + and would break every cross-user SIGKILL fallback on such a host. + + Same-user goes through `run([...])`, which really does execvp() a bare name + against this process's PATH, so there the resolution is load-bearing. + """ + + def _signal_and_capture_argv(self, user, message_queue, queue_handler) -> list[str]: + logger = build_logger(queue_handler) + proc = LoggingSubprocess(logger=logger, args=["/path/to/workload.sh"], user=user) + proc._sudo_child_process_group_id = 4321 + + with ( + patch.object(subprocess_mod, "is_posix", return_value=True), + patch.object(subprocess_mod, "is_windows", return_value=False), + patch.object(subprocess_mod, "is_linux", return_value=False), + patch.object( + subprocess_mod, + "system_command_path", + side_effect=lambda name: f"/trusted/{name}", + ), + patch.object(subprocess_mod, "os") as mock_os, + patch.object(subprocess_mod, "run") as mock_run, + ): + mock_os.killpg.side_effect = OSError(1, "not permitted") + mock_run.return_value = MagicMock(returncode=0) + proc._posix_signal_subprocess(MagicMock(pid=999999), signal_name="kill") + + assert mock_run.call_count == 1, "the sudo/run fallback did not execute" + return list(mock_run.call_args.args[0]) + + def test_cross_user_leaves_kill_bare_for_the_login_shell_builtin( + self, message_queue: SimpleQueue, queue_handler: QueueHandler + ) -> None: + # GIVEN + user = MagicMock(spec=PosixSessionUser) + user.user = "job-user" + user.is_process_user.return_value = False + + # WHEN + argv = self._signal_and_capture_argv(user, message_queue, queue_handler) + + # THEN + assert argv == [ + "/trusted/sudo", + "-u", + "job-user", + "-i", + "kill", + "-s", + "kill", + "--", + "-4321", + ] + # Spelled out separately from the equality above, because this is the + # property that matters and the reason for it is not obvious from the list. + assert ( + "kill" in argv and "/trusted/kill" not in argv + ), "kill must stay a bare name so the login shell builtin is used" + + def test_same_user_resolves_kill_because_run_execvps_it( + self, message_queue: SimpleQueue, queue_handler: QueueHandler + ) -> None: + # GIVEN + user = MagicMock(spec=PosixSessionUser) + user.user = "same-user" + user.is_process_user.return_value = True + + # WHEN + argv = self._signal_and_capture_argv(user, message_queue, queue_handler) + + # THEN + assert argv == ["/trusted/kill", "-s", "kill", "--", "-4321"] + assert "sudo" not in " ".join(argv) diff --git a/test/openjd/sessions_v0/test_system_commands.py b/test/openjd/sessions_v0/test_system_commands.py index 6e1118ee..01434c34 100644 --- a/test/openjd/sessions_v0/test_system_commands.py +++ b/test/openjd/sessions_v0/test_system_commands.py @@ -19,6 +19,7 @@ import pytest +from openjd.sessions._os_checker import is_posix from openjd.sessions._system_commands import ( SystemCommandNotFoundError, TRUSTED_SYSTEM_DIRECTORIES, @@ -107,6 +108,11 @@ def test_returns_the_first_matching_directory(self, tmp_path: Path) -> None: # THEN assert result == str(first / "target-cmd") + @pytest.mark.skipif( + not is_posix(), + reason="On Windows os.access(X_OK) is true for any existing file, so " + "'not executable' is not expressible there", + ) def test_ignores_a_non_executable_file(self, tmp_path: Path) -> None: # GIVEN (tmp_path / "target-cmd").write_text("not executable") @@ -128,6 +134,11 @@ class TestRejectsNonBareNames: pytest.param("", id="empty"), pytest.param(".", id="curdir"), pytest.param("..", id="pardir"), + # ntpath.join(r"C:\Windows\System32", "D:evil") == "D:evil" -- a + # drive-relative name discards the trusted prefix while containing no + # separator, so a separator-only guard lets it through. + pytest.param("D:evil", id="drive-relative"), + pytest.param("a:b", id="colon"), ], ) def test_rejects_name_with_a_path_component(self, name: str) -> None: @@ -179,11 +190,31 @@ def test_raises_rather_than_returning_the_bare_name(self) -> None: assert "openjd-definitely-not-installed" in message assert "PATH is deliberately not searched" in message - def test_is_not_a_filenotfounderror(self) -> None: - """The subprocess machinery catches OSError subclasses to mean "the thing I - tried to launch is missing, carry on degraded". An unavailable privileged - helper must not be absorbed by that handling.""" - assert not issubclass(SystemCommandNotFoundError, OSError) + def test_is_an_oserror_so_the_cancel_path_can_absorb_it(self) -> None: + """This assertion is the inverse of what an earlier revision asserted, and + the reversal is the point. + + That revision made the error a plain ``Exception`` on the theory that an + unavailable privileged helper must not be absorbed by "carry on degraded" + handlers. The theory was never checked against the handlers themselves. + ``_runner_base``'s cancel path catches ``OSError`` around + ``notify()``/``terminate()`` deliberately, so that failing to signal does + not unwind an in-progress cancelation -- its comment says "a cancel path is + the wrong place to raise". A non-OSError escapes that guard and costs the + cancel its bookkeeping. + + Signalling reaches ``system_command_path("sudo")``, so this is a live path, + not a hypothetical one. + """ + assert issubclass(SystemCommandNotFoundError, OSError) + assert issubclass(SystemCommandNotFoundError, FileNotFoundError) + + def test_is_still_distinguishable_from_a_plain_exec_failure(self) -> None: + """Being a FileNotFoundError must not cost a caller the ability to tell + "no trusted directory has it" apart from "exec failed".""" + assert SystemCommandNotFoundError is not FileNotFoundError + with pytest.raises(SystemCommandNotFoundError): + system_command_path("openjd-definitely-not-installed") class TestTrustedDirectories: @@ -204,7 +235,25 @@ def test_searches_both_sbin_locations(self) -> None: assert "/usr/sbin" in TRUSTED_SYSTEM_DIRECTORIES assert "/sbin" in TRUSTED_SYSTEM_DIRECTORIES + def test_the_two_nixos_entries_are_present_as_a_pair(self) -> None: + """/run/wrappers/bin alone supports no complete code path. + It holds only the setuid wrappers, so on NixOS it resolves `sudo` and + nothing else -- /usr/bin has just `env`, /bin just `sh`, and the sbin + directories are absent. `setsid` and `pgrep` are in the sw/bin symlink + farm. Keeping the wrapper entry without that one would resolve `sudo` and + then fail on `setsid` one line later, so the ordering comment would be + describing support that does not exist. + """ + assert "/run/wrappers/bin" in TRUSTED_SYSTEM_DIRECTORIES + assert "/run/current-system/sw/bin" in TRUSTED_SYSTEM_DIRECTORIES + + +@pytest.mark.skipif( + not is_posix(), + reason="TRUSTED_SYSTEM_DIRECTORIES is a POSIX layout; on Windows none of these " + "directories exist and every lookup is expected to raise", +) class TestRealCommandsResolve: """Positive controls against the real filesystem. From a1448816daecf631d91e7627d86d0f6434454bc5 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:50:46 -0700 Subject: [PATCH 5/9] test: pin wrapper-before-symlink-farm ordering on NixOS Review finding. /run/current-system/sw/bin also contains a sudo, but a non-setuid one, because nix store paths cannot carry the setuid bit. That is the reason the wrapper directory exists at all. _is_executable_file checks only that the candidate is a file with an execute bit, so it cannot tell an elevating sudo from a non-elevating one. Tuple order is the only thing that decides which wins, and the existing assertion covers only wrapper-before-/usr/bin, which does not constrain the pair because sw/bin sits between them. Getting the order backwards would resolve a sudo that cannot elevate, so cross-user sessions would fail on a host where the correct binary was present the whole time. Mutation-checked: swapping the two entries fails test_the_setuid_wrapper_precedes_the_nixos_symlink_farm and nothing else, against a green baseline with the source restored afterwards. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../sessions_v0/test_system_commands.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/openjd/sessions_v0/test_system_commands.py b/test/openjd/sessions_v0/test_system_commands.py index 01434c34..cdcfad06 100644 --- a/test/openjd/sessions_v0/test_system_commands.py +++ b/test/openjd/sessions_v0/test_system_commands.py @@ -235,6 +235,26 @@ def test_searches_both_sbin_locations(self) -> None: assert "/usr/sbin" in TRUSTED_SYSTEM_DIRECTORIES assert "/sbin" in TRUSTED_SYSTEM_DIRECTORIES + def test_the_setuid_wrapper_precedes_the_nixos_symlink_farm(self) -> None: + """Order between the two NixOS entries decides which `sudo` wins. + + `/run/current-system/sw/bin` also contains a `sudo`, but a non-setuid one: + nix store paths cannot carry the setuid bit, which is the whole reason the + wrapper directory exists. `_is_executable_file` checks only that the + candidate is a file with an execute bit, so it cannot tell the two apart -- + tuple order is the only thing that does. + + Getting this backwards would resolve a `sudo` that cannot elevate, so + cross-user sessions would fail on a host where the correct binary was + present all along. The wrapper-before-/usr/bin assertion below does not + cover it, because sw/bin sits between them. + """ + directories = TRUSTED_SYSTEM_DIRECTORIES + + assert directories.index("/run/wrappers/bin") < directories.index( + "/run/current-system/sw/bin" + ) + def test_the_two_nixos_entries_are_present_as_a_pair(self) -> None: """/run/wrappers/bin alone supports no complete code path. From 98e15fe465521d1a8fa609dfb8a691cceb7a3965 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:08:05 -0700 Subject: [PATCH 6/9] docs: Reframe resolver comments around problem and solution Two changes to comments only. No behaviour change. Dropped the vulnerability-classification references. They named a taxonomy without telling a reader anything actionable about this code, and the comment reads better stating what goes wrong and what the module does about it. Dropped the "each is pinned by a test in " bookkeeping. It told the reader where tests live rather than why the code is shaped this way, and it goes stale the moment a test file moves. The properties themselves are still listed, now with the reason each one is easy to undo, which is the part that helps someone editing this later. The module docstrings now open with the problem, then the approach, then the three properties and what breaks if each is lost. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_subprocess.py | 4 +- src/openjd/sessions/_system_commands.py | 54 +++++++++---------- .../test_subprocess_process_group.py | 4 +- .../sessions_v0/test_system_commands.py | 16 +++--- 4 files changed, 38 insertions(+), 40 deletions(-) diff --git a/src/openjd/sessions/_subprocess.py b/src/openjd/sessions/_subprocess.py index dda295d0..3af0ba1b 100644 --- a/src/openjd/sessions/_subprocess.py +++ b/src/openjd/sessions/_subprocess.py @@ -959,8 +959,8 @@ def _posix_signal_subprocess( # In the cross-user branch it must stay a bare name. `sudo -i` simulates an # initial login: it starts the target user's login shell and hands it the # command via -c, so `kill` is a *shell builtin* there. Nothing is looked up - # on PATH, and -i has already reset the environment, so CWE-426 does not - # reach this position. Resolving it would instead invent a hard dependency + # on PATH, and -i has already reset the environment, so a job-controlled + # PATH cannot reach this position. Resolving it would invent a hard dependency # on kill(1) from procps, which Debian `-slim` images -- a common worker # base -- do not install, and would break every cross-user SIGKILL fallback # on such a host. diff --git a/src/openjd/sessions/_system_commands.py b/src/openjd/sessions/_system_commands.py index 893da454..ee6ed61b 100644 --- a/src/openjd/sessions/_system_commands.py +++ b/src/openjd/sessions/_system_commands.py @@ -2,34 +2,32 @@ """Resolution of system command names to absolute paths, without consulting PATH. -This module exists because of CWE-426 (Untrusted Search Path). A session runs -job-supplied actions, and the job controls the environment those actions run -with -- including ``PATH``. That same environment is handed to the ``Popen`` call -that launches our *own* privileged helpers (``sudo``, ``setsid``, ``kill``), so a -bare command name in that argv is resolved through a search path the job wrote. -A job that drops an executable named ``sudo`` early on ``PATH`` gets it run at -the session's privilege level. - -The fix is to never let a command name reach ``execvp``-style resolution. Every -name is resolved here instead, by scanning a fixed list of trusted absolute -directories. - -Three properties are load-bearing, and each is pinned by a mutation-checked test -in ``test/openjd/sessions_v0/test_system_commands.py``: - - -* **``PATH`` is never read.** Not directly, and not indirectly via - :func:`shutil.which` or ``command -v`` -- both of which resolve through - ``PATH`` and so would reintroduce the vulnerability while appearing to fix it. -* **Only paths under :data:`TRUSTED_SYSTEM_DIRECTORIES` are returned.** -* **A command that cannot be found raises.** Falling back to the bare name would - silently restore the vulnerability, which is the worst available failure mode - for this class of fix: the code would look fixed and behave as if it were not. - -This module is POSIX-oriented; :data:`TRUSTED_SYSTEM_DIRECTORIES` lists POSIX -locations. On Windows nothing will be found and the lookups raise, which is -correct for this library because the cross-user code paths that need these -commands are POSIX-only. +The problem: a session launches its own privileged helpers (``sudo``, ``setsid``, +``kill``) with the environment it also gives the job, and that environment +includes the job's ``PATH``. A bare command name in such an argv is resolved +through that ``PATH``, so a job that puts an executable named ``sudo`` early on it +has that executable run at the session's privilege level rather than the job +user's. + +The solution: never let a command name reach ``execvp``-style resolution. Callers +pass a bare name here and get back an absolute path found by scanning a fixed +list of trusted directories. + +Three properties make that work, and all three are easy to undo by accident: + +* ``PATH`` is never read. Not directly, and not through :func:`shutil.which` or + ``command -v``, which resolve via ``PATH`` and so would restore the original + behaviour while looking like a fix. +* Only paths under :data:`TRUSTED_SYSTEM_DIRECTORIES` are returned. A name + containing a path separator is rejected, because ``os.path.join`` would + otherwise let ``../../tmp/evil`` escape the directory being searched. +* A command that cannot be found raises. Returning the bare name as a fallback + would put resolution back on ``PATH`` while the code still read as though it + did not. + +This module is POSIX-oriented, and :data:`TRUSTED_SYSTEM_DIRECTORIES` lists POSIX +locations. On Windows nothing is found and the lookups raise, which suits this +library because the cross-user paths needing these commands are POSIX-only. """ from __future__ import annotations diff --git a/test/openjd/sessions_v0/test_subprocess_process_group.py b/test/openjd/sessions_v0/test_subprocess_process_group.py index 27b53b29..54a4f912 100644 --- a/test/openjd/sessions_v0/test_subprocess_process_group.py +++ b/test/openjd/sessions_v0/test_subprocess_process_group.py @@ -101,8 +101,8 @@ class TestSignalArgvCommandResolution: Cross-user goes through `sudo -u -i kill ...`. `sudo -i` simulates an initial login: it starts the target user's login shell and passes the command to it via -c, so `kill` is a **shell builtin** in that position. Nothing is - resolved on PATH and -i has already reset the environment, so CWE-426 does not - reach it. Resolving it to an absolute path would instead invent a hard + resolved on PATH and -i has already reset the environment, so a job-controlled + PATH cannot reach it. Resolving it to an absolute path would instead invent a hard dependency on kill(1) from procps, which Debian `-slim` images do not install, and would break every cross-user SIGKILL fallback on such a host. diff --git a/test/openjd/sessions_v0/test_system_commands.py b/test/openjd/sessions_v0/test_system_commands.py index cdcfad06..5feeab7e 100644 --- a/test/openjd/sessions_v0/test_system_commands.py +++ b/test/openjd/sessions_v0/test_system_commands.py @@ -2,14 +2,14 @@ """Tests for the trusted-path command resolver. -These pin the security properties of ``_system_commands``, the fix for the PATH -injection reported as HackerOne 3942741. Each has been mutation-checked: -reverting the corresponding production behaviour makes a named test here fail. - -Why this matters more than usual: 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 reads as fixed. Without a test that fails when the -trusted-directory scan is removed, the fix is an unverified claim. +``_system_commands`` fails silently when it fails at all: a resolver that quietly +falls back to ``PATH`` returns a working path for every command that is installed, +so it behaves identically to a correct one on any normal host. Only a caller with +an attacker-controlled ``PATH`` sees the difference. + +These tests exist so that the difference is observable here instead. Each one +fails if the behaviour it describes is removed, which is what makes the resolver's +guarantees checkable rather than asserted. """ import os From 3fcbe09cbb4c1aefb975c3fa5ebd6d820ca5b475 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:37:20 -0700 Subject: [PATCH 7/9] fix: do not cache failed command lookups Review finding, and a real availability bug rather than a style point. find_system_command was lru_cached, which caches None as readily as a path. A package manager replacing a binary unlinks and relinks it, so a lookup landing in that window finds nothing. Caching that answer made one unlucky moment permanent for the rest of a long-lived agent's life: every later cross-user launch would fail on a command sitting on disk the whole time. Replaced with a dict that stores successful lookups only. Successes stay valid for the life of the process and these lookups sit on process-launch and signal-delivery paths, so caching them is still worth it; re-walking a handful of directories on the miss path costs nothing worth having. clear_command_cache() replaces cache_clear() for tests that patch the directory list. 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. Mutation-checked: caching the miss as well fails test_a_command_that_appears_later_is_found and nothing else, against a green baseline with the source restored afterwards. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_system_commands.py | 34 +++++++++---- .../sessions_v0/test_system_commands.py | 48 +++++++++++++++++-- 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/src/openjd/sessions/_system_commands.py b/src/openjd/sessions/_system_commands.py index ee6ed61b..987dd333 100644 --- a/src/openjd/sessions/_system_commands.py +++ b/src/openjd/sessions/_system_commands.py @@ -33,11 +33,11 @@ from __future__ import annotations import os -from functools import lru_cache from typing import Optional, Tuple __all__ = [ "SystemCommandNotFoundError", + "clear_command_cache", "TRUSTED_SYSTEM_DIRECTORIES", "find_system_command", "system_command_path", @@ -124,7 +124,27 @@ def _is_executable_file(path: str) -> bool: return os.path.isfile(path) and os.access(path, os.X_OK) -@lru_cache(maxsize=None) +_RESOLVED_COMMANDS: dict[str, str] = {} +"""Cache of successful lookups only, keyed by bare command name. + +Successes are safe to keep for the life of the process: a resolved path stays +valid, and these lookups sit on process-launch and signal-delivery paths where a +directory walk per signal is wasteful. + +Failures are not cached, and that asymmetry is the point. A package manager +replacing a binary unlinks and relinks it, so a lookup landing in that window sees +nothing. Caching that answer would make one unlucky moment permanent for the rest +of a long-lived agent's life, turning a transient miss into a session that can +never start a cross-user process again. Re-walking a handful of directories on the +miss path costs nothing worth having. +""" + + +def clear_command_cache() -> None: + """Discard resolved paths. For tests that patch the trusted directory list.""" + _RESOLVED_COMMANDS.clear() + + def find_system_command(name: str) -> Optional[str]: """Return the absolute path to ``name``, or ``None`` if it is not installed. @@ -132,19 +152,17 @@ def find_system_command(name: str) -> Optional[str]: consulted. Use this for commands whose absence is tolerable; use :func:`system_command_path` when the command is required. - The result is cached: the filesystem layout does not change underneath a - running session, and these lookups sit on process-launch and - signal-delivery paths. Tests that patch - :data:`TRUSTED_SYSTEM_DIRECTORIES` must call - ``find_system_command.cache_clear()``. - Raises: ValueError: if ``name`` is not a bare command name. """ _validate_command_name(name) + cached = _RESOLVED_COMMANDS.get(name) + if cached is not None: + return cached for directory in TRUSTED_SYSTEM_DIRECTORIES: candidate = os.path.join(directory, name) if _is_executable_file(candidate): + _RESOLVED_COMMANDS[name] = candidate return candidate return None diff --git a/test/openjd/sessions_v0/test_system_commands.py b/test/openjd/sessions_v0/test_system_commands.py index 5feeab7e..53c684ae 100644 --- a/test/openjd/sessions_v0/test_system_commands.py +++ b/test/openjd/sessions_v0/test_system_commands.py @@ -22,6 +22,7 @@ from openjd.sessions._os_checker import is_posix from openjd.sessions._system_commands import ( SystemCommandNotFoundError, + clear_command_cache, TRUSTED_SYSTEM_DIRECTORIES, find_system_command, system_command_path, @@ -32,11 +33,11 @@ @pytest.fixture(autouse=True) def clear_resolver_cache(): - """``find_system_command`` is ``lru_cache``d, so a result computed under one - patched directory list would otherwise leak into the next test.""" - find_system_command.cache_clear() + """Successful lookups are cached, so a path resolved under one patched + directory list would otherwise leak into the next test.""" + clear_command_cache() yield - find_system_command.cache_clear() + clear_command_cache() @pytest.fixture @@ -297,3 +298,42 @@ def test_sudo_resolves_on_this_host(self) -> None: pytest.skip("sudo is not installed on this host") assert os.path.dirname(system_command_path("sudo")) in TRUSTED_SYSTEM_DIRECTORIES + + +class TestCacheDoesNotRememberAbsence: + """Successes are cached; failures are not. + + A package manager replacing a binary unlinks and relinks it, so a lookup + landing in that window finds nothing. Caching that answer would make one + unlucky moment permanent for the rest of a long-lived agent's life: every + later cross-user launch would fail on a command that is sitting on disk. + """ + + def test_a_command_that_appears_later_is_found(self, tmp_path: Path) -> None: + # GIVEN a lookup that misses, as it would mid-upgrade + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", (str(tmp_path),)): + assert find_system_command("target-cmd") is None + + # WHEN the binary appears + target = tmp_path / "target-cmd" + target.write_text("#!/bin/sh\ntrue\n") + target.chmod(target.stat().st_mode | stat.S_IXUSR) + + # THEN the next lookup finds it rather than repeating the cached miss + assert find_system_command("target-cmd") == str(target) + + def test_a_resolved_path_is_cached(self, executable_dir: Path) -> None: + """The other half of the asymmetry. Without this, 'do not cache misses' + could be satisfied by caching nothing at all, and the reason the cache + exists (these lookups sit on signal-delivery paths) would be lost.""" + # GIVEN one successful lookup + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", (str(executable_dir),)): + first = find_system_command("target-cmd") + assert first == str(executable_dir / "target-cmd") + + # WHEN the directory list no longer contains it, and the file is gone + (executable_dir / "target-cmd").unlink() + + # THEN the cached answer is still returned, without touching the filesystem + with patch(f"{_MODULE}.TRUSTED_SYSTEM_DIRECTORIES", ()): + assert find_system_command("target-cmd") == first From fe9c014dfeaca7445caf3f33d9d2388e018e4483 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:43:11 -0700 Subject: [PATCH 8/9] test: Wrap a RANGE_EXPR parameter in string() before repr_sh The `parameter_types::all_types_scenario.yaml` scenario has been failing CI with: Failed to parse interpolation expression at [378, 411]. No matching signature for repr_sh(range_expr) repr_sh(Param.RangeExprParam) `repr_sh` is registered for `(string)`, `(path)`, `(list[string])`, `(list[path])` and `(list[nulltype])`. It has no `range_expr` overload, so openjd-model 0.11.3 is right to reject this and the template was wrong. `string` does declare `(range_expr) -> string`, so wrapping resolves it, and every other non-string parameter on the surrounding lines already does exactly that: echo IntParam={{repr_sh(string(Param.IntParam))}} echo BoolParam={{repr_sh(string(Param.BoolParam))}} echo len-RangeExprParam={{repr_sh(string(len(Param.RangeExprParam)))}} The rendered output is unchanged, so the scenario's expected `RangeExprParam=10-15` still matches. Confirmed by running the scenario against a build that accepts both forms: it passes before and after, which is what makes this a signature fix rather than a behaviour change. This is the only affected call. The other bare `repr_sh(Param.*)` uses in the test scenarios are all `string` or `path` typed, which it accepts. Unrelated to the rest of this branch. It surfaces here only because CI resolves released openjd-model while a local editable checkout can be on an older build that still accepted the bare form. Separated into its own commit so it can be dropped or landed independently. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- .../scenarios/parameter_types/all_types_template.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/openjd/sessions_v1/scenarios/parameter_types/all_types_template.yaml b/test/openjd/sessions_v1/scenarios/parameter_types/all_types_template.yaml index 48d5a29e..57920844 100644 --- a/test/openjd/sessions_v1/scenarios/parameter_types/all_types_template.yaml +++ b/test/openjd/sessions_v1/scenarios/parameter_types/all_types_template.yaml @@ -56,7 +56,7 @@ steps: echo PathParam={{repr_sh(Param.PathParam)}} echo PathParam.name={{repr_sh(Param.PathParam.name)}} echo BoolParam={{repr_sh(string(Param.BoolParam))}} - echo RangeExprParam={{repr_sh(Param.RangeExprParam)}} + echo RangeExprParam={{repr_sh(string(Param.RangeExprParam))}} echo len-RangeExprParam={{repr_sh(string(len(Param.RangeExprParam)))}} echo ListStringParam={{repr_sh(string(Param.ListStringParam))}} echo ListStringParam-0={{repr_sh(Param.ListStringParam[0])}} From 76faa232d436cc7e36d52f90c3d3c369ed6ca76c Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:56:03 -0700 Subject: [PATCH 9/9] test: Use posixpath.isabs for the POSIX trusted-directory entries test_all_entries_are_absolute failed the windows-latest 3.13 leg on '/run/wrappers/bin is not absolute'. From Python 3.13, ntpath.isabs() treats a single-slash path as drive-relative rather than absolute, so os.path.isabs made this assertion a statement about the host running the tests instead of about the constant. Every entry in TRUSTED_SYSTEM_DIRECTORIES is a POSIX path, and the module is POSIX-oriented, so posixpath.isabs is the right question to ask. Verified the mechanism locally rather than inferring it: ntpath.isabs ('/run/wrappers/bin') is False while posixpath.isabs is True. Same fix as the sibling job-attachments change; I applied it there and missed the twin here, which is why it only surfaced on the Windows leg after the other failure stopped masking it via fail-fast. 975 passed, 40 skipped, 16 xfailed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- test/openjd/sessions_v0/test_system_commands.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/openjd/sessions_v0/test_system_commands.py b/test/openjd/sessions_v0/test_system_commands.py index 53c684ae..a0422fa2 100644 --- a/test/openjd/sessions_v0/test_system_commands.py +++ b/test/openjd/sessions_v0/test_system_commands.py @@ -13,6 +13,7 @@ """ import os +import posixpath import stat from pathlib import Path from unittest.mock import patch @@ -221,9 +222,16 @@ def test_is_still_distinguishable_from_a_plain_exec_failure(self) -> None: class TestTrustedDirectories: def test_all_entries_are_absolute(self) -> None: """A relative entry would resolve against the process working directory, - which a session changes.""" + which a session changes. + + posixpath rather than os.path: these entries are POSIX paths, and from Python + 3.13 ntpath.isabs() treats a single-slash path as drive-relative rather than + absolute. Using os.path made this a statement about the host running the + tests rather than about the constant, and it failed the windows-latest 3.13 + leg on "/run/wrappers/bin is not absolute". + """ for directory in TRUSTED_SYSTEM_DIRECTORIES: - assert os.path.isabs(directory), f"{directory} is not absolute" + assert posixpath.isabs(directory), f"{directory} is not absolute" def test_searches_the_setuid_wrapper_directory_before_usr_bin(self) -> None: assert TRUSTED_SYSTEM_DIRECTORIES.index(