diff --git a/git/cmd.py b/git/cmd.py index 3d26dbad2..c4dea0393 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -29,8 +29,8 @@ cygpath, expand_path, is_cygwin_git, - patch_env, remove_password_if_present, + safer_popen, stream_copy, ) @@ -225,14 +225,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. @@ -985,9 +977,6 @@ def execute( if inline_env is not None: env.update(inline_env) - if shell is None: - shell = self.USE_SHELL - if os.name == "nt": cmd_not_found_exception = OSError if kill_after_timeout is not None: @@ -995,18 +984,13 @@ def execute( redacted_command, '"kill_after_timeout" feature is not supported on Windows.', ) - # Search PATH but not CWD. The "1" can be any value. We'll patch just before - # the Popen call and unpatch just after, or we get a worse race condition. - maybe_patch_caller_env = patch_env("NoDefaultCurrentDirectoryInExePath", "1") - if shell: - # Modify the direct shell subprocess's own search behavior accordingly. - 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") + if shell is None: + shell = self.USE_SHELL log.debug( "Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)", redacted_command, @@ -1016,20 +1000,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 --git a/git/index/fun.py b/git/index/fun.py index 303141bbe..500fb251d 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -3,7 +3,6 @@ """Standalone functions to accompany the index implementation and make it more versatile.""" -import contextlib from io import BytesIO import os import os.path as osp @@ -19,7 +18,7 @@ ) import subprocess -from git.cmd import PROC_CREATIONFLAGS, handle_process_output +from git.cmd import handle_process_output from git.compat import defenc, force_bytes, force_text, safe_decode from git.exc import HookExecutionError, UnmergedEntriesError from git.objects.fun import ( @@ -27,7 +26,7 @@ traverse_trees_recursive, tree_to_stream, ) -from git.util import IndexFileSHA1Writer, finalize_process, patch_env +from git.util import IndexFileSHA1Writer, finalize_process, safer_popen from gitdb.base import IStream from gitdb.typ import str_tree_type @@ -91,10 +90,6 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None: env = os.environ.copy() env["GIT_INDEX_FILE"] = safe_decode(str(index.path)) env["GIT_EDITOR"] = ":" - if os.name == "nt": - maybe_patch_caller_env = patch_env("NoDefaultCurrentDirectoryInExePath", "1") - else: - maybe_patch_caller_env = contextlib.nullcontext() cmd = [hp] try: if os.name == "nt" and not _has_file_extension(hp): @@ -103,15 +98,13 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None: relative_hp = Path(hp).relative_to(index.repo.working_dir).as_posix() cmd = ["bash.exe", relative_hp] - with maybe_patch_caller_env: - process = subprocess.Popen( - cmd + list(args), - env=env, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - cwd=index.repo.working_dir, - creationflags=PROC_CREATIONFLAGS, - ) + process = safer_popen( + cmd + list(args), + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + cwd=index.repo.working_dir, + ) except Exception as ex: raise HookExecutionError(hp, ex) from ex else: diff --git a/git/util.py b/git/util.py index c5afea241..5f5fb8566 100644 --- a/git/util.py +++ b/git/util.py @@ -33,6 +33,7 @@ IO, Iterator, List, + Mapping, Optional, Pattern, Sequence, @@ -327,6 +328,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() @@ -524,6 +536,67 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: return new_cmdline +def _safer_popen_windows( + command: Union[str, Sequence[Any]], + *, + shell: bool = False, + env: Optional[Mapping[str, str]] = None, + **kwargs: Any, +) -> subprocess.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 subprocess.Popen( + command, + shell=shell, + env=safer_env, + creationflags=creationflags, + **kwargs, + ) + + +if os.name == "nt": + safer_popen = _safer_popen_windows +else: + safer_popen = subprocess.Popen + # } END utilities # { Classes diff --git a/test/test_git.py b/test/test_git.py index e80ad37ef..39e19bb8a 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -25,7 +25,7 @@ import ddt from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd -from git.util import cwd, finalize_process +from git.util import cwd, finalize_process, safer_popen from test.lib import TestBase, fixture_path, with_rw_directory @@ -115,28 +115,28 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): def _do_shell_combo(self, value_in_call, value_from_class): with mock.patch.object(Git, "USE_SHELL", value_from_class): # git.cmd gets Popen via a "from" import, so patch it there. - with mock.patch.object(cmd, "Popen", wraps=cmd.Popen) as mock_popen: + with mock.patch.object(cmd, "safer_popen", wraps=cmd.safer_popen) as mock_safer_popen: # Use a command with no arguments (besides the program name), so it runs # with or without a shell, on all OSes, with the same effect. self.git.execute(["git"], with_exceptions=False, shell=value_in_call) - return mock_popen + return mock_safer_popen @ddt.idata(_shell_cases) def test_it_uses_shell_or_not_as_specified(self, case): """A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`.""" value_in_call, value_from_class, expected_popen_arg = case - mock_popen = self._do_shell_combo(value_in_call, value_from_class) - mock_popen.assert_called_once() - self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg) + mock_safer_popen = self._do_shell_combo(value_in_call, value_from_class) + mock_safer_popen.assert_called_once() + self.assertIs(mock_safer_popen.call_args.kwargs["shell"], expected_popen_arg) @ddt.idata(full_case[:2] for full_case in _shell_cases) def test_it_logs_if_it_uses_a_shell(self, case): """``shell=`` in the log message agrees with what is passed to `Popen`.""" value_in_call, value_from_class = case with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher: - mock_popen = self._do_shell_combo(value_in_call, value_from_class) - self._assert_logged_for_popen(log_watcher, "shell", mock_popen.call_args.kwargs["shell"]) + mock_safer_popen = self._do_shell_combo(value_in_call, value_from_class) + self._assert_logged_for_popen(log_watcher, "shell", mock_safer_popen.call_args.kwargs["shell"]) @ddt.data( ("None", None), @@ -405,13 +405,12 @@ def counter_stderr(line): fixture_path("cat_file.py"), str(fixture_path("issue-301_stderr")), ] - proc = subprocess.Popen( + proc = safer_popen( cmdline, stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=False, - creationflags=cmd.PROC_CREATIONFLAGS, ) handle_process_output(proc, counter_stdout, counter_stderr, finalize_process)