From 3eb7c2ab82e6dbe101ff916fca29d539cc2793af Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 21 Dec 2023 03:48:53 -0500 Subject: [PATCH] Move safer_popen from git.util to git.cmd I had originally written it in git.util because it is used from two separate modules (git.cmd and git.index.fun) and is arguably the same sort of thing as remove_password_if_present, in that both relate to running processes (and to security) and both are used from multiple modules yet are not meant for use outside GitPython. However, all of this is also true of git.cmd.handle_process_output, which this really has more in common with: it's a utility related *only* to the use of subprocesses, while remove_password_if_present can be used for other sanitization. In addition, it also replaces git.cmd.PROC_CREATIONFLAGS (also imported in git.index.fun) by passing that to Popen on Windows (since the situations where a custom value of creationflags should be used are the same as those where safer_popen should be called for its primary benefit of avoiding an untrusted path search when creating the subprocess). safer_popen and its Windows implementation _safer_popen_windows are thus moved from git/util.py to git/cmd.py, with related changes, such as to imports, done everywhere they are needed. --- git/cmd.py | 67 ++++++++++++++++++++++++++++++++++++++++++++++-- git/index/fun.py | 4 +-- git/util.py | 62 -------------------------------------------- test/test_git.py | 5 ++-- 4 files changed, 69 insertions(+), 69 deletions(-) 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]