From dc95a768b3381cf5a68fc9ad4e8dc4bba37b0ca0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 7 Mar 2024 11:34:11 -0500 Subject: [PATCH] Fix mypy error with creationflags in subprocess module On Windows, Python's subprocess module contains constants useful to pass to the `creationflags` parameter of subprocess.Popen. These are absent on other platforms, where they are not meaningful. The code was already safe at runtime from any AttributeError related to these constants, because they were only used in git.cmd._safer_popen_windows, which was defined on Windows. But in #1792 I did not write the code in a way where mypy can verify its correctness. So if a regression of the kind mypy can in principle catch were to occur, it would be harder to notice it. This refactors the code, keeping the same behavior but expressing it in a way mypy can understand. This consists of two changes: 1. Only define _safer_popen_windows when the platform is Windows, placing it in the first branch of the `if` statement. This is needed because mypy will not take the only current call to that nonpublic function being on Windows as sufficient evidence that the platform is always Windows when it is run. 2. Determine the platform, for this purpose, using sys.platform instead of os.name. These are far from equivalent in general (see the deprecation rationale for is_ in #1732, revised in a0fa2bd in #1787). However, in Python 3 (GitPython no longer supports Python 2), in the specific case of Windows, we have a choice of which to use, as both `sys.platform == "win32"` and `os.name == "nt"`. os.name is "nt" on native Windows, and "posix" on Cygwin. sys.platform is "win32" on native Windows (including 64-bit systems with 64-bit Python builds), and "cygwin" on Cygwin. See: https://docs.python.org/3/library/sys.html#sys.platform This is needed because the type stubs for the subprocess module use this sys.platform check (rather than an os.name check) to determine if the platform is Windows for the purpose of deciding which constants to say the subprocess module defines. I have verified that neither of these changes is enough by itself. --- git/cmd.py | 113 +++++++++++++++++++++++++++-------------------------- 1 file changed, 57 insertions(+), 56 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index c37d28988..04037c06f 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -14,6 +14,7 @@ import signal from subprocess import Popen, PIPE, DEVNULL import subprocess +import sys import threading from textwrap import dedent @@ -220,67 +221,67 @@ 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. +safer_popen: Callable[..., Popen] - :note: - This currently works by setting the :envvar:`NoDefaultCurrentDirectoryInExePath` - environment variable during subprocess creation. It also takes care of passing - Windows-specific process creation flags, but that is unrelated to path search. +if sys.platform == "win32": - :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: - # The original may be immutable or reused by the caller. Make changes in a copy. - env = {} if env is None else dict(env) - env["NoDefaultCurrentDirectoryInExePath"] = "1" - - # 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 that is unpatched, then in the rare case the ComSpec environment - # variable is unset, the search for the shell itself is unsafe. Setting - # NoDefaultCurrentDirectoryInExePath in all cases, as is done here, is simpler and - # protects against that. (As above, the "1" can be any value.) - with patch_env("NoDefaultCurrentDirectoryInExePath", "1"): - return Popen( - command, - shell=shell, - env=env, - creationflags=creationflags, - **kwargs, - ) + 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 :envvar:`NoDefaultCurrentDirectoryInExePath` + environment variable during subprocess creation. It also takes care of passing + Windows-specific process creation flags, but that is unrelated to path search. -safer_popen: Callable[..., Popen] + :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: + # The original may be immutable or reused by the caller. Make changes in a copy. + env = {} if env is None else dict(env) + env["NoDefaultCurrentDirectoryInExePath"] = "1" + + # 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 that is unpatched, then in the rare case the ComSpec environment + # variable is unset, the search for the shell itself is unsafe. Setting + # NoDefaultCurrentDirectoryInExePath in all cases, as is done here, is simpler and + # protects against that. (As above, the "1" can be any value.) + with patch_env("NoDefaultCurrentDirectoryInExePath", "1"): + return Popen( + command, + shell=shell, + env=env, + creationflags=creationflags, + **kwargs, + ) -if os.name == "nt": safer_popen = _safer_popen_windows else: safer_popen = Popen