From 440e51bff9f576cff5cab2736aace27b507eeac4 Mon Sep 17 00:00:00 2001 From: necusjz Date: Thu, 8 Oct 2026 19:25:04 +1100 Subject: [PATCH 1/4] feat: add safe executable --- doc/cli_subprocess_guidelines.md | 2 +- doc/secure_executable_invocation.md | 145 ++++++++++++++++++ .../azure/cli/core/tests/test_util.py | 57 ++++++- src/azure-cli-core/azure/cli/core/util.py | 69 +++++++++ .../azure/cli/command_modules/util/custom.py | 24 ++- 5 files changed, 292 insertions(+), 5 deletions(-) create mode 100644 doc/secure_executable_invocation.md diff --git a/doc/cli_subprocess_guidelines.md b/doc/cli_subprocess_guidelines.md index b504497ad12..926fa1930bd 100644 --- a/doc/cli_subprocess_guidelines.md +++ b/doc/cli_subprocess_guidelines.md @@ -70,7 +70,7 @@ All various kinds of `subprocess` Popen calling use cases can be easily adjusted Besides that, developers might need to know some parts of the accessibility in both `run_cmd` and `subprocess` 1. when calling shell built-in cmds, like `dir` or `echo`, using `shell=True` **in windows platform**, `subprocess` implicitly uses `cmd.exe`, while `run_cmd` asks developers to provide the `cmd.exe` as executable file specifically in the arg list's first item, like `["cmd.exe", "/c", "echo", "abc"]` 2. if developers want to find an easy way to split their current cmd string into list, **for unix-like platforms**, developers can apply [`shlex.split`](https://docs.python.org/3/library/shlex.html#shlex.split) for quick access. But a prepared cmd statement is still more recommended (for more info about prepared cmd statement, please read below sections). -3. if developers want to locate the target command's executable file, a tool developers can use is `shutil.which` that gives the full executable file path in system, like `shutil.which(git)` returns the full `git.exe` path in windows platform `C:\\Program Files\\Git\\cmd\\git.EXE`. +3. if developers want to locate the target command's executable file, a tool developers can use is `shutil.which` that gives the full executable file path in system, like `shutil.which(git)` returns the full `git.exe` path in windows platform `C:\\Program Files\\Git\\cmd\\git.EXE`. To additionally defend against binary planting (CWE-427), resolve the executable with `azure.cli.core.util.get_safe_executable_path`, which excludes the current working directory. See [secure_executable_invocation.md](./secure_executable_invocation.md). 4. if the target cmd is `az`-related, like `az group show --name xxxx`, please see the following "`az`-related cmd execution" section. ### `az`-related command execution diff --git a/doc/secure_executable_invocation.md b/doc/secure_executable_invocation.md new file mode 100644 index 00000000000..7ff626c79c7 --- /dev/null +++ b/doc/secure_executable_invocation.md @@ -0,0 +1,145 @@ +# Secure Executable Invocation (Preventing Binary Planting) + +This guide explains how Azure CLI protects against **binary planting** (a.k.a. executable +hijacking, [CWE-427: Uncontrolled Search Path Element](https://cwe.mitre.org/data/definitions/427.html)) +and how command-module owners should remediate bare executable invocations such as `cmd.exe`, +`docker`, `git`, `gh`, and `kubectl`. + +> TL;DR: Never spawn a bare executable name. Resolve it to an absolute path from **trusted** +> directories first (excluding the current working directory), then invoke that absolute path. +> Use `azure.cli.core.util.get_safe_executable_path`. + +## The problem + +When you launch a process by a bare name, the operating system decides *which* file on disk to run +by walking a search path. On Windows this search is especially dangerous: + +- `CreateProcess` (and therefore `subprocess` with `shell=False`) resolves the **current working + directory first**, before the system directory and `PATH`. +- `cmd.exe` (used implicitly by `shell=True`) applies the same current-directory-first rule. + +So if a victim `cd`s into an attacker-controlled directory (a cloned repo, an unzipped artifact) +that contains a planted `cmd.exe`, `az.cmd`, `docker.exe`, `git.exe`, `gh.exe`, or `kubectl.exe`, +a command that spawns that bare name will execute the **attacker's** binary inside the victim's +Azure CLI session — leading to arbitrary (and potentially elevated) code execution. + +```python +# ❌ Vulnerable: the first token is resolved against the current working directory first on Windows. +subprocess.check_output(['az', 'version', '-o', 'json'], shell=platform.system() == 'Windows') +subprocess.run(['cmd.exe', '/c', 'echo', 'hi'], shell=False) +run_cmd(['docker', 'ps']) +``` + +`shell=False` does **not** help — the current-directory-first resolution happens in the OS, not the +shell. + +## The fix: resolve to an absolute path from trusted directories + +Azure CLI Core provides a single, reusable resolution helper: + +```python +from azure.cli.core.util import get_safe_executable_path +``` + +### `get_safe_executable_path(executable) -> str` + +Resolves a bare executable **name** to an absolute path, searching only trusted directories: + +1. If the input already contains a directory component, it is returned unchanged (nothing to do). +2. On Windows, the Windows system directory (`%SystemRoot%\System32`) and `%SystemRoot%` are + searched first, so system binaries like `cmd.exe` always resolve there. +3. `PATH` entries are searched next — **but the current working directory (and empty `PATH` + entries, which Windows treats as the current directory) are excluded.** +4. Windows `PATHEXT` extensions are expanded, so `az` correctly resolves to `az.cmd` / `az.exe`. +5. If resolution fails (the binary is not on `PATH`), the original name is returned as a safe + fallback — callers keep their previous behavior and existing error handling. + +```python +# ✅ Safe: resolve the absolute path first, then invoke it. +from azure.cli.core.util import get_safe_executable_path + +docker = get_safe_executable_path('docker') # e.g. C:\Program Files\Docker\docker.exe +run_cmd([docker, 'ps']) +``` + +### Invoking `az` itself (or any batch script) on Windows + +On Windows, `az` is a batch script (`az.cmd`) that must be run through `cmd.exe`. Resolve **both** +`cmd.exe` and `az` to absolute paths and avoid `shell=True`: + +```python +import platform +from azure.cli.core.util import get_safe_executable_path + +def _get_az_cmd_prefix(): + if platform.system() == 'Windows': + return [get_safe_executable_path('cmd.exe'), '/c', get_safe_executable_path('az')] + return [get_safe_executable_path('az')] + +# az version +subprocess.check_output(_get_az_cmd_prefix() + ['version', '-o', 'json']) # shell=False +``` + +See `src/azure-cli/azure/cli/command_modules/util/custom.py` for the real implementation used by +`az upgrade`. + +> For `az`-related work, prefer an in-process call (SDK, codegen atomic command, or +> `run_az_cmd`) over spawning a subprocess. Spawning a fresh `az` process is only appropriate when +> you specifically need a clean interpreter — for example after an in-place upgrade. See +> [cli_subprocess_guidelines.md](./cli_subprocess_guidelines.md). + +## Remediation checklist for command-module owners + +Address binary planting in your command module by reviewing every bare executable invocation. +Known affected binaries include `cmd.exe`, `docker`, `git`, `gh`, and `kubectl`. + +1. **Find the sinks.** Search your module for subprocess spawns: + ```bash + grep -rnE "(subprocess\.(run|call|check_call|check_output|Popen)|run_cmd)\(\s*\[?['\"](cmd\.exe|docker|git|gh|kubectl)" src/azure-cli/azure/cli/command_modules/ + ``` +2. **Resolve before spawning.** Replace the bare name with + `get_safe_executable_path('')` and pass the result as the first list element. +3. **Drop `shell=True`.** Pass arguments as a list with `shell=False` (the default). For batch + scripts on Windows, invoke them through a resolved absolute `cmd.exe /c`. +4. **Keep args as a list.** Never build a command string from user input. Continue to use + `run_cmd` / `run_az_cmd` where applicable. +5. **Regression-test from a hostile directory.** Add a test that places a decoy executable in the + current working directory and asserts it is **not** selected (see + `TestSafeExecutablePath` in `src/azure-cli-core/azure/cli/core/tests/test_util.py`). + +### Before / after + +```python +# ❌ Before +run_cmd(['docker', 'ps']) +subprocess.call(['kubectl', 'get', 'pods']) +run_cmd(['gh', 'auth', 'status'], capture_output=True) + +# ✅ After +from azure.cli.core.util import get_safe_executable_path +run_cmd([get_safe_executable_path('docker'), 'ps']) +subprocess.call([get_safe_executable_path('kubectl'), 'get', 'pods']) +run_cmd([get_safe_executable_path('gh'), 'auth', 'status'], capture_output=True) +``` + +## Existing precedents in the codebase + +Several modules already defend against this class of issue. Prefer the shared Core helper for new +code so the logic stays centralized, but these are useful references: + +| Location | Mechanism | Notes | +| --- | --- | --- | +| `azure.cli.core.util.get_safe_executable_path` | Resolve to absolute path, exclude CWD | **Preferred shared helper for new code.** | +| `command_modules/util/custom.py` (`_get_az_cmd_prefix`) | Resolve `cmd.exe` + `az`, drop `shell=True` | Used by `az upgrade`. | +| `command_modules/acr/custom.py` (`_validate_command_path`) | Reject a binary that resolves into the CWD | Honors `DOCKER_COMMAND` override. | +| `command_modules/acs/custom.py` (`which`) | Manual `PATH` lookup (no CWD prepend) | `kubectl` location. | + +## Summary + +- Binary planting lets an attacker run their executable from a victim's working directory. +- `shell=False` alone does not protect you on Windows. +- Resolve bare executable names to absolute paths from trusted directories **before** spawning, + using `get_safe_executable_path`. +- Review `cmd.exe`, `docker`, `git`, `gh`, and `kubectl` invocations in your module and add a + regression test that proves a decoy in the CWD is never selected. + diff --git a/src/azure-cli-core/azure/cli/core/tests/test_util.py b/src/azure-cli-core/azure/cli/core/tests/test_util.py index 776d2c66739..ab33d358cce 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_util.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_util.py @@ -17,7 +17,8 @@ (get_file_json, truncate_text, shell_safe_json_parse, b64_to_hex, hash_string, random_string, open_page_in_browser, can_launch_browser, handle_exception, ConfiguredDefaultSetter, send_raw_request, should_disable_connection_verify, parse_proxy_resource_id, get_az_user_agent, get_az_rest_user_agent, - _get_parent_proc_name, is_wsl, run_cmd, run_az_cmd, roughly_parse_command, is_same_origin) + _get_parent_proc_name, is_wsl, run_cmd, run_az_cmd, roughly_parse_command, is_same_origin, + get_safe_executable_path) from azure.cli.core.mock import DummyCli @@ -742,5 +743,59 @@ def test_roughly_parse_command(self): self.assertEqual(roughly_parse_command(['--help']), '') # Starts with flag self.assertEqual(roughly_parse_command(['-h']), '') # Starts with short flag +class TestSafeExecutablePath(unittest.TestCase): + """Tests for get_safe_executable_path, which mitigates binary planting (CWE-427).""" + + def test_explicit_path_returned_as_is(self): + # A path that already includes a directory component should be returned unchanged. + explicit = os.path.join('some', 'dir', 'tool') + self.assertEqual(get_safe_executable_path(explicit), explicit) + + def test_current_working_directory_is_excluded(self): + # A matching executable planted in the current working directory must NOT be selected. + with tempfile.TemporaryDirectory() as tmp_dir: + exe_name = 'planted.exe' if platform.system() == 'Windows' else 'planted' + planted = os.path.join(tmp_dir, exe_name) + with open(planted, 'w', encoding='utf-8') as f: + f.write('') + os.chmod(planted, 0o755) + + orig_cwd = os.getcwd() + try: + os.chdir(tmp_dir) + # CWD is deliberately not part of the search set, so resolution falls back + # to the original bare name rather than the planted binary. + with mock.patch.dict(os.environ, {'PATH': tmp_dir}): + resolved = get_safe_executable_path(exe_name) + self.assertEqual(resolved, exe_name) + self.assertNotEqual(resolved, planted) + finally: + os.chdir(orig_cwd) + + def test_resolves_from_trusted_path_dir(self): + # An executable located in a trusted PATH directory (not the CWD) should be resolved + # to its absolute path. + with tempfile.TemporaryDirectory() as path_dir, tempfile.TemporaryDirectory() as work_dir: + exe_name = 'mytool.exe' if platform.system() == 'Windows' else 'mytool' + target = os.path.join(path_dir, exe_name) + with open(target, 'w', encoding='utf-8') as f: + f.write('') + os.chmod(target, 0o755) + + orig_cwd = os.getcwd() + try: + os.chdir(work_dir) + with mock.patch.dict(os.environ, {'PATH': path_dir}): + resolved = get_safe_executable_path(exe_name) + self.assertEqual(os.path.normcase(resolved), os.path.normcase(target)) + finally: + os.chdir(orig_cwd) + + @unittest.skipUnless(platform.system() == 'Windows', 'cmd.exe resolution is Windows-specific') + def test_cmd_exe_resolves_to_system32(self): + resolved = get_safe_executable_path('cmd.exe') + self.assertTrue(os.path.isabs(resolved)) + self.assertTrue(resolved.lower().endswith('cmd.exe')) + if __name__ == '__main__': unittest.main() diff --git a/src/azure-cli-core/azure/cli/core/util.py b/src/azure-cli-core/azure/cli/core/util.py index 0efad6ab8ad..54d42b1e950 100644 --- a/src/azure-cli-core/azure/cli/core/util.py +++ b/src/azure-cli-core/azure/cli/core/util.py @@ -1634,6 +1634,75 @@ def should_encrypt_token_cache(cli_ctx): return encrypt +def _get_trusted_search_dirs(): + """Return an ordered list of trusted directories to search for executables. + + The current working directory (and empty ``PATH`` entries, which Windows treats as the current + directory) are deliberately excluded so a planted binary there can never be selected. + """ + search_dirs = [] + if sys.platform.startswith('win'): + # Prefer the Windows system directory for system executables such as cmd.exe. + system_root = os.environ.get('SystemRoot') or os.environ.get('windir') or r'C:\Windows' + search_dirs.append(os.path.join(system_root, 'System32')) + search_dirs.append(system_root) + + cwd = os.path.abspath(os.getcwd()) + for directory in os.environ.get('PATH', os.defpath).split(os.pathsep): + directory = directory.strip('"') + if directory and os.path.abspath(directory) != cwd: + search_dirs.append(directory) + + return search_dirs + + +def _get_executable_candidates(executable): + """Return the candidate file names for an executable, expanding Windows PATHEXT extensions.""" + if not sys.platform.startswith('win'): + return [executable] + + pathext = [ext for ext in os.environ.get('PATHEXT', '.COM;.EXE;.BAT;.CMD').split(os.pathsep) if ext] + if os.path.splitext(executable)[1].lower() in [ext.lower() for ext in pathext]: + # The name already carries a recognized extension (e.g. cmd.exe); use it as-is. + return [executable] + + return [executable + ext for ext in pathext] + [executable] + + +def get_safe_executable_path(executable): + """Resolve an executable name to an absolute path using only trusted directories. + + This mitigates binary planting / executable hijacking (CWE-427). On Windows, ``CreateProcess`` + (and ``cmd.exe``) resolve a bare executable name such as ``cmd.exe`` or ``az`` against the + calling process's current directory *before* the system directory and ``PATH``. An attacker who + can drop a malicious ``cmd.exe`` / ``az.{exe,cmd,bat}`` into a directory the victim ``cd``'s into + could therefore get arbitrary code execution. Resolving to an absolute path from trusted + directories only (never the current working directory) removes that ambiguity. + + :param executable: The executable name to resolve, e.g. ``cmd.exe`` or ``az``. + :return: The resolved absolute path if found, otherwise the original ``executable`` value so + callers keep their existing (pre-resolution) behavior as a fallback. + """ + # Already an explicit path (absolute or containing a directory component): nothing to resolve. + if os.path.dirname(executable): + return executable + + candidates = _get_executable_candidates(executable) + seen = set() + for directory in _get_trusted_search_dirs(): + normalized = os.path.normcase(os.path.abspath(directory)) + if normalized in seen: + continue + seen.add(normalized) + for candidate in candidates: + full_path = os.path.join(directory, candidate) + if os.path.isfile(full_path) and os.access(full_path, os.X_OK): + return full_path + + # Fall back to the original name if resolution fails (e.g. executable not on PATH). + return executable + + def run_cmd(args, *, capture_output=False, timeout=None, check=False, encoding=None, env=None): """Run command in a subprocess. It reduces (not eliminates) shell injection by forcing args to be a list and shell=False. Other arguments are keyword-only. For their documentation, see diff --git a/src/azure-cli/azure/cli/command_modules/util/custom.py b/src/azure-cli/azure/cli/command_modules/util/custom.py index 138e1f51407..6d0ed559bdd 100644 --- a/src/azure-cli/azure/cli/command_modules/util/custom.py +++ b/src/azure-cli/azure/cli/command_modules/util/custom.py @@ -181,7 +181,10 @@ def upgrade_version(cmd, update_all=None, yes=None, allow_preview=None): # pyli importlib.reload(subprocess) importlib.reload(json) - version_result = subprocess.check_output(['az', 'version', '-o', 'json'], shell=platform.system() == 'Windows') + az_cmd_prefix = _get_az_cmd_prefix() + # shell=False is used together with absolute executable paths (see _get_az_cmd_prefix) to avoid + # binary planting from the current working directory (CWE-427). + version_result = subprocess.check_output(az_cmd_prefix + ['version', '-o', 'json']) version_json = json.loads(version_result) new_version = version_json['azure-cli-core'] @@ -196,16 +199,31 @@ def upgrade_version(cmd, update_all=None, yes=None, allow_preview=None): # pyli for ext_name in exts: try: logger.warning("Checking update for %s", ext_name) - cmds = ['az', 'extension', 'update', '-n', ext_name] + cmds = az_cmd_prefix + ['extension', 'update', '-n', ext_name] if allow_preview is not None: cmds += ["--allow-preview", str(allow_preview)] - subprocess.call(cmds, shell=platform.system() == 'Windows') + subprocess.call(cmds) except Exception as ex: # pylint: disable=broad-except msg = "Extension {} update failed during az upgrade. {}".format(ext_name, str(ex)) raise CLIError(msg) logger.warning("Upgrade finished.") +def _get_az_cmd_prefix(): + """Return the command prefix used to safely invoke 'az' in a subprocess. + + On Windows, 'az' is a batch script (az.cmd) that must be run through cmd.exe. Both cmd.exe and + az are resolved to absolute paths while excluding the current working directory, so a planted + cmd.exe / az.{exe,cmd,bat} in the working directory cannot be executed (CWE-427). On other + platforms 'az' is an executable on PATH and is resolved the same way. + """ + import platform + from azure.cli.core.util import get_safe_executable_path + if platform.system() == 'Windows': + return [get_safe_executable_path('cmd.exe'), '/c', get_safe_executable_path('az')] + return [get_safe_executable_path('az')] + + def _upgrade_on_windows(): """Download MSI to a temp folder and install it with msiexec.exe. Directly installing from URL may be blocked by policy: https://github.com/Azure/azure-cli/issues/19171 From 69e2becd3d706be8ef6dcd3e9a0b3ddaacfc8656 Mon Sep 17 00:00:00 2001 From: necusjz Date: Mon, 12 Oct 2026 15:58:59 +1100 Subject: [PATCH 2/4] chore: fallback to bare name --- doc/secure_executable_invocation.md | 36 ++++--- .../azure/cli/core/tests/test_util.py | 97 +++++++++++-------- src/azure-cli-core/azure/cli/core/util.py | 47 ++++++--- .../azure/cli/command_modules/util/custom.py | 16 ++- 4 files changed, 129 insertions(+), 67 deletions(-) diff --git a/doc/secure_executable_invocation.md b/doc/secure_executable_invocation.md index 7ff626c79c7..df9b5557e2f 100644 --- a/doc/secure_executable_invocation.md +++ b/doc/secure_executable_invocation.md @@ -43,38 +43,53 @@ from azure.cli.core.util import get_safe_executable_path ### `get_safe_executable_path(executable) -> str` -Resolves a bare executable **name** to an absolute path, searching only trusted directories: +Resolves a bare executable **name** to an absolute path, searching only trusted directories. It is +a drop-in replacement for `shutil.which` (same `None`-when-missing contract): 1. If the input already contains a directory component, it is returned unchanged (nothing to do). 2. On Windows, the Windows system directory (`%SystemRoot%\System32`) and `%SystemRoot%` are searched first, so system binaries like `cmd.exe` always resolve there. -3. `PATH` entries are searched next — **but the current working directory (and empty `PATH` - entries, which Windows treats as the current directory) are excluded.** -4. Windows `PATHEXT` extensions are expanded, so `az` correctly resolves to `az.cmd` / `az.exe`. -5. If resolution fails (the binary is not on `PATH`), the original name is returned as a safe - fallback — callers keep their previous behavior and existing error handling. +3. Absolute `PATH` entries are searched next. **The current working directory is skipped**, compared + by filesystem identity, so differences in case, short names (`PROGRA~1`) and junctions don't + matter. **Empty and relative entries** (`""`, `.`, `bin`) are skipped too, because they resolve + against the current directory. +4. A match whose symlink target lives in the current working directory is rejected. +5. Windows `PATHEXT` extensions are expanded, so `az` correctly resolves to `az.cmd` / `az.exe`. +6. If no trusted match exists, or the current directory can't be determined, it returns **`None`** + (fail closed). It never falls back to the bare name, because spawning a bare name brings back + the lookup this helper prevents. ```python # ✅ Safe: resolve the absolute path first, then invoke it. +from azure.cli.core.azclierror import ValidationError from azure.cli.core.util import get_safe_executable_path docker = get_safe_executable_path('docker') # e.g. C:\Program Files\Docker\docker.exe +if not docker: + raise ValidationError("Docker is not installed or not on PATH.") run_cmd([docker, 'ps']) ``` ### Invoking `az` itself (or any batch script) on Windows On Windows, `az` is a batch script (`az.cmd`) that must be run through `cmd.exe`. Resolve **both** -`cmd.exe` and `az` to absolute paths and avoid `shell=True`: +`cmd.exe` and `az` to absolute paths and avoid `shell=True`. If either can't be resolved, run the +CLI module with the current interpreter, which is also an absolute path: ```python import platform +import sys from azure.cli.core.util import get_safe_executable_path def _get_az_cmd_prefix(): + az_path = get_safe_executable_path('az') if platform.system() == 'Windows': - return [get_safe_executable_path('cmd.exe'), '/c', get_safe_executable_path('az')] - return [get_safe_executable_path('az')] + cmd_path = get_safe_executable_path('cmd.exe') + if az_path and cmd_path: + return [cmd_path, '/c', az_path] + elif az_path: + return [az_path] + return [sys.executable, '-m', 'azure.cli'] # az version subprocess.check_output(_get_az_cmd_prefix() + ['version', '-o', 'json']) # shell=False @@ -131,8 +146,6 @@ code so the logic stays centralized, but these are useful references: | --- | --- | --- | | `azure.cli.core.util.get_safe_executable_path` | Resolve to absolute path, exclude CWD | **Preferred shared helper for new code.** | | `command_modules/util/custom.py` (`_get_az_cmd_prefix`) | Resolve `cmd.exe` + `az`, drop `shell=True` | Used by `az upgrade`. | -| `command_modules/acr/custom.py` (`_validate_command_path`) | Reject a binary that resolves into the CWD | Honors `DOCKER_COMMAND` override. | -| `command_modules/acs/custom.py` (`which`) | Manual `PATH` lookup (no CWD prepend) | `kubectl` location. | ## Summary @@ -142,4 +155,3 @@ code so the logic stays centralized, but these are useful references: using `get_safe_executable_path`. - Review `cmd.exe`, `docker`, `git`, `gh`, and `kubectl` invocations in your module and add a regression test that proves a decoy in the CWD is never selected. - diff --git a/src/azure-cli-core/azure/cli/core/tests/test_util.py b/src/azure-cli-core/azure/cli/core/tests/test_util.py index ab33d358cce..dc5985bdd2d 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_util.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_util.py @@ -746,56 +746,71 @@ def test_roughly_parse_command(self): class TestSafeExecutablePath(unittest.TestCase): """Tests for get_safe_executable_path, which mitigates binary planting (CWE-427).""" + EXE_NAME = 'planted.exe' if platform.system() == 'Windows' else 'planted' + + def setUp(self): + self._orig_cwd = os.getcwd() + self._tmp = tempfile.TemporaryDirectory() + self.work_dir = self._tmp.name + + def tearDown(self): + os.chdir(self._orig_cwd) + self._tmp.cleanup() + + def _make_exe(self, directory): + path = os.path.join(directory, self.EXE_NAME) + with open(path, 'w', encoding='utf-8') as f: + f.write('') + os.chmod(path, 0o755) + return path + + def _resolve_from_cwd(self, path_value): + os.chdir(self.work_dir) + with mock.patch.dict(os.environ, {'PATH': path_value}): + return get_safe_executable_path(self.EXE_NAME) + def test_explicit_path_returned_as_is(self): - # A path that already includes a directory component should be returned unchanged. explicit = os.path.join('some', 'dir', 'tool') self.assertEqual(get_safe_executable_path(explicit), explicit) - def test_current_working_directory_is_excluded(self): - # A matching executable planted in the current working directory must NOT be selected. - with tempfile.TemporaryDirectory() as tmp_dir: - exe_name = 'planted.exe' if platform.system() == 'Windows' else 'planted' - planted = os.path.join(tmp_dir, exe_name) - with open(planted, 'w', encoding='utf-8') as f: - f.write('') - os.chmod(planted, 0o755) - - orig_cwd = os.getcwd() - try: - os.chdir(tmp_dir) - # CWD is deliberately not part of the search set, so resolution falls back - # to the original bare name rather than the planted binary. - with mock.patch.dict(os.environ, {'PATH': tmp_dir}): - resolved = get_safe_executable_path(exe_name) - self.assertEqual(resolved, exe_name) - self.assertNotEqual(resolved, planted) - finally: - os.chdir(orig_cwd) - - def test_resolves_from_trusted_path_dir(self): - # An executable located in a trusted PATH directory (not the CWD) should be resolved - # to its absolute path. - with tempfile.TemporaryDirectory() as path_dir, tempfile.TemporaryDirectory() as work_dir: - exe_name = 'mytool.exe' if platform.system() == 'Windows' else 'mytool' - target = os.path.join(path_dir, exe_name) - with open(target, 'w', encoding='utf-8') as f: - f.write('') - os.chmod(target, 0o755) - - orig_cwd = os.getcwd() - try: - os.chdir(work_dir) - with mock.patch.dict(os.environ, {'PATH': path_dir}): - resolved = get_safe_executable_path(exe_name) - self.assertEqual(os.path.normcase(resolved), os.path.normcase(target)) - finally: - os.chdir(orig_cwd) + def test_cwd_on_path_is_excluded(self): + self._make_exe(self.work_dir) + self.assertIsNone(self._resolve_from_cwd(self.work_dir)) + + def test_relative_path_entry_is_excluded(self): + # '.' resolves against the current working directory, so it must never be trusted. + self._make_exe(self.work_dir) + self.assertIsNone(self._resolve_from_cwd('.')) + + @unittest.skipUnless(platform.system() == 'Windows', 'Case-insensitive paths are Windows-specific') + def test_cwd_excluded_regardless_of_case(self): + self._make_exe(self.work_dir) + self.assertIsNone(self._resolve_from_cwd(self.work_dir.swapcase())) + + def test_cwd_skipped_and_trusted_dir_used(self): + # A decoy in the CWD is skipped and the real binary later on PATH is selected. + self._make_exe(self.work_dir) + with tempfile.TemporaryDirectory() as path_dir: + target = self._make_exe(path_dir) + resolved = self._resolve_from_cwd(os.pathsep.join([self.work_dir, path_dir])) + self.assertEqual(os.path.normcase(resolved), os.path.normcase(target)) + + def test_not_found_returns_none(self): + with tempfile.TemporaryDirectory() as path_dir: + self.assertIsNone(self._resolve_from_cwd(path_dir)) + + def test_unknown_cwd_fails_closed(self): + with tempfile.TemporaryDirectory() as path_dir: + self._make_exe(path_dir) + with mock.patch.dict(os.environ, {'PATH': path_dir}), \ + mock.patch('os.getcwd', side_effect=OSError): + self.assertIsNone(get_safe_executable_path(self.EXE_NAME)) @unittest.skipUnless(platform.system() == 'Windows', 'cmd.exe resolution is Windows-specific') def test_cmd_exe_resolves_to_system32(self): resolved = get_safe_executable_path('cmd.exe') self.assertTrue(os.path.isabs(resolved)) - self.assertTrue(resolved.lower().endswith('cmd.exe')) + self.assertTrue(resolved.lower().endswith(os.path.join('system32', 'cmd.exe'))) if __name__ == '__main__': unittest.main() diff --git a/src/azure-cli-core/azure/cli/core/util.py b/src/azure-cli-core/azure/cli/core/util.py index 54d42b1e950..8b17216bb34 100644 --- a/src/azure-cli-core/azure/cli/core/util.py +++ b/src/azure-cli-core/azure/cli/core/util.py @@ -1634,11 +1634,26 @@ def should_encrypt_token_cache(cli_ctx): return encrypt +def _is_same_dir(dir1, dir2): + """Compare directories by filesystem identity, which handles case differences, short names + (e.g. PROGRA~1), junctions and mapped drives. Falls back to normalized string comparison.""" + try: + return os.path.samefile(dir1, dir2) + + except (OSError, ValueError): + return os.path.normcase(os.path.normpath(dir1)) == os.path.normcase(os.path.normpath(dir2)) + + +def _is_located_in_dir(path, directory): + """Whether ``path`` or its symlink target lives directly in ``directory``.""" + return any(_is_same_dir(os.path.dirname(p), directory) for p in (path, os.path.realpath(path))) + + def _get_trusted_search_dirs(): - """Return an ordered list of trusted directories to search for executables. + """Return an ordered list of candidate directories to search for executables. - The current working directory (and empty ``PATH`` entries, which Windows treats as the current - directory) are deliberately excluded so a planted binary there can never be selected. + Empty and relative ``PATH`` entries are skipped because both resolve against the current working + directory. The current working directory itself is filtered out by the caller. """ search_dirs = [] if sys.platform.startswith('win'): @@ -1647,10 +1662,9 @@ def _get_trusted_search_dirs(): search_dirs.append(os.path.join(system_root, 'System32')) search_dirs.append(system_root) - cwd = os.path.abspath(os.getcwd()) for directory in os.environ.get('PATH', os.defpath).split(os.pathsep): directory = directory.strip('"') - if directory and os.path.abspath(directory) != cwd: + if os.path.isabs(directory): search_dirs.append(directory) return search_dirs @@ -1680,27 +1694,36 @@ def get_safe_executable_path(executable): directories only (never the current working directory) removes that ambiguity. :param executable: The executable name to resolve, e.g. ``cmd.exe`` or ``az``. - :return: The resolved absolute path if found, otherwise the original ``executable`` value so - callers keep their existing (pre-resolution) behavior as a fallback. + :return: The resolved absolute path, or ``None`` if no trusted match exists (same contract as + ``shutil.which``). Never fall back to the bare name: spawning it would reintroduce the + current-directory lookup this function exists to prevent. """ # Already an explicit path (absolute or containing a directory component): nothing to resolve. if os.path.dirname(executable): return executable + try: + cwd = os.getcwd() + + except OSError: + # Cannot verify that a candidate is outside the working directory: fail closed. + logger.debug("Unable to determine the current working directory; not resolving '%s'.", executable) + return None + candidates = _get_executable_candidates(executable) seen = set() for directory in _get_trusted_search_dirs(): - normalized = os.path.normcase(os.path.abspath(directory)) - if normalized in seen: + normalized = os.path.normcase(os.path.normpath(directory)) + if normalized in seen or _is_same_dir(directory, cwd): continue + seen.add(normalized) for candidate in candidates: full_path = os.path.join(directory, candidate) - if os.path.isfile(full_path) and os.access(full_path, os.X_OK): + if (os.path.isfile(full_path) and os.access(full_path, os.X_OK) and not _is_located_in_dir(full_path, cwd)): return full_path - # Fall back to the original name if resolution fails (e.g. executable not on PATH). - return executable + return None def run_cmd(args, *, capture_output=False, timeout=None, check=False, encoding=None, env=None): diff --git a/src/azure-cli/azure/cli/command_modules/util/custom.py b/src/azure-cli/azure/cli/command_modules/util/custom.py index 6d0ed559bdd..1b3794c405b 100644 --- a/src/azure-cli/azure/cli/command_modules/util/custom.py +++ b/src/azure-cli/azure/cli/command_modules/util/custom.py @@ -216,12 +216,24 @@ def _get_az_cmd_prefix(): az are resolved to absolute paths while excluding the current working directory, so a planted cmd.exe / az.{exe,cmd,bat} in the working directory cannot be executed (CWE-427). On other platforms 'az' is an executable on PATH and is resolved the same way. + + If no trusted 'az' (or cmd.exe) can be resolved, fall back to running the CLI module with the + current interpreter, which is also an absolute path and never consults the working directory. """ import platform + import sys from azure.cli.core.util import get_safe_executable_path + + az_path = get_safe_executable_path('az') if platform.system() == 'Windows': - return [get_safe_executable_path('cmd.exe'), '/c', get_safe_executable_path('az')] - return [get_safe_executable_path('az')] + cmd_path = get_safe_executable_path('cmd.exe') + if az_path and cmd_path: + return [cmd_path, '/c', az_path] + + elif az_path: + return [az_path] + + return [sys.executable, '-m', 'azure.cli'] def _upgrade_on_windows(): From f9753f709581501446b4567b8d0e5f83b81172f0 Mon Sep 17 00:00:00 2001 From: necusjz Date: Tue, 13 Oct 2026 13:32:35 +1100 Subject: [PATCH 3/4] chore: safe execution for `msiexec.exe` --- src/azure-cli/azure/cli/command_modules/util/custom.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/azure-cli/azure/cli/command_modules/util/custom.py b/src/azure-cli/azure/cli/command_modules/util/custom.py index 1b3794c405b..11838cece8d 100644 --- a/src/azure-cli/azure/cli/command_modules/util/custom.py +++ b/src/azure-cli/azure/cli/command_modules/util/custom.py @@ -261,7 +261,14 @@ def _upgrade_on_windows(): msi_path = _download_from_url(msi_url, msi_dir) - subprocess.Popen(['msiexec.exe', '/i', msi_path]) + # Resolve msiexec.exe to an absolute path (System32) so a planted msiexec.exe in the current + # working directory cannot be executed (CWE-427). + from azure.cli.core.util import get_safe_executable_path + msiexec_path = get_safe_executable_path('msiexec.exe') + if not msiexec_path: + raise CLIError("Unable to locate msiexec.exe. Please install the downloaded MSI manually: {}".format(msi_path)) + + subprocess.Popen([msiexec_path, '/i', msi_path]) logger.warning("Installation started. Please complete the upgrade in the opened window.\nTo update extensions, " "please run `az upgrade` again after completing the upgrade.") sys.exit(0) From 27eb97328762c15d864441c255522115a48f9d16 Mon Sep 17 00:00:00 2001 From: necusjz Date: Tue, 13 Oct 2026 18:21:57 +1100 Subject: [PATCH 4/4] refactor: clean code --- doc/cli_subprocess_guidelines.md | 4 +- doc/secure_executable_invocation.md | 160 +++--------------- .../azure/cli/core/tests/test_util.py | 57 +++---- src/azure-cli-core/azure/cli/core/util.py | 100 +++-------- .../azure/cli/command_modules/util/custom.py | 23 +-- 5 files changed, 71 insertions(+), 273 deletions(-) diff --git a/doc/cli_subprocess_guidelines.md b/doc/cli_subprocess_guidelines.md index 926fa1930bd..fa64892509a 100644 --- a/doc/cli_subprocess_guidelines.md +++ b/doc/cli_subprocess_guidelines.md @@ -68,9 +68,9 @@ All various kinds of `subprocess` Popen calling use cases can be easily adjusted #### Notes Besides that, developers might need to know some parts of the accessibility in both `run_cmd` and `subprocess` -1. when calling shell built-in cmds, like `dir` or `echo`, using `shell=True` **in windows platform**, `subprocess` implicitly uses `cmd.exe`, while `run_cmd` asks developers to provide the `cmd.exe` as executable file specifically in the arg list's first item, like `["cmd.exe", "/c", "echo", "abc"]` +1. when calling shell built-in cmds, like `dir` or `echo`, using `shell=True` **in windows platform**, `subprocess` implicitly uses `cmd.exe`, while `run_cmd` asks developers to provide the `cmd.exe` as executable file specifically in the arg list's first item, like `[get_safe_executable_path("cmd.exe"), "/c", "echo", "abc"]` 2. if developers want to find an easy way to split their current cmd string into list, **for unix-like platforms**, developers can apply [`shlex.split`](https://docs.python.org/3/library/shlex.html#shlex.split) for quick access. But a prepared cmd statement is still more recommended (for more info about prepared cmd statement, please read below sections). -3. if developers want to locate the target command's executable file, a tool developers can use is `shutil.which` that gives the full executable file path in system, like `shutil.which(git)` returns the full `git.exe` path in windows platform `C:\\Program Files\\Git\\cmd\\git.EXE`. To additionally defend against binary planting (CWE-427), resolve the executable with `azure.cli.core.util.get_safe_executable_path`, which excludes the current working directory. See [secure_executable_invocation.md](./secure_executable_invocation.md). +3. to locate the target command's executable file, use `azure.cli.core.util.get_safe_executable_path` instead of `shutil.which`. It returns the full path (e.g. `C:\\Program Files\\Git\\cmd\\git.EXE` for `git`) or `None`, and never resolves from the current directory, which prevents binary planting (see [secure_executable_invocation.md](./secure_executable_invocation.md)). 4. if the target cmd is `az`-related, like `az group show --name xxxx`, please see the following "`az`-related cmd execution" section. ### `az`-related command execution diff --git a/doc/secure_executable_invocation.md b/doc/secure_executable_invocation.md index df9b5557e2f..6dc7e4a3f02 100644 --- a/doc/secure_executable_invocation.md +++ b/doc/secure_executable_invocation.md @@ -1,157 +1,49 @@ # Secure Executable Invocation (Preventing Binary Planting) -This guide explains how Azure CLI protects against **binary planting** (a.k.a. executable -hijacking, [CWE-427: Uncontrolled Search Path Element](https://cwe.mitre.org/data/definitions/427.html)) -and how command-module owners should remediate bare executable invocations such as `cmd.exe`, -`docker`, `git`, `gh`, and `kubectl`. - -> TL;DR: Never spawn a bare executable name. Resolve it to an absolute path from **trusted** -> directories first (excluding the current working directory), then invoke that absolute path. -> Use `azure.cli.core.util.get_safe_executable_path`. - -## The problem - -When you launch a process by a bare name, the operating system decides *which* file on disk to run -by walking a search path. On Windows this search is especially dangerous: - -- `CreateProcess` (and therefore `subprocess` with `shell=False`) resolves the **current working - directory first**, before the system directory and `PATH`. -- `cmd.exe` (used implicitly by `shell=True`) applies the same current-directory-first rule. - -So if a victim `cd`s into an attacker-controlled directory (a cloned repo, an unzipped artifact) -that contains a planted `cmd.exe`, `az.cmd`, `docker.exe`, `git.exe`, `gh.exe`, or `kubectl.exe`, -a command that spawns that bare name will execute the **attacker's** binary inside the victim's -Azure CLI session — leading to arbitrary (and potentially elevated) code execution. +On Windows, a bare executable name (`cmd.exe`, `az`, `docker`, `git`, `gh`, `kubectl`, ...) is looked up in the +**current working directory first**, both by `CreateProcess` (`shell=False`) and by `cmd.exe` (`shell=True`). +If a user runs a command from an untrusted directory (e.g. a cloned repo) containing a planted binary, Azure CLI +would execute it ([CWE-427](https://cwe.mitre.org/data/definitions/427.html)). ```python -# ❌ Vulnerable: the first token is resolved against the current working directory first on Windows. -subprocess.check_output(['az', 'version', '-o', 'json'], shell=platform.system() == 'Windows') -subprocess.run(['cmd.exe', '/c', 'echo', 'hi'], shell=False) +# ❌ Vulnerable +subprocess.check_output(['az', 'version'], shell=True) run_cmd(['docker', 'ps']) ``` -`shell=False` does **not** help — the current-directory-first resolution happens in the OS, not the -shell. - -## The fix: resolve to an absolute path from trusted directories - -Azure CLI Core provides a single, reusable resolution helper: - -```python -from azure.cli.core.util import get_safe_executable_path -``` - -### `get_safe_executable_path(executable) -> str` +## Fix -Resolves a bare executable **name** to an absolute path, searching only trusted directories. It is -a drop-in replacement for `shutil.which` (same `None`-when-missing contract): +Resolve the name to an absolute path with `azure.cli.core.util.get_safe_executable_path` before spawning it. +It works like `shutil.which`, except that it: -1. If the input already contains a directory component, it is returned unchanged (nothing to do). -2. On Windows, the Windows system directory (`%SystemRoot%\System32`) and `%SystemRoot%` are - searched first, so system binaries like `cmd.exe` always resolve there. -3. Absolute `PATH` entries are searched next. **The current working directory is skipped**, compared - by filesystem identity, so differences in case, short names (`PROGRA~1`) and junctions don't - matter. **Empty and relative entries** (`""`, `.`, `bin`) are skipped too, because they resolve - against the current directory. -4. A match whose symlink target lives in the current working directory is rejected. -5. Windows `PATHEXT` extensions are expanded, so `az` correctly resolves to `az.cmd` / `az.exe`. -6. If no trusted match exists, or the current directory can't be determined, it returns **`None`** - (fail closed). It never falls back to the bare name, because spawning a bare name brings back - the lookup this helper prevents. +- searches `%SystemRoot%\System32` and `%SystemRoot%` first on Windows, then `PATH`; +- skips the current working directory as well as relative `PATH` entries (`""`, `.`); +- returns `None` when nothing trusted is found or the working directory is unknown. It never falls back to the + bare name. ```python -# ✅ Safe: resolve the absolute path first, then invoke it. +# ✅ Safe from azure.cli.core.azclierror import ValidationError -from azure.cli.core.util import get_safe_executable_path +from azure.cli.core.util import get_safe_executable_path, run_cmd -docker = get_safe_executable_path('docker') # e.g. C:\Program Files\Docker\docker.exe +docker = get_safe_executable_path('docker') if not docker: raise ValidationError("Docker is not installed or not on PATH.") run_cmd([docker, 'ps']) ``` -### Invoking `az` itself (or any batch script) on Windows - -On Windows, `az` is a batch script (`az.cmd`) that must be run through `cmd.exe`. Resolve **both** -`cmd.exe` and `az` to absolute paths and avoid `shell=True`. If either can't be resolved, run the -CLI module with the current interpreter, which is also an absolute path: +To run `az` itself (a batch script, `az.cmd`, on Windows), resolve both `cmd.exe` and `az`, and fall back to +`[sys.executable, '-m', 'azure.cli']`. See `_get_az_cmd_prefix` in +`src/azure-cli/azure/cli/command_modules/util/custom.py`. Prefer in-process calls over spawning `az`; see +[cli_subprocess_guidelines.md](./cli_subprocess_guidelines.md). -```python -import platform -import sys -from azure.cli.core.util import get_safe_executable_path - -def _get_az_cmd_prefix(): - az_path = get_safe_executable_path('az') - if platform.system() == 'Windows': - cmd_path = get_safe_executable_path('cmd.exe') - if az_path and cmd_path: - return [cmd_path, '/c', az_path] - elif az_path: - return [az_path] - return [sys.executable, '-m', 'azure.cli'] - -# az version -subprocess.check_output(_get_az_cmd_prefix() + ['version', '-o', 'json']) # shell=False -``` +## Checklist for module owners -See `src/azure-cli/azure/cli/command_modules/util/custom.py` for the real implementation used by -`az upgrade`. - -> For `az`-related work, prefer an in-process call (SDK, codegen atomic command, or -> `run_az_cmd`) over spawning a subprocess. Spawning a fresh `az` process is only appropriate when -> you specifically need a clean interpreter — for example after an in-place upgrade. See -> [cli_subprocess_guidelines.md](./cli_subprocess_guidelines.md). - -## Remediation checklist for command-module owners - -Address binary planting in your command module by reviewing every bare executable invocation. -Known affected binaries include `cmd.exe`, `docker`, `git`, `gh`, and `kubectl`. - -1. **Find the sinks.** Search your module for subprocess spawns: +1. Find bare-name spawns in your module: ```bash - grep -rnE "(subprocess\.(run|call|check_call|check_output|Popen)|run_cmd)\(\s*\[?['\"](cmd\.exe|docker|git|gh|kubectl)" src/azure-cli/azure/cli/command_modules/ + grep -rnE "(subprocess\.(run|call|check_call|check_output|Popen)|run_cmd)\(\s*\[?['\"](cmd\.exe|docker|git|gh|kubectl)" src/azure-cli/azure/cli/command_modules/ ``` -2. **Resolve before spawning.** Replace the bare name with - `get_safe_executable_path('')` and pass the result as the first list element. -3. **Drop `shell=True`.** Pass arguments as a list with `shell=False` (the default). For batch - scripts on Windows, invoke them through a resolved absolute `cmd.exe /c`. -4. **Keep args as a list.** Never build a command string from user input. Continue to use - `run_cmd` / `run_az_cmd` where applicable. -5. **Regression-test from a hostile directory.** Add a test that places a decoy executable in the - current working directory and asserts it is **not** selected (see +2. Replace each bare name with `get_safe_executable_path('')` and handle `None`. +3. Pass arguments as a list and drop `shell=True`. +4. Add a test that puts a decoy executable in the CWD and checks that it isn't selected (see `TestSafeExecutablePath` in `src/azure-cli-core/azure/cli/core/tests/test_util.py`). - -### Before / after - -```python -# ❌ Before -run_cmd(['docker', 'ps']) -subprocess.call(['kubectl', 'get', 'pods']) -run_cmd(['gh', 'auth', 'status'], capture_output=True) - -# ✅ After -from azure.cli.core.util import get_safe_executable_path -run_cmd([get_safe_executable_path('docker'), 'ps']) -subprocess.call([get_safe_executable_path('kubectl'), 'get', 'pods']) -run_cmd([get_safe_executable_path('gh'), 'auth', 'status'], capture_output=True) -``` - -## Existing precedents in the codebase - -Several modules already defend against this class of issue. Prefer the shared Core helper for new -code so the logic stays centralized, but these are useful references: - -| Location | Mechanism | Notes | -| --- | --- | --- | -| `azure.cli.core.util.get_safe_executable_path` | Resolve to absolute path, exclude CWD | **Preferred shared helper for new code.** | -| `command_modules/util/custom.py` (`_get_az_cmd_prefix`) | Resolve `cmd.exe` + `az`, drop `shell=True` | Used by `az upgrade`. | - -## Summary - -- Binary planting lets an attacker run their executable from a victim's working directory. -- `shell=False` alone does not protect you on Windows. -- Resolve bare executable names to absolute paths from trusted directories **before** spawning, - using `get_safe_executable_path`. -- Review `cmd.exe`, `docker`, `git`, `gh`, and `kubectl` invocations in your module and add a - regression test that proves a decoy in the CWD is never selected. diff --git a/src/azure-cli-core/azure/cli/core/tests/test_util.py b/src/azure-cli-core/azure/cli/core/tests/test_util.py index dc5985bdd2d..d7642fc965f 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_util.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_util.py @@ -744,72 +744,53 @@ def test_roughly_parse_command(self): self.assertEqual(roughly_parse_command(['-h']), '') # Starts with short flag class TestSafeExecutablePath(unittest.TestCase): - """Tests for get_safe_executable_path, which mitigates binary planting (CWE-427).""" - - EXE_NAME = 'planted.exe' if platform.system() == 'Windows' else 'planted' + EXE = 'planted.exe' if platform.system() == 'Windows' else 'planted' def setUp(self): self._orig_cwd = os.getcwd() self._tmp = tempfile.TemporaryDirectory() - self.work_dir = self._tmp.name + self.cwd = self._tmp.name + os.chdir(self.cwd) + self._make_exe(self.cwd) # decoy in the CWD def tearDown(self): os.chdir(self._orig_cwd) self._tmp.cleanup() def _make_exe(self, directory): - path = os.path.join(directory, self.EXE_NAME) - with open(path, 'w', encoding='utf-8') as f: - f.write('') + path = os.path.join(directory, self.EXE) + open(path, 'w').close() os.chmod(path, 0o755) return path - def _resolve_from_cwd(self, path_value): - os.chdir(self.work_dir) - with mock.patch.dict(os.environ, {'PATH': path_value}): - return get_safe_executable_path(self.EXE_NAME) + def _resolve(self, *path_dirs): + with mock.patch.dict(os.environ, {'PATH': os.pathsep.join(path_dirs)}): + return get_safe_executable_path(self.EXE) def test_explicit_path_returned_as_is(self): explicit = os.path.join('some', 'dir', 'tool') self.assertEqual(get_safe_executable_path(explicit), explicit) - def test_cwd_on_path_is_excluded(self): - self._make_exe(self.work_dir) - self.assertIsNone(self._resolve_from_cwd(self.work_dir)) - - def test_relative_path_entry_is_excluded(self): - # '.' resolves against the current working directory, so it must never be trusted. - self._make_exe(self.work_dir) - self.assertIsNone(self._resolve_from_cwd('.')) - - @unittest.skipUnless(platform.system() == 'Windows', 'Case-insensitive paths are Windows-specific') - def test_cwd_excluded_regardless_of_case(self): - self._make_exe(self.work_dir) - self.assertIsNone(self._resolve_from_cwd(self.work_dir.swapcase())) + def test_cwd_never_resolved(self): + self.assertIsNone(self._resolve(self.cwd)) + self.assertIsNone(self._resolve('.')) + if platform.system() == 'Windows': + self.assertIsNone(self._resolve(self.cwd.swapcase())) - def test_cwd_skipped_and_trusted_dir_used(self): - # A decoy in the CWD is skipped and the real binary later on PATH is selected. - self._make_exe(self.work_dir) + def test_trusted_dir_used_after_cwd(self): with tempfile.TemporaryDirectory() as path_dir: target = self._make_exe(path_dir) - resolved = self._resolve_from_cwd(os.pathsep.join([self.work_dir, path_dir])) - self.assertEqual(os.path.normcase(resolved), os.path.normcase(target)) - - def test_not_found_returns_none(self): - with tempfile.TemporaryDirectory() as path_dir: - self.assertIsNone(self._resolve_from_cwd(path_dir)) + self.assertEqual(os.path.normcase(self._resolve(self.cwd, path_dir)), os.path.normcase(target)) def test_unknown_cwd_fails_closed(self): with tempfile.TemporaryDirectory() as path_dir: self._make_exe(path_dir) - with mock.patch.dict(os.environ, {'PATH': path_dir}), \ - mock.patch('os.getcwd', side_effect=OSError): - self.assertIsNone(get_safe_executable_path(self.EXE_NAME)) + with mock.patch('os.getcwd', side_effect=OSError): + self.assertIsNone(self._resolve(path_dir)) - @unittest.skipUnless(platform.system() == 'Windows', 'cmd.exe resolution is Windows-specific') + @unittest.skipUnless(platform.system() == 'Windows', 'Windows only') def test_cmd_exe_resolves_to_system32(self): resolved = get_safe_executable_path('cmd.exe') - self.assertTrue(os.path.isabs(resolved)) self.assertTrue(resolved.lower().endswith(os.path.join('system32', 'cmd.exe'))) if __name__ == '__main__': diff --git a/src/azure-cli-core/azure/cli/core/util.py b/src/azure-cli-core/azure/cli/core/util.py index 8b17216bb34..3960de13d14 100644 --- a/src/azure-cli-core/azure/cli/core/util.py +++ b/src/azure-cli-core/azure/cli/core/util.py @@ -1634,95 +1634,35 @@ def should_encrypt_token_cache(cli_ctx): return encrypt -def _is_same_dir(dir1, dir2): - """Compare directories by filesystem identity, which handles case differences, short names - (e.g. PROGRA~1), junctions and mapped drives. Falls back to normalized string comparison.""" - try: - return os.path.samefile(dir1, dir2) - - except (OSError, ValueError): - return os.path.normcase(os.path.normpath(dir1)) == os.path.normcase(os.path.normpath(dir2)) - - -def _is_located_in_dir(path, directory): - """Whether ``path`` or its symlink target lives directly in ``directory``.""" - return any(_is_same_dir(os.path.dirname(p), directory) for p in (path, os.path.realpath(path))) - - -def _get_trusted_search_dirs(): - """Return an ordered list of candidate directories to search for executables. - - Empty and relative ``PATH`` entries are skipped because both resolve against the current working - directory. The current working directory itself is filtered out by the caller. - """ - search_dirs = [] - if sys.platform.startswith('win'): - # Prefer the Windows system directory for system executables such as cmd.exe. - system_root = os.environ.get('SystemRoot') or os.environ.get('windir') or r'C:\Windows' - search_dirs.append(os.path.join(system_root, 'System32')) - search_dirs.append(system_root) - - for directory in os.environ.get('PATH', os.defpath).split(os.pathsep): - directory = directory.strip('"') - if os.path.isabs(directory): - search_dirs.append(directory) - - return search_dirs - - -def _get_executable_candidates(executable): - """Return the candidate file names for an executable, expanding Windows PATHEXT extensions.""" - if not sys.platform.startswith('win'): - return [executable] - - pathext = [ext for ext in os.environ.get('PATHEXT', '.COM;.EXE;.BAT;.CMD').split(os.pathsep) if ext] - if os.path.splitext(executable)[1].lower() in [ext.lower() for ext in pathext]: - # The name already carries a recognized extension (e.g. cmd.exe); use it as-is. - return [executable] - - return [executable + ext for ext in pathext] + [executable] - - def get_safe_executable_path(executable): - """Resolve an executable name to an absolute path using only trusted directories. - - This mitigates binary planting / executable hijacking (CWE-427). On Windows, ``CreateProcess`` - (and ``cmd.exe``) resolve a bare executable name such as ``cmd.exe`` or ``az`` against the - calling process's current directory *before* the system directory and ``PATH``. An attacker who - can drop a malicious ``cmd.exe`` / ``az.{exe,cmd,bat}`` into a directory the victim ``cd``'s into - could therefore get arbitrary code execution. Resolving to an absolute path from trusted - directories only (never the current working directory) removes that ambiguity. - - :param executable: The executable name to resolve, e.g. ``cmd.exe`` or ``az``. - :return: The resolved absolute path, or ``None`` if no trusted match exists (same contract as - ``shutil.which``). Never fall back to the bare name: spawning it would reintroduce the - current-directory lookup this function exists to prevent. + """Resolve an executable name to an absolute path like ``shutil.which``, but never from the current working + directory, which Windows searches first for bare names (binary planting, CWE-427). + Return ``None`` if nothing trusted is found; never fall back to the bare name. """ - # Already an explicit path (absolute or containing a directory component): nothing to resolve. if os.path.dirname(executable): return executable - try: cwd = os.getcwd() - except OSError: - # Cannot verify that a candidate is outside the working directory: fail closed. - logger.debug("Unable to determine the current working directory; not resolving '%s'.", executable) - return None + return None # Fail closed: the directory to exclude is unknown. - candidates = _get_executable_candidates(executable) - seen = set() - for directory in _get_trusted_search_dirs(): - normalized = os.path.normcase(os.path.normpath(directory)) - if normalized in seen or _is_same_dir(directory, cwd): + search_dirs = os.environ.get('PATH', os.defpath).split(os.pathsep) + names = [executable] + if sys.platform.startswith('win'): + system_root = os.environ.get('SystemRoot', r'C:\Windows') + search_dirs = [os.path.join(system_root, 'System32'), system_root] + search_dirs + exts = [ext for ext in os.environ.get('PATHEXT', '.COM;.EXE;.BAT;.CMD').split(os.pathsep) if ext] + if os.path.splitext(executable)[1].upper() not in (ext.upper() for ext in exts): + names = [executable + ext for ext in exts] + + for directory in (d.strip('"') for d in search_dirs): + # Relative entries ('', '.') resolve against the CWD as well. + if not os.path.isabs(directory) or not os.path.isdir(directory) or os.path.samefile(directory, cwd): continue - - seen.add(normalized) - for candidate in candidates: - full_path = os.path.join(directory, candidate) - if (os.path.isfile(full_path) and os.access(full_path, os.X_OK) and not _is_located_in_dir(full_path, cwd)): - return full_path - + for name in names: + path = os.path.join(directory, name) + if os.path.isfile(path) and os.access(path, os.X_OK): + return path return None diff --git a/src/azure-cli/azure/cli/command_modules/util/custom.py b/src/azure-cli/azure/cli/command_modules/util/custom.py index 11838cece8d..10808dfc8c5 100644 --- a/src/azure-cli/azure/cli/command_modules/util/custom.py +++ b/src/azure-cli/azure/cli/command_modules/util/custom.py @@ -182,8 +182,6 @@ def upgrade_version(cmd, update_all=None, yes=None, allow_preview=None): # pyli importlib.reload(json) az_cmd_prefix = _get_az_cmd_prefix() - # shell=False is used together with absolute executable paths (see _get_az_cmd_prefix) to avoid - # binary planting from the current working directory (CWE-427). version_result = subprocess.check_output(az_cmd_prefix + ['version', '-o', 'json']) version_json = json.loads(version_result) new_version = version_json['azure-cli-core'] @@ -210,15 +208,8 @@ def upgrade_version(cmd, update_all=None, yes=None, allow_preview=None): # pyli def _get_az_cmd_prefix(): - """Return the command prefix used to safely invoke 'az' in a subprocess. - - On Windows, 'az' is a batch script (az.cmd) that must be run through cmd.exe. Both cmd.exe and - az are resolved to absolute paths while excluding the current working directory, so a planted - cmd.exe / az.{exe,cmd,bat} in the working directory cannot be executed (CWE-427). On other - platforms 'az' is an executable on PATH and is resolved the same way. - - If no trusted 'az' (or cmd.exe) can be resolved, fall back to running the CLI module with the - current interpreter, which is also an absolute path and never consults the working directory. + """Return an absolute-path command prefix for spawning 'az', so nothing is resolved from the CWD (CWE-427). + On Windows, az.cmd runs through cmd.exe. Fall back to the current interpreter if either can't be resolved. """ import platform import sys @@ -229,10 +220,8 @@ def _get_az_cmd_prefix(): cmd_path = get_safe_executable_path('cmd.exe') if az_path and cmd_path: return [cmd_path, '/c', az_path] - elif az_path: return [az_path] - return [sys.executable, '-m', 'azure.cli'] @@ -246,7 +235,7 @@ def _upgrade_on_windows(): import sys import tempfile - from azure.cli.core.util import rmtree_with_retry + from azure.cli.core.util import rmtree_with_retry, get_safe_executable_path if platform.architecture()[0] == '32bit': msi_url = 'https://aka.ms/installazurecliwindows' @@ -261,13 +250,9 @@ def _upgrade_on_windows(): msi_path = _download_from_url(msi_url, msi_dir) - # Resolve msiexec.exe to an absolute path (System32) so a planted msiexec.exe in the current - # working directory cannot be executed (CWE-427). - from azure.cli.core.util import get_safe_executable_path msiexec_path = get_safe_executable_path('msiexec.exe') if not msiexec_path: - raise CLIError("Unable to locate msiexec.exe. Please install the downloaded MSI manually: {}".format(msi_path)) - + raise CLIError("Unable to locate msiexec.exe. Please install the MSI manually: {}".format(msi_path)) subprocess.Popen([msiexec_path, '/i', msi_path]) logger.warning("Installation started. Please complete the upgrade in the opened window.\nTo update extensions, " "please run `az upgrade` again after completing the upgrade.")