Merge branch 'main' into fix-bash-exe

Author: emanspeaks

Diff for: .pre-commit-config.yaml

@@ -29,7 +29,7 @@ repos:
   hooks:
   - id: shellcheck
     args: [--color]
-    exclude: ^git/ext/
+    exclude: ^test/fixtures/polyglot$|^git/ext/
 
 - repo: https://github.com/pre-commit/pre-commit-hooks
   rev: v4.4.0

Diff for: .readthedocs.yaml

@@ -0,0 +1,35 @@
+# Read the Docs configuration file for Sphinx projects
+# See https://docs.readthedocs.io/en/stable/config-file/v2.html for details
+
+# Required
+version: 2
+
+# Set the OS, Python version and other tools you might need
+build:
+  os: ubuntu-22.04
+  tools:
+    python: "3.12"
+    # You can also specify other tool versions:
+    # nodejs: "20"
+    # rust: "1.70"
+    # golang: "1.20"
+
+# Build documentation in the "docs/" directory with Sphinx
+sphinx:
+  configuration: doc/source/conf.py
+  # You can configure Sphinx to use a different builder, for instance use the dirhtml builder for simpler URLs
+  # builder: "dirhtml"
+  # Fail on all warnings to avoid broken references
+  # fail_on_warning: true
+
+# Optionally build your docs in additional formats such as PDF and ePub
+# formats:
+#   - pdf
+#   - epub
+
+# Optional but recommended, declare the Python requirements required
+# to build your documentation
+# See https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html
+# python:
+#   install:
+#     - requirements: docs/requirements.txt

Diff for: AUTHORS

@@ -53,6 +53,7 @@ Contributors are:
 -Santos Gallegos <stsewd _at_ proton.me>
 -Wenhan Zhu <wzhu.cosmos _at_ gmail.com>
 -Eliah Kagan <eliah.kagan _at_ gmail.com>
+-Ethan Lin <et.repositories _at_ gmail.com>
 -Randy Eckman <emanspeaks _at_ gmail.com>
 
 Portions derived from other open source works and are clearly marked.

Diff for: VERSION

@@ -1 +1 @@
-3.1.40
+3.1.41

Diff for: doc/requirements.txt

@@ -1,3 +1,8 @@
-sphinx==4.3.0
+sphinx == 4.3.2
 sphinx_rtd_theme
+sphinxcontrib-applehelp >= 1.0.2, <= 1.0.4
+sphinxcontrib-devhelp == 1.0.2
+sphinxcontrib-htmlhelp >= 2.0.0, <= 2.0.1
+sphinxcontrib-qthelp == 1.0.3
+sphinxcontrib-serializinghtml == 1.1.5
 sphinx-autodoc-typehints

Diff for: doc/source/changes.rst

@@ -2,6 +2,18 @@
 Changelog
 =========
 
+3.1.41
+======
+
+This release is relevant for security as it fixes a possible arbitary
+code execution on Windows.
+
+See this PR for details: https://github.com/gitpython-developers/GitPython/pull/1792
+An advisory is available soon at: https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-2mqj-m65w-jghx
+
+See the following for all changes.
+https://github.com/gitpython-developers/GitPython/releases/tag/3.1.41
+
 3.1.40
 ======
 

Diff for: doc/source/conf.py

@@ -93,7 +93,7 @@
 # Options for HTML output
 # -----------------------
 
-html_theme = "sphinx_rtd_theme"
+# html_theme = "sphinx_rtd_theme"
 html_theme_options = {}
 
 # The name for this set of Sphinx documents.  If None, it defaults to

Diff for: git/cmd.py

@@ -47,6 +47,7 @@
     Iterator,
     List,
     Mapping,
+    Optional,
     Sequence,
     TYPE_CHECKING,
     TextIO,
@@ -103,7 +104,7 @@ def handle_process_output(
         Callable[[bytes, "Repo", "DiffIndex"], None],
     ],
     stderr_handler: Union[None, Callable[[AnyStr], None], Callable[[List[AnyStr]], None]],
