Fix #2768: Quote template strings in activation scripts (#2771)

y5c4l3 · web-flow · commit 86dddeda7c99 · 2024-09-27T09:16:08.000-07:00
diff --git a/docs/changelog/2768.bugfix.rst 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/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
@@ -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
@@ -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
@@ -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
@@ -58,20 +58,20 @@ 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
     set -gx _OLD_VIRTUAL_PATH (_bashify_path $PATH)
 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
@@ -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
@@ -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
@@ -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
@@ -37,16 +37,16 @@ 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 )
 }
 
 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+Properly quote string placeholders in activation script templates to mitigate`
	`2`	+potential command injection - by :user:`y5c4l3`.