fix!: Windows locate_executable finds wrong binary to run (#141)

This fixes issue #140, by calling a Python subprocess to
run `shutil.where` instead of a cmd subprocess to run `where`.

It is a breaking change, because it introduces a new
requirement that when impersonating a user for subprocesses, the
Python installation hosting the library can be run by the
impersonated user as well.

Signed-off-by: Mark Wiebe <[email protected]>

BREAKING CHANGE: If you are using impersonation on Windows, then you must
ensure that the Python installation hosting the library can be run by the impersonated
user as well.

README.md

@@ -26,12 +26,12 @@ This library requires:
 
 ## Versioning
 
-This package's version follows [Semantic Versioning 2.0](https://semver.org/), but is still considered to be in its 
+This package's version follows [Semantic Versioning 2.0](https://semver.org/), but is still considered to be in its
 initial development, thus backwards incompatible versions are denoted by minor version bumps. To help illustrate how
 versions will increment during this initial development stage, they are described below:
 
-1. The MAJOR version is currently 0, indicating initial development. 
-2. The MINOR version is currently incremented when backwards incompatible changes are introduced to the public API. 
+1. The MAJOR version is currently 0, indicating initial development.
+2. The MINOR version is currently incremented when backwards incompatible changes are introduced to the public API.
 3. The PATCH version is currently incremented when bug fixes or backwards compatible changes are introduced to the public API.
 
 ## Contributing
@@ -101,7 +101,7 @@ job_template = decode_job_template(
                 ],
                 "parameterSpace": {
                     "taskParameterDefinitions": [
-                        { "name": "Bar", "type": "INT", "range": "1-10" }   
+                        { "name": "Bar", "type": "INT", "range": "1-10" }
                     ]
                 },
                 "script": {
@@ -226,7 +226,7 @@ with Session(
     ...
 ```
 
-You must ensure that the `host` user is able to run commands as the `actions` user 
+You must ensure that the `host` user is able to run commands as the `actions` user
 with passwordless `sudo` by, for example, adding a rule like follows to your
 `sudoers` file or making the equivalent change in your user permissions directory:
 
@@ -259,6 +259,11 @@ with Session(
     ...
 ```
 
+You must ensure that the Python installation hosting this code can be run by any impersonated
+user in addition to the `host` user. The library makes impersonated subprocess calls to
+perform operations dependent on the impersonated user file system permissions, such as finding
+files in search paths.
+
 If running in a Windows Service, then you must ensure that:
 1. The `host` user is an Administrator, LocalSystem, or LocalService user as your
    security posture requires; and
@@ -288,7 +293,7 @@ For example, if you would like to verify your download of the wheel for version
 3) Save the following contents to a file called `openjobdescription-pgp.asc`:
     ```
     -----BEGIN PGP PUBLIC KEY BLOCK-----
-    
+
     mQINBGXGjx0BEACdChrQ/nch2aYGJ4fxHNQwlPE42jeHECqTdlc1V/mug+7qN7Pc
     C4NQk4t68Y72WX/NG49gRfpAxPlSeNt18c3vJ9/sWTukmonWYGK0jQGnDWjuVgFT
     XtvJAAQBFilQXN8h779Th2lEuD4bQX+mGB7l60Xvh7vIehE3C4Srbp6KJXskPLPo
@@ -335,36 +340,36 @@ For example, if you would like to verify your download of the wheel for version
     gpg (GnuPG) 2.0.22; Copyright (C) 2013 Free Software Foundation, Inc.
     This is free software: you are free to change and redistribute it.
     There is NO WARRANTY, to the extent permitted by law.
-    
-    
+
+
     pub  4096R/BCC40987  created: 2024-02-09  expires: 2026-02-08  usage: SCEA
                          trust: unknown       validity: unknown
     [ unknown] (1). Open Job Description <[email protected]>
-    
+
     gpg> trust
     pub  4096R/BCC40987  created: 2024-02-09  expires: 2026-02-08  usage: SCEA
                          trust: unknown       validity: unknown
     [ unknown] (1). Open Job Description <[email protected]>
-    
+
     Please decide how far you trust this user to correctly verify other users' keys
     (by looking at passports, checking fingerprints from different sources, etc.)
-    
+
       1 = I don't know or won't say
       2 = I do NOT trust
       3 = I trust marginally
       4 = I trust fully
       5 = I trust ultimately
       m = back to the main menu
-    
+
     Your decision? 5
     Do you really want to set this key to ultimate trust? (y/N) y
-    
+
     pub  4096R/BCC40987  created: 2024-02-09  expires: 2026-02-08  usage: SCEA
                          trust: ultimate      validity: unknown
     [ unknown] (1). Open Job Description <[email protected]>
     Please note that the shown key validity is not necessarily correct
     unless you restart the program.
-    
+
     gpg> quit
     ```
 

src/openjd/sessions/_scripts/_windows/_signal_win_subprocess.py

@@ -38,7 +38,6 @@
 
 
 def signal_process(pgid: int):
-
     # Send signal can only target processes in the same console.
     # We first detach from the current console and re-attach to that of process group.
     if not kernel32.FreeConsole():

src/openjd/sessions/_win32/_locate_executable.py

@@ -1,6 +1,7 @@
 # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 
 import os
+import sys
 import shutil
 from logging import INFO, LoggerAdapter, getLogger
 from logging.handlers import QueueHandler
@@ -44,17 +45,23 @@ def locate_windows_executable(
     return return_args
 
 
-def _locate_for_same_user(
-    command: Path, os_env_vars: Optional[dict[str, Optional[str]]], working_dir: str
-) -> str:
-    # Running as the same user, so we can use shutil.which.
+def _get_path_var_for_shutil_which(
+    os_env_vars: Optional[dict[str, Optional[str]]], working_dir: str
+):
     path_var: Optional[str] = None
     if os_env_vars:
         env_var_keys = {k.lower(): k for k in os_env_vars}
         path_var = os_env_vars.get(env_var_keys["path"]) if "path" in env_var_keys else None
     if path_var is None:
         path_var = os.environ.get("PATH", "")
     path_var = "%s;%s" % (working_dir, path_var)
+    return path_var
+
+
+def _locate_for_same_user(
+    command: Path, os_env_vars: Optional[dict[str, Optional[str]]], working_dir: str
+) -> str:
+    path_var: Optional[str] = _get_path_var_for_shutil_which(os_env_vars, working_dir)
     exe = str(shutil.which(str(command), path=path_var))
     if not exe:
         raise RuntimeError("Could not find executable file: %s" % command)
@@ -66,7 +73,7 @@ def _locate_for_other_user(
     os_env_vars: Optional[dict[str, Optional[str]]],
     working_dir: str,
     user: SessionUser,
-) -> str:
+) -> str:  # pragma: nocover
     # Running as a potentially different user, so it's possible that
     # this process doesn't have read access to the executable file's location.
     # Thus, we need to rely on running a subprocess as the user to be able
@@ -93,23 +100,36 @@ def _locate_for_other_user(
                 _internal_logger_queue.get(block=False)
         except Empty:
             pass  # Will happen when the queue is fully empty
+
+        # When running in a service context, we want to call the non-service Python binary
+        sys_executable = sys.executable.lower().replace("pythonservice.exe", "python.exe")
+
+        # In the subprocess code, we avoid exit code 1 as that is returned if a Python exception is thrown.
+        exit_code_success = 2
+        exit_code_could_not_find_exe = 3
+
+        path_var: Optional[str] = _get_path_var_for_shutil_which(os_env_vars, working_dir)
         process = LoggingSubprocess(
             logger=_internal_logger_adapter,
             args=[
-                str(Path(os.environ.get("WINDIR", r"C:\Windows")) / "System32" / "cmd.exe"),
-                "/C",
+                sys_executable,
+                "-c",
                 # Command injection here is possible, but it's irrelevant. The command is running
                 # as the given user. No need for an attacker to be fancy here, they could just run
                 # the desired attack command directly in the job template.
-                "where %s" % command,
+                "import shutil, sys, pathlib\n"
+                + f"cmd = shutil.which({str(command)!r}, path={path_var!r})\n"
+                + "if cmd:\n"
+                + "  print(str(pathlib.Path(cmd).absolute()))\n"
+                + f"  sys.exit({exit_code_success})\n"
+                + f"sys.exit({exit_code_could_not_find_exe})\n",
             ],
             user=user,
             os_env_vars=os_env_vars,
             working_dir=str(working_dir),
         )
         process.run()  # blocking call
-        if process.exit_code != 0:
-            raise RuntimeError("Could not find executable file: %s" % command)
+        exit_code = process.exit_code
 
         # We're seeing random errors when trying to run an Action's command immediately after this
         # outside of Session 0; theory is that maybe this has something to do with running two
@@ -120,15 +140,40 @@ def _locate_for_other_user(
         #  [WinError 1018] Illegal operation attempted on a registry key that has been marked for deletion
         del process
 
+        if exit_code == exit_code_could_not_find_exe:
+            raise RuntimeError(f"Could not find executable file: {command}")
+
         # Parse the output
         try:
             while True:
                 record = _internal_logger_queue.get(block=False)
                 message = record.getMessage()
                 if "Output:" in message:
                     break
-            exe_record = _internal_logger_queue.get(block=False)
-            # The first line of output from 'where' is the location of the command
-            return exe_record.getMessage()
+
+            if exit_code == exit_code_success:
+                exe_record = _internal_logger_queue.get(block=False)
+                # The line of output with the result of 'shutil.which' is the location of the command
+                return exe_record.getMessage()
+        except Empty:
+            raise RuntimeError(
+                f"Could not run Python as user {user.user} to find executable {command} in PATH.\n"
+                + f"The host configuration must allow users to run {sys_executable}."
+            )
+
+        # Collect the error output from the subprocess
+        error_messages = []
+        try:
+            while True:
+                record = _internal_logger_queue.get(block=False)
+                error_messages.append(record.getMessage())
         except Empty:
-            raise RuntimeError("Could not find executable file: %s" % command) from None  #
+            pass
+
+        # Something went wrong in launching sys_executable.
+        # Because this scenario may be difficult to diagnose, we include more context.
+        raise RuntimeError(
+            f"Could not run Python as user {user.user} to find executable {command} in PATH.\n"
+            + f"The host configuration must allow users to run {sys_executable}.\n\nError output:\n"
+            + "\n".join(error_messages)
+        )

src/openjd/sessions/_win32/_popen_as_user.py

@@ -180,7 +180,6 @@ def _merge_environment(
                     # Raises: OSError
                     raise ctypes.WinError()
             elif self.user.logon_token is not None:
-
                 siex = STARTUPINFOEX()
                 ctypes.memmove(
                     ctypes.pointer(siex.StartupInfo), ctypes.pointer(si), ctypes.sizeof(STARTUPINFO)