fix: resolve sudo and findmnt from trusted dirs to prevent PATH injection

src/deadline/job_attachments/_system_commands.py

@@ -0,0 +1,137 @@
+# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+
+"""Resolution of system command names to absolute paths, without consulting PATH.
+
+The problem: the VFS mount and unmount paths invoke ``sudo`` to act as the job
+user, and query mount state with ``findmnt``. Invoking those by bare name resolves
+them through ``PATH``, which makes the binary actually run depend on the search
+path of whatever launched the process, for commands that cross a user boundary.
+
+The solution: callers pass a bare name here and get back an absolute path found by
+scanning a fixed list of trusted directories, so ``PATH`` plays no part.
+
+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`,
+  which resolves 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 missing command raises. Returning the bare name as a fallback would put
+  resolution back on ``PATH`` while the code still read as though it did not.
+
+A resolver rather than absolute-path literals, because the locations are not
+universal: NixOS keeps the setuid ``sudo`` wrapper at ``/run/wrappers/bin/sudo``,
+so a hardcoded ``/usr/bin/sudo`` would leave those hosts unable to mount at all.
+"""
+
+from __future__ import annotations
+
+import os as _os
+from typing import Optional as _Optional, Tuple as _Tuple
+
+__all__ = [
+    "SystemCommandNotFoundError",
+    "TRUSTED_SYSTEM_DIRECTORIES",
+    "find_system_command",
+    "system_command_path",
+]
+
+
+TRUSTED_SYSTEM_DIRECTORIES: _Tuple[str, ...] = (
+    # Ordered, deliberately. On NixOS the setuid `sudo` wrapper lives here and the
+    # /usr/bin copy is absent or not setuid, so this must be searched first.
+    "/run/wrappers/bin",
+    # ...and these 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 are
+    # absent. `findmnt` lives in this symlink farm, which nixos-rebuild manages and
+    # root owns, so it is trust-equivalent to /usr/bin there. Without it the
+    # ordering above would resolve `sudo` and then fail on `findmnt`.
+    "/run/current-system/sw/bin",
+    "/usr/bin",
+    "/bin",
+    # sbin last: on non-usr-merged distributions some system commands exist only
+    # under /sbin.
+    "/usr/sbin",
+    "/sbin",
+)
+
+
+class SystemCommandNotFoundError(Exception):
+    """A required system command was not present in any trusted directory.
+
+    Deliberately not a :class:`FileNotFoundError`, because ``vfs`` already uses that
+    type for something else. Three ``except FileNotFoundError`` blocks there mean
+    "the VFS pid file is missing", and one of them wraps a call chain that reaches
+    this resolver: ``kill_all_processes`` -> ``shutdown_libfuse_mount`` ->
+    ``wait_for_mount`` -> ``is_mount``. Inheriting from ``FileNotFoundError`` let a
+    resolution failure be reported as a missing pid file and skip the cleanup that
+    follows.
+
+    Callers in ``vfs`` translate this into :class:`VFSExecutableMissingError`, which
+    is the type their own callers already handle by falling back to a copy-based
+    sync. This differs from the sibling resolver in ``openjd-sessions``, where
+    inheriting from ``OSError`` is correct because its cancel path deliberately
+    catches ``OSError`` so a failed signal cannot unwind a cancelation. Same
+    problem, opposite answer, because the surrounding handlers differ.
+    """
+
+
+def _validate_command_name(name: str) -> None:
+    """Reject anything that is not a bare command name."""
+    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.")
+    # Both separators are checked on both platforms. A backslash is a legal POSIX
+    # filename character, but no command resolved here contains one, and treating
+    # it as suspect keeps the check identical rather than subtly weaker on POSIX.
+    # 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 while containing no separator at all, so a
+    # separator-only check lets it through. posixpath 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)
+
+
+def find_system_command(name: str) -> _Optional[str]:
+    """Return the absolute path to ``name``, or ``None`` if it is not installed.
+
+    ``PATH`` is not consulted. Use this when the command's absence is tolerable;
+    use :func:`system_command_path` when it is required.
+
+    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 in no 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 searched."
+        )
+    return path

src/deadline/job_attachments/vfs.py

@@ -9,6 +9,15 @@
 import threading
 from typing import Callable, Dict, Union, Optional
 
+# Aliased under private names. Imported plainly, these two become public
+# attributes of `deadline.job_attachments.vfs`, which the API-surface check
+# reports as added public aliases -- an unintended widening of the package's
+# contract from what is meant to be an internal helper. All three are aliased,
+# including the exception: re-exporting it plainly failed the same gate a second
+# time, for the same reason.
+from ._system_commands import SystemCommandNotFoundError as _SystemCommandNotFoundError
+from ._system_commands import find_system_command as _find_system_command
+from ._system_commands import system_command_path as _system_command_path
 from .exceptions import (
     VFSExecutableMissingError,
     VFSFailedToMountError,
@@ -119,7 +128,24 @@
         if not os.path.exists(fusermount3_path):
             log.warning(f"fusermount3 not found at {cls.find_vfs_link_dir()}")
             return None
-        return ["sudo", "-u", os_user, fusermount3_path, "-u", mount_path]
+        # find_system_command rather than system_command_path: this function's
+        # contract is Optional[list], and it already answers "a binary I need is
+        # missing" with a warning and None just above. Raising for sudo instead
+        # would be a second, undeclared failure mode on the same line of code --
+        # and it escapes into shutdown_libfuse_mount's cleanup path, whose only
+        # handling for this function is the None check.
+        sudo_path = _find_system_command("sudo")
+        if sudo_path is None:
+            log.warning("sudo not found in any trusted directory; cannot unmount as the job user")
+            return None
+        return [
+            sudo_path,
+            "-u",
+            os_user,
+            fusermount3_path,
+            "-u",
+            mount_path,
+        ]
 
     @classmethod
     def shutdown_libfuse_mount(cls, mount_path: str, os_user: str, session_dir: Path) -> bool:
@@ -211,7 +237,24 @@
         os.path.ismount returns false for libfuse mounts owned by "other users",
         use findmnt instead
         """
