diff --git a/git/cmd.py b/git/cmd.py index c4dea0393..4413182e0 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, ) @@ -46,6 +46,7 @@ Iterator, List, Mapping, + Optional, Sequence, TYPE_CHECKING, TextIO, @@ -102,7 +103,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: @@ -207,6 +208,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("_", "-") diff --git a/git/index/fun.py b/git/index/fun.py index 500fb251d..580493f6d 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -18,7 +18,7 @@ ) import subprocess -from git.cmd import handle_process_output +from git.cmd import handle_process_output, safer_popen from git.compat import defenc, force_bytes, force_text, safe_decode from git.exc import HookExecutionError, UnmergedEntriesError from git.objects.fun import ( @@ -26,7 +26,7 @@ traverse_trees_recursive, tree_to_stream, ) -from git.util import IndexFileSHA1Writer, finalize_process, safer_popen +from git.util import IndexFileSHA1Writer, finalize_process from gitdb.base import IStream from gitdb.typ import str_tree_type diff --git a/git/util.py b/git/util.py index 5f5fb8566..5ccd2b04c 100644 --- a/git/util.py +++ b/git/util.py @@ -33,7 +33,6 @@ IO, Iterator, List, - Mapping, Optional, Pattern, Sequence, @@ -536,67 +535,6 @@ 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 39e19bb8a..3b9abc712 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, safer_popen +from git.util import cwd, finalize_process from test.lib import TestBase, fixture_path, with_rw_directory @@ -114,7 +114,6 @@ 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, "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. @@ -389,7 +388,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]