fix: resolve system commands from trusted dirs to prevent PATH injection #351
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
|
|
||
| """Resolution of system command names to absolute paths, without consulting PATH. | ||
|
|
||
| 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 | ||
|
|
||
| import os | ||
| from typing import Optional, Tuple | ||
|
|
||
| __all__ = [ | ||
| "SystemCommandNotFoundError", | ||
| "clear_command_cache", | ||
| "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", | ||
|
There was a problem hiding this comment.
The trusted-directory list is the entire trust boundary of this module, but membership is decided purely by path string — On a NixOS host that is exactly right. On every other host the directory is expected not to exist — but nothing here enforces that expectation, so if anything on the host (a container image with a loose Worth considering an
Contributor
Author
There was a problem hiding this comment. Correct, and parked rather than fixed. Membership is decided by path string alone: |
||
| # ...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", | ||
|
There was a problem hiding this comment. The ordering invariant that matters for the NixOS Adding On NixOS both of these exist:
The comment on line 58 also under-describes the entry: it explains why the directory is needed ( Suggest tightening the assertion to the invariant that has a failure mode — assert (This is also the concrete case for the ownership/mode check raised on the earlier revision:
Contributor
Author
There was a problem hiding this comment. Fixed in a144881. Correct that the existing assertion did not constrain the new entry, because sw/bin sits between the wrapper directory and |
||
| "/usr/bin", | ||
| "/bin", | ||
| # sbin entries are last: on non-usr-merged distributions (some Debian | ||
| # releases) some system commands exist only under /sbin. | ||
| "/usr/sbin", | ||
|
There was a problem hiding this comment. The NixOS case the ordering comment is written for still cannot launch a cross-user session, because The comment justifies putting
Everything else lives under Adding
Contributor
Author
There was a problem hiding this comment. Fixed in 68265a3 by adding |
||
| "/sbin", | ||
| ) | ||
|
|
||
|
|
||
| class SystemCommandNotFoundError(FileNotFoundError): | ||
| """A required system command was not present in any trusted directory. | ||
|
|
||
| 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. | ||
| """ | ||
|
There was a problem hiding this comment.
The docstring justifies the choice by saying callers "already catch
try:
if method == "notify":
process.notify()
else:
process.terminate()
except OSError as err: # pragma: nocover
self._logger.warning(f"Cancelation could not send {method} signal ...")Its own docstring states the intent explicitly: "a cancel path is the wrong place to raise: the caller is already unwinding a cancel, and losing the surrounding bookkeeping to a signal failure is worse than a warning."
Same for the If the intent is "cannot proceed safely, do not silently degrade", that still needs the raise to land somewhere that handles it — otherwise the failure mode moves from "cancel logged a warning" to "cancel aborted partway through". Worth either deriving from
Contributor
Author
There was a problem hiding this comment. Fixed in 68265a3. You were right and my original reasoning was wrong: I asserted that an unavailable privileged helper must not be absorbed by OSError handlers without checking what those handlers actually do. |
||
|
|
||
|
|
||
| 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. | ||
| # | ||
| # 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 or drive " | ||
| f"specifier, but got {name!r}." | ||
| ) | ||
|
|
||
|
|
||
| def _is_executable_file(path: str) -> bool: | ||
| return os.path.isfile(path) and os.access(path, os.X_OK) | ||
|
|
||
|
|
||
| _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. | ||
|
|
||
| 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. | ||
|
|
||
| Raises: | ||
| ValueError: if ``name`` is not a bare command name. | ||
| """ | ||
|
There was a problem hiding this comment. Negative results are cached for the life of the process.
The docstring justifies caching with "the filesystem layout does not change underneath a running session" — true for a hit (the resolved absolute path stays valid), but the absence of a file is not the same kind of stable fact. Caching only successful lookups would keep the stated performance benefit (the hot paths — process launch, signal delivery — are all hits) while making a miss retry: _cache: dict[str, str] = {}
def find_system_command(name: str) -> Optional[str]:
_validate_command_name(name)
if (cached := _cache.get(name)) is not None:
return cached
for directory in TRUSTED_SYSTEM_DIRECTORIES:
candidate = os.path.join(directory, name)
if _is_executable_file(candidate):
_cache[name] = candidate
return candidate
return None
Contributor
Author
There was a problem hiding this comment. Fixed in 3fcbe09, and thank you for this one, it was a genuine availability bug rather than a style point. |
||
| _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 | ||
|
|
||
|
|
||
| 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 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverage gap:
rm/powershellin the session-cleanup argv are still bare names.The new module states the invariant absolutely — "never let a command name reach
execvp-style resolution" — but one privileged argv is not converted._session.py:580-589builds the cleanup command that runs as the job user:That reaches
_start_subprocess, so on POSIX the argv becomes[<resolved sudo>, -u, user, -i, <resolved setsid>, -w, "rm", "-rf", ...]— the two helpers this PR hardened, followed by a barermthatsetsidresolves throughexecvp. In the same-user branchrmis argv[0] of thePopenitself. This is anrm -rfover the session directory, which makes it the highest-consequence bare name left in the tree.Two mitigations apply, and both are worth stating explicitly rather than leaving implicit:
sudo -iresets the environment (so the job'sPATHdoes not survive into the login shell), and thisLoggingSubprocessis constructed withoutos_env_vars, so the same-user branch inherits the agent's ownPATHrather than the job's. So this is not the same directly-job-controlled path the module's docstring describes.Still worth either resolving these two names too, or narrowing the module docstring's claim to the argv positions actually covered — as written, a future reader will reasonably conclude no bare command names remain on any privileged path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, and parked as out of scope for this change.
_session.pyis not a file this PR touches, and your own two mitigations are the reason I am comfortable deferring:sudo -iresets the environment before the login shell resolvesrm, and thatLoggingSubprocessis constructed withoutos_env_vars. It should be routed through the same resolver in a separate change against_session.py, where it can be reviewed on its own terms.