-        return subprocess.run(["findmnt", path]).returncode == 0
+        return subprocess.run([cls._resolve_or_raise("findmnt"), path]).returncode == 0
+
+    @staticmethod
+    def _resolve_or_raise(name: str) -> str:
+        """Resolve a system command, reporting failure as VFSExecutableMissingError.
+
+        The translation is the point. Callers of the mount and unmount paths handle
+        ``VFSExecutableMissingError`` -- ``asset_sync`` catches it and falls back to a
+        copy-based sync -- and handle no other type for "a binary I need is not
+        here". Letting the resolver's own exception escape would either bypass that
+        fallback, or, if it inherited from ``FileNotFoundError``, be misreported by
+        the ``except FileNotFoundError`` blocks in this module that mean "the VFS pid
+        file is missing".
+        """
+        try:
+            return _system_command_path(name)
+        except _SystemCommandNotFoundError as e:
+            raise VFSExecutableMissingError(str(e)) from e
 
     @classmethod
     def wait_for_mount(cls, mount_path, session_dir, mount_wait_seconds=60, expected=True) -> bool:
@@ -268,7 +311,7 @@
         log_file_path = self.logs_folder_path(session_dir) / log_file_name
         log.log(log_level, f"Printing last {lines} lines from {log_file_path}")
         if not os.path.exists(log_file_path):
             log.warning(f"No log file found at {log_file_path}")
             return
         with open(log_file_path, "r") as log_file:
             for this_line in log_file.readlines()[lines * -1 :]:
@@ -291,7 +334,7 @@
         executable = VFSProcessManager.find_vfs_launch_script()
 
         command = (
-            f"sudo -E -u {self._os_user}"
+            f"{self._resolve_or_raise('sudo')} -E -u {self._os_user}"
             f" {executable} {mount_point} -f --clienttype=deadline"
             f" --bucket={self._asset_bucket}"
             f" --manifest={self._manifest_path}"
@@ -344,6 +387,20 @@
         Determine where the VFS executable we'll be launching lives so we can
         find the correct relative paths around it for LD_LIBRARY_PATH and config files
         :return: Path to VFS executable
+
+        Note this deliberately does not use the trusted-directory resolver in
+        ``_system_commands``, and the difference is worth understanding before
+        changing either one. That resolver exists for *system* commands, whose
+        locations are fixed and small in number. The VFS executable is a shipped
+        artifact whose location is a deployment choice: this function consults
+        ``shutil.which``, then ``$DEADLINE_VFS_INSTALL_PATH``, then a path relative to
+        the working directory, because a deployment may legitimately put it in any of
+        them.
+
+        So the search path for this binary, and for the launch script found by
+        :meth:`find_vfs_launch_script`, remains wider than for ``sudo`` or
+        ``findmnt``. Narrowing it is a separate question about how the VFS is
+        deployed, not about command resolution, and it is knowingly out of scope here.
         """
         if VFSProcessManager.exe_path is not None:
             log.info(f"Using saved path {VFSProcessManager.exe_path}")
@@ -417,7 +474,7 @@
         """
         if VFSProcessManager.cwd_path is None:
             exe_path = VFSProcessManager.find_vfs()
             # Use cwd one folder up from bin
             VFSProcessManager.cwd_path = os.path.normpath(
                 os.path.join(os.path.dirname(exe_path), "..")
             )
@@ -447,7 +504,7 @@
             log.error(f"Manifest not found at {self._manifest_path}")
             return
         if self._os_group is not None:
             try:
                 shutil.chown(self._manifest_path, group=self._os_group)
                 os.chmod(self._manifest_path, DEADLINE_MANIFEST_GROUP_READ_PERMS)
             except OSError as e: