CVE-2024-22190 Hand Cherry-Pick gitpython-developers/GitPython/commit…

git/cmd.py

@@ -35,7 +35,7 @@
     is_win,
 )
 from git.exc import CommandError, UnsafeOptionError, UnsafeProtocolError
-from git.util import is_cygwin_git, cygpath, expand_path
+from git.util import is_cygwin_git, cygpath, expand_path, patch_env
 
 from .exc import (
     GitCommandError,
@@ -132,7 +132,7 @@ def pump_stream(cmdline, name, stream, is_decode, handler):
         return finalizer(process)
 
 
-def _safer_popen_windows(command, shell, env=None, **kwargs):
+def _safer_popen_windows(command, shell=False, env=None, **kwargs):
     """Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
     This avoids an untrusted search path condition where a file like ``git.exe`` in a
     malicious repository would be run when GitPython operates on the repository. The
@@ -154,15 +154,13 @@ def _safer_popen_windows(command, shell, env=None, **kwargs):
     # https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
     # https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
     creationflags = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
-
     # When using a shell, the shell is the direct subprocess, so the variable must be
     # set in its environment, to affect its search behavior. (The "1" can be any value.)
     if shell:
         safer_env = {} if env is None else dict(env)
         safer_env["NoDefaultCurrentDirectoryInExePath"] = "1"
     else:
         safer_env = env
-
     # When not using a shell, the current process does the search in a CreateProcessW
     # API call, so the variable must be set in our environment. With a shell, this is
     # unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
@@ -185,6 +183,12 @@ def _safer_popen_windows(command, shell, env=None, **kwargs):
     safer_popen = Popen
 
 
+if os.name == "nt":
+    safer_popen = _safer_popen_windows
+else:
+    safer_popen = Popen
+
+
 def dashify(string):
     return string.replace('_', '-')
 
@@ -801,15 +805,11 @@ def execute(self, command,
             cmd_not_found_exception = OSError
             if kill_after_timeout:
                 raise GitCommandError(command, '"kill_after_timeout" feature is not supported on Windows.')
-
-            # Only search PATH, not CWD. This must be in the *caller* environment. The "1" can be any value.
-            patch_caller_env = unittest.mock.patch.dict(os.environ, {"NoDefaultCurrentDirectoryInExePath": "1"})
         else:
             if sys.version_info[0] > 2:
                 cmd_not_found_exception = FileNotFoundError  # NOQA # exists, flake8 unknown @UndefinedVariable
             else:
                 cmd_not_found_exception = OSError
-            patch_caller_env = nullcontext()
         # end handle
 
         stdout_sink = (PIPE
@@ -829,7 +829,7 @@ def execute(self, command,
                 stdin=istream,
                 stderr=PIPE,
                 stdout=stdout_sink,
-                shell=shell is not None and shell or self.USE_SHELL,
+                shell=shell,
                 universal_newlines=universal_newlines,
                 **subprocess_kwargs
             )

git/index/fun.py

@@ -74,11 +74,11 @@ def run_commit_hook(name, index, *args):
     env["GIT_EDITOR"] = ":"
     try:
         cmd = safer_popen([hp] + list(args),
-                               env=env,
-                               stdout=subprocess.PIPE,
-                               stderr=subprocess.PIPE,
-                               cwd=index.repo.working_dir,
-                               close_fds=is_posix)
+                        env=env,
+                        stdout=subprocess.PIPE,
+                        stderr=subprocess.PIPE,
+                        cwd=index.repo.working_dir,
+                        close_fds=is_posix)
     except Exception as ex:
         raise HookExecutionError(hp, ex)
     else:

git/test/fixtures/polyglot

@@ -0,0 +1,8 @@
+#!/usr/bin/env sh
+# Valid script in both Bash and Python, but with different behavior.
+""":"
+echo 'Ran intended hook.' >output.txt
+exit
+" """
+from pathlib import Path
+Path('payload.txt').write_text('Ran impostor hook!', encoding='utf-8')

git/test/test_git.py

@@ -6,7 +6,8 @@
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 from __future__ import print_function
 
-import contextlib
+import contextlib2 as contextlib
+import ddt
 import os
 import shutil
 import subprocess
@@ -31,10 +32,11 @@
     assert_equal,
     assert_true,
     assert_match,
-    fixture_path
+    fixture_path,
+    with_rw_directory,
 )
-from git.test.lib import with_rw_directory
-from git.util import finalize_process
+
+from git.util import finalize_process, cwd
 
 import os.path as osp
 
@@ -51,6 +53,7 @@
 
 from git.compat import is_win
 
+
 @contextlib.contextmanager
 def _chdir(new_dir):
     """Context manager to temporarily change directory. Not reentrant."""
@@ -77,7 +80,7 @@ def _patch_out_env(name):
             os.environ[name] = old_value
 
 
-
+@ddt.ddt
 class TestGit(TestBase):
 
     @classmethod
@@ -137,23 +140,46 @@ def test_it_transforms_kwargs_into_git_command_arguments(self):
     def test_it_executes_git_to_shell_and_returns_result(self):
         assert_match(r'^git version [\d\.]{2}.*$', self.git.execute(["git", "version"]))
 
-    def test_it_executes_git_not_from_cwd(self):
-        with TemporaryDirectory() as tmpdir:
-            if is_win:
-                # Copy an actual binary executable that is not git.
-                other_exe_path = os.path.join(os.getenv("WINDIR"), "system32", "hostname.exe")
-                impostor_path = os.path.join(tmpdir, "git.exe")
-                shutil.copy(other_exe_path, impostor_path)
-            else:
-                # Create a shell script that doesn't do anything.
-                impostor_path = os.path.join(tmpdir, "git")
-                with open(impostor_path, mode="w") as file:
-                    print("#!/bin/sh", file=file)
-                os.chmod(impostor_path, 0o755)
-
-            with _chdir(tmpdir):
-                # six.assertRegex(self.git.execute(["git", "version"]).encode("UTF-8"), r"^git version\b")
-                self.assertRegexpMatches(self.git.execute(["git", "version"]), r"^git version\b")
+    @ddt.data(
+        # chdir_to_repo, shell, command, use_shell_impostor
+        (False, False, ["git", "version"], False),
+        (False, True, "git version", False),
+        (False, True, "git version", True),
+        (True, False, ["git", "version"], False),
+        (True, True, "git version", False),
+        (True, True, "git version", True),
+    )
+    def test_it_executes_git_not_from_cwd(self, case):
+        self.internal_it_executes_git_not_from_cwd(case)
+
+    @with_rw_directory
+    def internal_it_executes_git_not_from_cwd(self, rw_dir, case):
+        chdir_to_repo, shell, command, use_shell_impostor = case
+        repo = Repo.init(rw_dir)
+        if os.name == "nt":
+            # Copy an actual binary executable that is not git. (On Windows, running
+            # "hostname" only displays the hostname, it never tries to change it.)
+            other_exe_path = Path(os.environ["SystemRoot"], "system32", "hostname.exe")
+            impostor_path = Path(rw_dir, "git.exe")
+            shutil.copy(other_exe_path, str(impostor_path))
+        else:
+            # Create a shell script that doesn't do anything.
+            impostor_path = Path(rw_dir, "git")
+            impostor_path.write_text(u"#!/bin/sh\n", encoding="utf-8")
+            os.chmod(str(impostor_path), 0o755)
+        if use_shell_impostor:
+            shell_name = "cmd.exe" if os.name == "nt" else "sh"
+            shutil.copy(str(impostor_path), str(Path(rw_dir, shell_name)))
+        with contextlib.ExitStack() as stack:
+            if chdir_to_repo:
+                stack.enter_context(cwd(rw_dir))
+            if use_shell_impostor:
+                stack.enter_context(_patch_out_env("ComSpec"))
+            # Run the command without raising an exception on failure, as the exception
+            # message is currently misleading when the command is a string rather than a
+            # sequence of strings (it really runs "git", but then wrongly reports "g").
+            output = repo.git.execute(command, with_exceptions=False, shell=shell)
+        self.assertRegexpMatches(output, r"^git version\b")
 
     def test_it_accepts_stdin(self):
         filename = fixture_path("cat_file_blob")
@@ -325,7 +351,7 @@ def test_environment(self, rw_dir):
                     self.assertIn('FOO', str(err))
 
     def test_handle_process_output(self):
-        from git.cmd import handle_process_output
+        from git.cmd import handle_process_output, safer_popen
 
         line_count = 5002
         count = [None, 0, 0]
@@ -337,13 +363,11 @@ def counter_stderr(line):
             count[2] += 1
 
         cmdline = [sys.executable, fixture_path('cat_file.py'), str(fixture_path('issue-301_stderr'))]
-        proc = subprocess.Popen(cmdline,
-                                stdin=None,
-                                stdout=subprocess.PIPE,
-                                stderr=subprocess.PIPE,
-                                shell=False,
-                                # creationflags=cmd.PROC_CREATIONFLAGS,
-                                )
+        proc = safer_popen(cmdline,
+                           stdin=None,
+                           stdout=subprocess.PIPE,
+                           stderr=subprocess.PIPE,
+                           shell=False)
 
         handle_process_output(proc, counter_stdout, counter_stderr, finalize_process)