From 82dbaffa36fe317e331b2d22c5efbb6a0b6c7b19 Mon Sep 17 00:00:00 2001 From: Lumir Balhar Date: Tue, 8 Oct 2024 09:15:09 +0200 Subject: [PATCH] Prevent command injection --- docs/changelog/2768.bugfix.rst | 2 ++ src/virtualenv/activation/bash/activate.sh | 9 +++++---- src/virtualenv/activation/batch/__init__.py | 4 ++++ src/virtualenv/activation/cshell/activate.csh | 10 +++++----- 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 | 6 +++--- src/virtualenv/activation/via_template.py | 15 ++++++++++++-- tests/conftest.py | 6 +++++- tests/unit/activation/conftest.py | 3 +-- tests/unit/activation/test_batch.py | 10 +++++----- tests/unit/activation/test_powershell.py | 20 ++++++++++++++----- 16 files changed, 105 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 0000000..7651eb6 --- /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`. diff --git a/src/virtualenv/activation/bash/activate.sh b/src/virtualenv/activation/bash/activate.sh index fb40db6..d047ea4 100644 --- a/src/virtualenv/activation/bash/activate.sh +++ b/src/virtualenv/activation/bash/activate.sh @@ -44,14 +44,14 @@ 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 # unset PYTHONHOME if set @@ -62,8 +62,9 @@ fi if [ -z "${VIRTUAL_ENV_DISABLE_PROMPT-}" ] ; then _OLD_VIRTUAL_PS1="${PS1-}" - if [ "x__VIRTUAL_PROMPT__" != x ] ; then - PS1="(__VIRTUAL_PROMPT__) ${PS1-}" + if [ "x"__VIRTUAL_PROMPT__ != x ] ; then + PROMPT=__VIRTUAL_PROMPT__ + PS1="(${PROMPT}) ${PS1-}" else PS1="(`basename \"$VIRTUAL_ENV\"`) ${PS1-}" fi diff --git a/src/virtualenv/activation/batch/__init__.py b/src/virtualenv/activation/batch/__init__.py index 13ba097..b3e8540 100644 --- a/src/virtualenv/activation/batch/__init__.py +++ b/src/virtualenv/activation/batch/__init__.py @@ -13,6 +13,10 @@ class BatchActivator(ViaTemplateActivator): 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 837dcda..fcec426 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 - set env_name = '(__VIRTUAL_PROMPT__) ' +if (__VIRTUAL_PROMPT__ != "") then + set env_name = __VIRTUAL_PROMPT__ else set env_name = '('"$VIRTUAL_ENV:t:q"') ' endif @@ -42,7 +42,7 @@ if ( $do_prompt == "1" ) then if ( "$prompt:q" =~ *"$newline:q"* ) then : else - set prompt = "$env_name:q$prompt:q" + set prompt = '('"$env_name:q"') '"$prompt:q" endif endif endif diff --git a/src/virtualenv/activation/fish/activate.fish b/src/virtualenv/activation/fish/activate.fish index 62f631e..3637d55 100644 --- a/src/virtualenv/activation/fish/activate.fish +++ b/src/virtualenv/activation/fish/activate.fish @@ -57,7 +57,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 @@ -65,7 +65,7 @@ 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 # Unset `$PYTHONHOME` if set. if set -q PYTHONHOME @@ -87,8 +87,8 @@ if test -z "$VIRTUAL_ENV_DISABLE_PROMPT" # Prompt override provided? # If not, just prepend the environment name. - if test -n '__VIRTUAL_PROMPT__' - printf '(%s) ' '__VIRTUAL_PROMPT__' + if test -n __VIRTUAL_PROMPT__ + printf '(%s) ' __VIRTUAL_PROMPT__ else printf '(%s) ' (basename "$VIRTUAL_ENV") end diff --git a/src/virtualenv/activation/nushell/__init__.py b/src/virtualenv/activation/nushell/__init__.py index 4e2ea77..c96af22 100644 --- a/src/virtualenv/activation/nushell/__init__.py +++ b/src/virtualenv/activation/nushell/__init__.py @@ -5,6 +5,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: U100 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 3da1519..569a8a1 100644 --- a/src/virtualenv/activation/nushell/activate.nu +++ b/src/virtualenv/activation/nushell/activate.nu @@ -31,8 +31,8 @@ export-env { } let is_windows = ($nu.os-info.name | str downcase) == 'windows' - let virtual_env = '__VIRTUAL_ENV__' - let bin = '__BIN_NAME__' + let virtual_env = __VIRTUAL_ENV__ + let bin = __BIN_NAME__ let path_sep = (char esep) let path_name = (if $is_windows { if (has-env 'Path') { @@ -73,10 +73,10 @@ export-env { $new_env } else { # Creating the new prompt for the session - let virtual_prompt = (if ('__VIRTUAL_PROMPT__' == '') { + let virtual_prompt = (if (__VIRTUAL_PROMPT__ == '') { $'(char lparen)($virtual_env | path basename)(char rparen) ' } else { - '(__VIRTUAL_PROMPT__) ' + (__VIRTUAL_PROMPT__) }) # Back up the old prompt builder diff --git a/src/virtualenv/activation/powershell/__init__.py b/src/virtualenv/activation/powershell/__init__.py index 4e74ecb..773d690 100644 --- a/src/virtualenv/activation/powershell/__init__.py +++ b/src/virtualenv/activation/powershell/__init__.py @@ -5,6 +5,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 d524347..346980d 100644 --- a/src/virtualenv/activation/powershell/activate.ps1 +++ b/src/virtualenv/activation/powershell/activate.ps1 @@ -35,18 +35,18 @@ $env:VIRTUAL_ENV = $VIRTUAL_ENV 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 { "" } $function:_old_virtual_prompt = $function:prompt - if ("__VIRTUAL_PROMPT__" -ne "") { + if (__VIRTUAL_PROMPT__ -ne "") { function global:prompt { # Add the custom prefix to the existing prompt $previous_prompt_value = & $function:_old_virtual_prompt - ("(__VIRTUAL_PROMPT__) " + $previous_prompt_value) + ((__VIRTUAL_PROMPT__) + $previous_prompt_value) } } else { diff --git a/src/virtualenv/activation/python/__init__.py b/src/virtualenv/activation/python/__init__.py index a49444b..8f0971e 100644 --- a/src/virtualenv/activation/python/__init__.py +++ b/src/virtualenv/activation/python/__init__.py @@ -9,10 +9,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()) win_py2 = creator.interpreter.platform == "win32" and creator.interpreter.version_info.major == 2 replacements.update( { diff --git a/src/virtualenv/activation/python/activate_this.py b/src/virtualenv/activation/python/activate_this.py index e8eeb84..976952e 100644 --- a/src/virtualenv/activation/python/activate_this.py +++ b/src/virtualenv/activation/python/activate_this.py @@ -14,7 +14,7 @@ except NameError: raise AssertionError("You must use exec(open(this_file).read(), {'__file__': this_file}))") 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)) @@ -22,9 +22,9 @@ os.environ["VIRTUAL_ENV"] = base # virtual env is right above bin directory # 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 cc9dbda..b1dfa6b 100644 --- a/src/virtualenv/activation/via_template.py +++ b/src/virtualenv/activation/via_template.py @@ -1,4 +1,5 @@ import os +import shlex import sys from abc import ABCMeta, abstractmethod @@ -19,6 +20,16 @@ class ViaTemplateActivator(Activator, metaclass=ABCMeta): 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) @@ -58,8 +69,8 @@ class ViaTemplateActivator(Activator, metaclass=ABCMeta): binary = read_binary(self.__module__, template) text = binary.decode("utf-8", errors="strict") for key, value in replacements.items(): - value = self._repr_unicode(creator, value) - text = text.replace(key, value) + value_uni = self._repr_unicode(creator, value) + text = text.replace(key, self.quote(value_uni)) return text @staticmethod diff --git a/tests/conftest.py b/tests/conftest.py index a7ec4e0..b2fae66 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -292,7 +292,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 e24a4fc..2d8b3ce 100644 --- a/tests/unit/activation/conftest.py +++ b/tests/unit/activation/conftest.py @@ -4,7 +4,6 @@ import subprocess import sys from os.path import dirname, normcase from pathlib import Path -from shlex import quote from subprocess import Popen import pytest @@ -146,7 +145,7 @@ class ActivationTester: 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 9e6d6d6..e95f7ea 100644 --- a/tests/unit/activation/test_batch.py +++ b/tests/unit/activation/test_batch.py @@ -1,5 +1,3 @@ -from shlex import quote - import pytest from virtualenv.activation import BatchActivator @@ -25,10 +23,12 @@ def test_batch(activation_tester_class, activation_tester, tmp_path): return ["@echo off", "", "chcp 65001 1>NUL"] + 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 761237f..f495353 100644 --- a/tests/unit/activation/test_powershell.py +++ b/tests/unit/activation/test_powershell.py @@ -1,5 +1,4 @@ import sys -from shlex import quote import pytest @@ -19,10 +18,6 @@ def test_powershell(activation_tester_class, activation_tester, monkeypatch): self.activate_cmd = "." self.script_encoding = "utf-16" - def quote(self, s): - """powershell double quote needed for quotes within single quotes""" - return quote(s).replace('"', '""') - def _get_test_lines(self, activate_script): # for BATCH utf-8 support need change the character code page to 650001 return super()._get_test_lines(activate_script) @@ -33,4 +28,19 @@ def test_powershell(activation_tester_class, activation_tester, monkeypatch): 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) -- 2.46.2