diff --git a/README.md b/README.md index a88229d..8a0283b 100644 --- a/README.md +++ b/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 - + gpg> trust pub 4096R/BCC40987 created: 2024-02-09 expires: 2026-02-08 usage: SCEA trust: unknown validity: unknown [ unknown] (1). Open Job Description - + 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 Please note that the shown key validity is not necessarily correct unless you restart the program. - + gpg> quit ``` diff --git a/src/openjd/sessions/_scripts/_windows/_signal_win_subprocess.py b/src/openjd/sessions/_scripts/_windows/_signal_win_subprocess.py index c63a5f1..c26a977 100644 --- a/src/openjd/sessions/_scripts/_windows/_signal_win_subprocess.py +++ b/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(): diff --git a/src/openjd/sessions/_win32/_locate_executable.py b/src/openjd/sessions/_win32/_locate_executable.py index 9fb982f..e5b08d8 100644 --- a/src/openjd/sessions/_win32/_locate_executable.py +++ b/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,10 +45,9 @@ 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} @@ -55,6 +55,13 @@ def _locate_for_same_user( 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,6 +140,9 @@ 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: @@ -127,8 +150,30 @@ def _locate_for_other_user( 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) + ) diff --git a/src/openjd/sessions/_win32/_popen_as_user.py b/src/openjd/sessions/_win32/_popen_as_user.py index fd759ce..dc5ec9d 100644 --- a/src/openjd/sessions/_win32/_popen_as_user.py +++ b/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)