-    finalizer: Union[None, Callable[[Union[subprocess.Popen, "Git.AutoInterrupt"]], None]] = None,
+    finalizer: Union[None, Callable[[Union[Popen, "Git.AutoInterrupt"]], None]] = None,
     decode_streams: bool = True,
     kill_after_timeout: Union[None, float] = None,
 ) -> None:
@@ -208,6 +209,68 @@ def pump_stream(
         finalizer(process)
 
 
+def _safer_popen_windows(
+    command: Union[str, Sequence[Any]],
+    *,
+    shell: bool = False,
+    env: Optional[Mapping[str, str]] = None,
+    **kwargs: Any,
+) -> Popen:
+    """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
+    process using GitPython may have an untrusted repository's working tree as its
+    current working directory. Some operations may temporarily change to that directory
+    before running a subprocess. In addition, while by default GitPython does not run
+    external commands with a shell, it can be made to do so, in which case the CWD of
+    the subprocess, which GitPython usually sets to a repository working tree, can
+    itself be searched automatically by the shell. This wrapper covers all those cases.
+
+    :note: This currently works by setting the ``NoDefaultCurrentDirectoryInExePath``
+        environment variable during subprocess creation. It also takes care of passing
+        Windows-specific process creation flags, but that is unrelated to path search.
+
+    :note: The current implementation contains a race condition on :attr:`os.environ`.
+        GitPython isn't thread-safe, but a program using it on one thread should ideally
+        be able to mutate :attr:`os.environ` on another, without unpredictable results.
+        See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
+    """
+    # CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
+    # 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
+    # patched. If not, in the rare case the ComSpec environment variable is unset, the
+    # shell is searched for unsafely. Setting NoDefaultCurrentDirectoryInExePath in all
+    # cases, as here, is simpler and protects against that. (The "1" can be any value.)
+    with patch_env("NoDefaultCurrentDirectoryInExePath", "1"):
+        return Popen(
+            command,
+            shell=shell,
+            env=safer_env,
+            creationflags=creationflags,
+            **kwargs,
+        )
+
+
+if os.name == "nt":
+    safer_popen = _safer_popen_windows
+else:
+    safer_popen = Popen
+
+
 def dashify(string: str) -> str:
     return string.replace("_", "-")
 
@@ -226,14 +289,6 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc
 ## -- End Utilities -- @}
 
 
-if os.name == "nt":
-    # CREATE_NEW_PROCESS_GROUP is needed to allow killing it afterwards. See:
-    # https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
-    PROC_CREATIONFLAGS = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
-else:
-    PROC_CREATIONFLAGS = 0
-
-
 class Git(LazyMixin):
     """The Git class manages communication with the Git binary.
 
@@ -1160,11 +1215,8 @@ def execute(
                     redacted_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.
-            maybe_patch_caller_env = patch_env("NoDefaultCurrentDirectoryInExePath", "1")
         else:
             cmd_not_found_exception = FileNotFoundError
-            maybe_patch_caller_env = contextlib.nullcontext()
         # END handle
 
         stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
@@ -1179,20 +1231,18 @@ def execute(
             universal_newlines,
         )
         try:
-            with maybe_patch_caller_env:
-                proc = Popen(
-                    command,
-                    env=env,
-                    cwd=cwd,
-                    bufsize=-1,
-                    stdin=(istream or DEVNULL),
-                    stderr=PIPE,
-                    stdout=stdout_sink,
-                    shell=shell,
-                    universal_newlines=universal_newlines,
-                    creationflags=PROC_CREATIONFLAGS,
-                    **subprocess_kwargs,
-                )
+            proc = safer_popen(
+                command,
+                env=env,
+                cwd=cwd,
+                bufsize=-1,
+                stdin=(istream or DEVNULL),
+                stderr=PIPE,
+                stdout=stdout_sink,
+                shell=shell,
+                universal_newlines=universal_newlines,
+                **subprocess_kwargs,
+            )
         except cmd_not_found_exception as err:
             raise GitCommandNotFound(redacted_command, err) from err
         else:

Diff for: git/index/fun.py

@@ -18,7 +18,7 @@
 )
 import subprocess
 
-from git.cmd import PROC_CREATIONFLAGS, handle_process_output, Git
+from git.cmd import handle_process_output, Git, safer_popen
 from git.compat import defenc, force_bytes, force_text, safe_decode
 from git.exc import HookExecutionError, UnmergedEntriesError
 from git.objects.fun import (
@@ -98,13 +98,12 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
             relative_hp = Path(hp).relative_to(index.repo.working_dir).as_posix()
             cmd = [Git.GIT_PYTHON_BASH_EXECUTABLE, relative_hp]
 
-        process = subprocess.Popen(
+        process = safer_popen(
             cmd + list(args),
             env=env,
             stdout=subprocess.PIPE,
             stderr=subprocess.PIPE,
             cwd=index.repo.working_dir,
-            creationflags=PROC_CREATIONFLAGS,
         )
     except Exception as ex:
         raise HookExecutionError(hp, ex) from ex

Diff for: git/objects/tree.py

@@ -53,54 +53,6 @@
 __all__ = ("TreeModifier", "Tree")
 
 
-def git_cmp(t1: TreeCacheTup, t2: TreeCacheTup) -> int:
-    a, b = t1[2], t2[2]
-    # assert isinstance(a, str) and isinstance(b, str)
-    len_a, len_b = len(a), len(b)
-    min_len = min(len_a, len_b)
-    min_cmp = cmp(a[:min_len], b[:min_len])
-
-    if min_cmp:
-        return min_cmp
-
-    return len_a - len_b
-
-
-def merge_sort(a: List[TreeCacheTup], cmp: Callable[[TreeCacheTup, TreeCacheTup], int]) -> None:
-    if len(a) < 2:
-        return
-
-    mid = len(a) // 2
-    lefthalf = a[:mid]
-    righthalf = a[mid:]
-
-    merge_sort(lefthalf, cmp)
-    merge_sort(righthalf, cmp)
-
-    i = 0
-    j = 0
-    k = 0
-
-    while i < len(lefthalf) and j < len(righthalf):
-        if cmp(lefthalf[i], righthalf[j]) <= 0:
-            a[k] = lefthalf[i]
-            i = i + 1
-        else:
-            a[k] = righthalf[j]
-            j = j + 1
-        k = k + 1
-
-    while i < len(lefthalf):
-        a[k] = lefthalf[i]
-        i = i + 1
-        k = k + 1
-
-    while j < len(righthalf):
-        a[k] = righthalf[j]
-        j = j + 1
-        k = k + 1
-
-
 class TreeModifier:
     """A utility class providing methods to alter the underlying cache in a list-like fashion.
 
@@ -131,7 +83,7 @@ def set_done(self) -> "TreeModifier":
 
         :return self:
         """
-        merge_sort(self._cache, git_cmp)
+        self._cache.sort(key=lambda x: (x[2] + "/") if x[1] == Tree.tree_id << 12 else x[2])
         return self
 
     # } END interface

Diff for: git/util.py

@@ -327,6 +327,17 @@ def _get_exe_extensions() -> Sequence[str]:
 
 
 def py_where(program: str, path: Optional[PathLike] = None) -> List[str]:
+    """Perform a path search to assist :func:`is_cygwin_git`.
+
+    This is not robust for general use. It is an implementation detail of
+    :func:`is_cygwin_git`. When a search following all shell rules is needed,
+    :func:`shutil.which` can be used instead.
+
+    :note: Neither this function nor :func:`shutil.which` will predict the effect of an
+        executable search on a native Windows system due to a :class:`subprocess.Popen`
+        call without ``shell=True``, because shell and non-shell executable search on
+        Windows differ considerably.
+    """
     # From: http://stackoverflow.com/a/377028/548792
     winprog_exts = _get_exe_extensions()
 

Diff for: test-requirements.txt

@@ -9,4 +9,3 @@ pytest-cov
 pytest-instafail
 pytest-mock
 pytest-sugar
-sumtypes

Diff for: 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')