From fed2947a2996b8140b50768bff544bd9c8c22283 Mon Sep 17 00:00:00 2001 From: y5c4l3 Date: Wed, 25 Sep 2024 23:36:06 +0800 Subject: [PATCH] Quote template strings in activation script This patch adds `quote` method in `ViaTemplateActivator` so that the magic template strings can be quoted correctly when replacing. This mitigates potential command injection (#2768). Signed-off-by: y5c4l3 --- docs/changelog/2768.bugfix.rst | 2 ++ pyproject.toml | 3 +++ src/virtualenv/activation/bash/activate.sh | 8 +++---- src/virtualenv/activation/batch/__init__.py | 4 ++++ src/virtualenv/activation/cshell/activate.csh | 8 +++---- src/virtualenv/activation/fish/activate.fish | 8 +++---- src/virtualenv/activation/nushell/__init__.py | 19 +++++++++++++++++ src/virtualenv/activation/nushell/activate.nu | 8 +++---- .../activation/powershell/__init__.py | 12 +++++++++++ .../activation/powershell/activate.ps1 | 6 +++--- src/virtualenv/activation/python/__init__.py | 6 +++++- .../activation/python/activate_this.py | 8 +++---- src/virtualenv/activation/via_template.py | 13 +++++++++++- tests/conftest.py | 6 +++++- tests/unit/activation/conftest.py | 3 +-- tests/unit/activation/test_batch.py | 10 ++++----- tests/unit/activation/test_powershell.py | 21 +++++++++++++------ 17 files changed, 106 insertions(+), 39 deletions(-) create mode 100644 docs/changelog/2768.bugfix.rst diff --git a/docs/changelog/2768.bugfix.rst b/docs/changelog/2768.bugfix.rst new file mode 100644 index 000000000..ec7d15d08 --- /dev/null +++ b/docs/changelog/2768.bugfix.rst @@ -0,0 +1,2 @@ +Properly quote string placeholders in activation script templates to mitigate +potential command injection. - by :user:`y5c4l3`. \ No newline at end of file diff --git a/pyproject.toml b/pyproject.toml index de0e961fa..fabf43460 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -133,6 +133,9 @@ lint.ignore = [ "S404", # Using subprocess is alright "S603", # subprocess calls are fine ] +lint.per-file-ignores."src/virtualenv/activation/python/activate_this.py" = [ + "F821", # ignore undefined template string placeholders +] lint.per-file-ignores."tests/**/*.py" = [ "D", # don't care about documentation in tests "FBT", # don't care about booleans as positional arguments in tests diff --git a/src/virtualenv/activation/bash/activate.sh b/src/virtualenv/activation/bash/activate.sh index 04d5f5e05..d3cf34784 100644 --- a/src/virtualenv/activation/bash/activate.sh +++ b/src/virtualenv/activation/bash/activate.sh @@ -45,18 +45,18 @@ deactivate () { # unset irrelevant variables deactivate nondestructive -VIRTUAL_ENV='__VIRTUAL_ENV__' +VIRTUAL_ENV=__VIRTUAL_ENV__ if ([ "$OSTYPE" = "cygwin" ] || [ "$OSTYPE" = "msys" ]) && $(command -v cygpath &> /dev/null) ; then VIRTUAL_ENV=$(cygpath -u "$VIRTUAL_ENV") fi export VIRTUAL_ENV _OLD_VIRTUAL_PATH="$PATH" -PATH="$VIRTUAL_ENV/__BIN_NAME__:$PATH" +PATH="$VIRTUAL_ENV/"__BIN_NAME__":$PATH" export PATH -if [ "x__VIRTUAL_PROMPT__" != x ] ; then - VIRTUAL_ENV_PROMPT="__VIRTUAL_PROMPT__" +if [ "x"__VIRTUAL_PROMPT__ != x ] ; then + VIRTUAL_ENV_PROMPT=__VIRTUAL_PROMPT__ else VIRTUAL_ENV_PROMPT=$(basename "$VIRTUAL_ENV") fi diff --git a/src/virtualenv/activation/batch/__init__.py b/src/virtualenv/activation/batch/__init__.py index a6d58ebb4..3d74ba835 100644 --- a/src/virtualenv/activation/batch/__init__.py +++ b/src/virtualenv/activation/batch/__init__.py @@ -15,6 +15,10 @@ def templates(self): yield "deactivate.bat" yield "pydoc.bat" + @staticmethod + def quote(string): + return string + def instantiate_template(self, replacements, template, creator): # ensure the text has all newlines as \r\n - required by batch base = super().instantiate_template(replacements, template, creator) diff --git a/src/virtualenv/activation/cshell/activate.csh b/src/virtualenv/activation/cshell/activate.csh index f0c9cca96..24de5508b 100644 --- a/src/virtualenv/activation/cshell/activate.csh +++ b/src/virtualenv/activation/cshell/activate.csh @@ -10,15 +10,15 @@ alias deactivate 'test $?_OLD_VIRTUAL_PATH != 0 && setenv PATH "$_OLD_VIRTUAL_PA # Unset irrelevant variables. deactivate nondestructive -setenv VIRTUAL_ENV '__VIRTUAL_ENV__' +setenv VIRTUAL_ENV __VIRTUAL_ENV__ set _OLD_VIRTUAL_PATH="$PATH:q" -setenv PATH "$VIRTUAL_ENV:q/__BIN_NAME__:$PATH:q" +setenv PATH "$VIRTUAL_ENV:q/"__BIN_NAME__":$PATH:q" -if ('__VIRTUAL_PROMPT__' != "") then - setenv VIRTUAL_ENV_PROMPT '__VIRTUAL_PROMPT__' +if (__VIRTUAL_PROMPT__ != "") then + setenv VIRTUAL_ENV_PROMPT __VIRTUAL_PROMPT__ else setenv VIRTUAL_ENV_PROMPT "$VIRTUAL_ENV:t:q" endif diff --git a/src/virtualenv/activation/fish/activate.fish b/src/virtualenv/activation/fish/activate.fish index c453caf9e..f3cd1f2ab 100644 --- a/src/virtualenv/activation/fish/activate.fish +++ b/src/virtualenv/activation/fish/activate.fish @@ -58,7 +58,7 @@ end # Unset irrelevant variables. deactivate nondestructive -set -gx VIRTUAL_ENV '__VIRTUAL_ENV__' +set -gx VIRTUAL_ENV __VIRTUAL_ENV__ # https://github.com/fish-shell/fish-shell/issues/436 altered PATH handling if test (echo $FISH_VERSION | head -c 1) -lt 3 @@ -66,12 +66,12 @@ if test (echo $FISH_VERSION | head -c 1) -lt 3 else set -gx _OLD_VIRTUAL_PATH $PATH end -set -gx PATH "$VIRTUAL_ENV"'/__BIN_NAME__' $PATH +set -gx PATH "$VIRTUAL_ENV"'/'__BIN_NAME__ $PATH # Prompt override provided? # If not, just use the environment name. -if test -n '__VIRTUAL_PROMPT__' - set -gx VIRTUAL_ENV_PROMPT '__VIRTUAL_PROMPT__' +if test -n __VIRTUAL_PROMPT__ + set -gx VIRTUAL_ENV_PROMPT __VIRTUAL_PROMPT__ else set -gx VIRTUAL_ENV_PROMPT (basename "$VIRTUAL_ENV") end diff --git a/src/virtualenv/activation/nushell/__init__.py b/src/virtualenv/activation/nushell/__init__.py index 68cd4a3b4..ef7a79a9c 100644 --- a/src/virtualenv/activation/nushell/__init__.py +++ b/src/virtualenv/activation/nushell/__init__.py @@ -7,6 +7,25 @@ class NushellActivator(ViaTemplateActivator): def templates(self): yield "activate.nu" + @staticmethod + def quote(string): + """ + Nushell supports raw strings like: r###'this is a string'###. + + This method finds the maximum continuous sharps in the string and then + quote it with an extra sharp. + """ + max_sharps = 0 + current_sharps = 0 + for char in string: + if char == "#": + current_sharps += 1 + max_sharps = max(current_sharps, max_sharps) + else: + current_sharps = 0 + wrapping = "#" * (max_sharps + 1) + return f"r{wrapping}'{string}'{wrapping}" + def replacements(self, creator, dest_folder): # noqa: ARG002 return { "__VIRTUAL_PROMPT__": "" if self.flag_prompt is None else self.flag_prompt, diff --git a/src/virtualenv/activation/nushell/activate.nu b/src/virtualenv/activation/nushell/activate.nu index 482e7cb81..4da1c8c07 100644 --- a/src/virtualenv/activation/nushell/activate.nu +++ b/src/virtualenv/activation/nushell/activate.nu @@ -32,8 +32,8 @@ export-env { } } - let virtual_env = '__VIRTUAL_ENV__' - let bin = '__BIN_NAME__' + let virtual_env = __VIRTUAL_ENV__ + let bin = __BIN_NAME__ let is_windows = ($nu.os-info.family) == 'windows' let path_name = (if (has-env 'Path') { @@ -47,10 +47,10 @@ export-env { let new_path = ($env | get $path_name | prepend $venv_path) # If there is no default prompt, then use the env name instead - let virtual_env_prompt = (if ('__VIRTUAL_PROMPT__' | is-empty) { + let virtual_env_prompt = (if (__VIRTUAL_PROMPT__ | is-empty) { ($virtual_env | path basename) } else { - '__VIRTUAL_PROMPT__' + __VIRTUAL_PROMPT__ }) let new_env = { diff --git a/src/virtualenv/activation/powershell/__init__.py b/src/virtualenv/activation/powershell/__init__.py index 1f6d0f4e4..8489656cc 100644 --- a/src/virtualenv/activation/powershell/__init__.py +++ b/src/virtualenv/activation/powershell/__init__.py @@ -7,6 +7,18 @@ class PowerShellActivator(ViaTemplateActivator): def templates(self): yield "activate.ps1" + @staticmethod + def quote(string): + """ + This should satisfy PowerShell quoting rules [1], unless the quoted + string is passed directly to Windows native commands [2]. + + [1]: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules + [2]: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_parsing#passing-arguments-that-contain-quote-characters + """ # noqa: D205 + string = string.replace("'", "''") + return f"'{string}'" + __all__ = [ "PowerShellActivator", diff --git a/src/virtualenv/activation/powershell/activate.ps1 b/src/virtualenv/activation/powershell/activate.ps1 index 5ccfe120c..bd30e2eed 100644 --- a/src/virtualenv/activation/powershell/activate.ps1 +++ b/src/virtualenv/activation/powershell/activate.ps1 @@ -37,8 +37,8 @@ deactivate -nondestructive $VIRTUAL_ENV = $BASE_DIR $env:VIRTUAL_ENV = $VIRTUAL_ENV -if ("__VIRTUAL_PROMPT__" -ne "") { - $env:VIRTUAL_ENV_PROMPT = "__VIRTUAL_PROMPT__" +if (__VIRTUAL_PROMPT__ -ne "") { + $env:VIRTUAL_ENV_PROMPT = __VIRTUAL_PROMPT__ } else { $env:VIRTUAL_ENV_PROMPT = $( Split-Path $env:VIRTUAL_ENV -Leaf ) @@ -46,7 +46,7 @@ else { New-Variable -Scope global -Name _OLD_VIRTUAL_PATH -Value $env:PATH -$env:PATH = "$env:VIRTUAL_ENV/__BIN_NAME____PATH_SEP__" + $env:PATH +$env:PATH = "$env:VIRTUAL_ENV/" + __BIN_NAME__ + __PATH_SEP__ + $env:PATH if (!$env:VIRTUAL_ENV_DISABLE_PROMPT) { function global:_old_virtual_prompt { "" diff --git a/src/virtualenv/activation/python/__init__.py b/src/virtualenv/activation/python/__init__.py index 3126a39f2..e900f7ec9 100644 --- a/src/virtualenv/activation/python/__init__.py +++ b/src/virtualenv/activation/python/__init__.py @@ -10,10 +10,14 @@ class PythonActivator(ViaTemplateActivator): def templates(self): yield "activate_this.py" + @staticmethod + def quote(string): + return repr(string) + def replacements(self, creator, dest_folder): replacements = super().replacements(creator, dest_folder) lib_folders = OrderedDict((os.path.relpath(str(i), str(dest_folder)), None) for i in creator.libs) - lib_folders = os.pathsep.join(lib_folders.keys()).replace("\\", "\\\\") # escape Windows path characters + lib_folders = os.pathsep.join(lib_folders.keys()) replacements.update( { "__LIB_FOLDERS__": lib_folders, diff --git a/src/virtualenv/activation/python/activate_this.py b/src/virtualenv/activation/python/activate_this.py index 388e00153..9cc816fab 100644 --- a/src/virtualenv/activation/python/activate_this.py +++ b/src/virtualenv/activation/python/activate_this.py @@ -20,18 +20,18 @@ raise AssertionError(msg) from exc bin_dir = os.path.dirname(abs_file) -base = bin_dir[: -len("__BIN_NAME__") - 1] # strip away the bin part from the __file__, plus the path separator +base = bin_dir[: -len(__BIN_NAME__) - 1] # strip away the bin part from the __file__, plus the path separator # prepend bin to PATH (this file is inside the bin directory) os.environ["PATH"] = os.pathsep.join([bin_dir, *os.environ.get("PATH", "").split(os.pathsep)]) os.environ["VIRTUAL_ENV"] = base # virtual env is right above bin directory -os.environ["VIRTUAL_ENV_PROMPT"] = "__VIRTUAL_PROMPT__" or os.path.basename(base) # noqa: SIM222 +os.environ["VIRTUAL_ENV_PROMPT"] = __VIRTUAL_PROMPT__ or os.path.basename(base) # add the virtual environments libraries to the host python import mechanism prev_length = len(sys.path) -for lib in "__LIB_FOLDERS__".split(os.pathsep): +for lib in __LIB_FOLDERS__.split(os.pathsep): path = os.path.realpath(os.path.join(bin_dir, lib)) - site.addsitedir(path.decode("utf-8") if "__DECODE_PATH__" else path) + site.addsitedir(path.decode("utf-8") if __DECODE_PATH__ else path) sys.path[:] = sys.path[prev_length:] + sys.path[0:prev_length] sys.real_prefix = sys.prefix diff --git a/src/virtualenv/activation/via_template.py b/src/virtualenv/activation/via_template.py index ab9b90131..6fa4474d2 100644 --- a/src/virtualenv/activation/via_template.py +++ b/src/virtualenv/activation/via_template.py @@ -1,6 +1,7 @@ from __future__ import annotations import os +import shlex import sys from abc import ABC, abstractmethod @@ -21,6 +22,16 @@ class ViaTemplateActivator(Activator, ABC): def templates(self): raise NotImplementedError + @staticmethod + def quote(string): + """ + Quote strings in the activation script. + + :param string: the string to quote + :return: quoted string that works in the activation script + """ + return shlex.quote(string) + def generate(self, creator): dest_folder = creator.bin_dir replacements = self.replacements(creator, dest_folder) @@ -63,7 +74,7 @@ def instantiate_template(self, replacements, template, creator): text = binary.decode("utf-8", errors="strict") for key, value in replacements.items(): value_uni = self._repr_unicode(creator, value) - text = text.replace(key, value_uni) + text = text.replace(key, self.quote(value_uni)) return text @staticmethod diff --git a/tests/conftest.py b/tests/conftest.py index 5995d228f..0310b5525 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -275,7 +275,11 @@ def is_inside_ci(): @pytest.fixture(scope="session") def special_char_name(): - base = "e-$ Γ¨Ρ€Ρ‚πŸš’β™žδΈ­η‰‡-j" + base = "'\";&&e-$ Γ¨Ρ€Ρ‚πŸš’β™žδΈ­η‰‡-j" + if IS_WIN: + # get rid of invalid characters on Windows + base = base.replace('"', "") + base = base.replace(";", "") # workaround for pypy3 https://bitbucket.org/pypy/pypy/issues/3147/venv-non-ascii-support-windows encoding = "ascii" if IS_WIN else sys.getfilesystemencoding() # let's not include characters that the file system cannot encode) diff --git a/tests/unit/activation/conftest.py b/tests/unit/activation/conftest.py index 26a601297..e320038ea 100644 --- a/tests/unit/activation/conftest.py +++ b/tests/unit/activation/conftest.py @@ -6,7 +6,6 @@ import sys from os.path import dirname, normcase from pathlib import Path -from shlex import quote from subprocess import Popen import pytest @@ -154,7 +153,7 @@ def assert_output(self, out, raw, tmp_path): assert out[-1] == "None", raw def quote(self, s): - return quote(s) + return self.of_class.quote(s) def python_cmd(self, cmd): return f"{os.path.basename(sys.executable)} -c {self.quote(cmd)}" diff --git a/tests/unit/activation/test_batch.py b/tests/unit/activation/test_batch.py index ddcb251b4..1d22767c3 100644 --- a/tests/unit/activation/test_batch.py +++ b/tests/unit/activation/test_batch.py @@ -1,7 +1,5 @@ from __future__ import annotations -from shlex import quote - import pytest from virtualenv.activation import BatchActivator @@ -26,10 +24,12 @@ def _get_test_lines(self, activate_script): return ["@echo off", *super()._get_test_lines(activate_script)] def quote(self, s): - """double quotes needs to be single, and single need to be double""" - return "".join(("'" if c == '"' else ('"' if c == "'" else c)) for c in quote(s)) + if '"' in s or " " in s: + text = s.replace('"', r"\"") + return f'"{text}"' + return s def print_prompt(self): - return "echo %PROMPT%" + return 'echo "%PROMPT%"' activation_tester(Batch) diff --git a/tests/unit/activation/test_powershell.py b/tests/unit/activation/test_powershell.py index c81a4cca4..dab5748d7 100644 --- a/tests/unit/activation/test_powershell.py +++ b/tests/unit/activation/test_powershell.py @@ -1,7 +1,6 @@ from __future__ import annotations import sys -from shlex import quote import pytest @@ -21,11 +20,6 @@ def __init__(self, session) -> None: self.activate_cmd = "." self.script_encoding = "utf-8-sig" - def quote(self, s): - """powershell double quote needed for quotes within single quotes""" - text = quote(s) - return text.replace('"', '""') if sys.platform == "win32" else text - def _get_test_lines(self, activate_script): return super()._get_test_lines(activate_script) @@ -35,4 +29,19 @@ def invoke_script(self): def print_prompt(self): return "prompt" + def quote(self, s): + """ + Tester will pass strings to native commands on Windows so extra + parsing rules are used. Check `PowerShellActivator.quote` for more + details. + """ + text = PowerShellActivator.quote(s) + return text.replace('"', '""') if sys.platform == "win32" else text + + def activate_call(self, script): + # Commands are called without quotes in PowerShell + cmd = self.activate_cmd + scr = self.quote(str(script)) + return f"{cmd} {scr}".strip() + activation_tester(PowerShell)