Merge pull request #1792 from EliahKagan/popen · gitpython-developers/GitPython@ef3192c
@@ -46,6 +46,7 @@
4646Iterator,
4747List,
4848Mapping,
49+Optional,
4950Sequence,
5051TYPE_CHECKING,
5152TextIO,
@@ -102,7 +103,7 @@ def handle_process_output(
102103Callable[[bytes, "Repo", "DiffIndex"], None],
103104 ],
104105stderr_handler: Union[None, Callable[[AnyStr], None], Callable[[List[AnyStr]], None]],
105-finalizer: Union[None, Callable[[Union[subprocess.Popen, "Git.AutoInterrupt"]], None]] = None,
106+finalizer: Union[None, Callable[[Union[Popen, "Git.AutoInterrupt"]], None]] = None,
106107decode_streams: bool = True,
107108kill_after_timeout: Union[None, float] = None,
108109) -> None:
@@ -207,6 +208,68 @@ def pump_stream(
207208finalizer(process)
208209209210211+def _safer_popen_windows(
212+command: Union[str, Sequence[Any]],
213+*,
214+shell: bool = False,
215+env: Optional[Mapping[str, str]] = None,
216+**kwargs: Any,
217+) -> Popen:
218+"""Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
219+220+ This avoids an untrusted search path condition where a file like ``git.exe`` in a
221+ malicious repository would be run when GitPython operates on the repository. The
222+ process using GitPython may have an untrusted repository's working tree as its
223+ current working directory. Some operations may temporarily change to that directory
224+ before running a subprocess. In addition, while by default GitPython does not run
225+ external commands with a shell, it can be made to do so, in which case the CWD of
226+ the subprocess, which GitPython usually sets to a repository working tree, can
227+ itself be searched automatically by the shell. This wrapper covers all those cases.
228+229+ :note: This currently works by setting the ``NoDefaultCurrentDirectoryInExePath``
230+ environment variable during subprocess creation. It also takes care of passing
231+ Windows-specific process creation flags, but that is unrelated to path search.
232+233+ :note: The current implementation contains a race condition on :attr:`os.environ`.
234+ GitPython isn't thread-safe, but a program using it on one thread should ideally
235+ be able to mutate :attr:`os.environ` on another, without unpredictable results.
236+ See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
237+ """
238+# CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
239+# https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
240+# https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
241+creationflags = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
242+243+# When using a shell, the shell is the direct subprocess, so the variable must be
244+# set in its environment, to affect its search behavior. (The "1" can be any value.)
245+if shell:
246+safer_env = {} if env is None else dict(env)
247+safer_env["NoDefaultCurrentDirectoryInExePath"] = "1"
248+else:
249+safer_env = env
250+251+# When not using a shell, the current process does the search in a CreateProcessW
252+# API call, so the variable must be set in our environment. With a shell, this is
253+# unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
254+# patched. If not, in the rare case the ComSpec environment variable is unset, the
255+# shell is searched for unsafely. Setting NoDefaultCurrentDirectoryInExePath in all
256+# cases, as here, is simpler and protects against that. (The "1" can be any value.)
257+with patch_env("NoDefaultCurrentDirectoryInExePath", "1"):
258+return Popen(
259+command,
260+shell=shell,
261+env=safer_env,
262+creationflags=creationflags,
263+**kwargs,
264+ )
265+266+267+if os.name == "nt":
268+safer_popen = _safer_popen_windows
269+else:
270+safer_popen = Popen
271+272+210273def dashify(string: str) -> str:
211274return string.replace("_", "-")
212275@@ -225,14 +288,6 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc
225288## -- End Utilities -- @}
226289227290228-if os.name == "nt":
229-# CREATE_NEW_PROCESS_GROUP is needed to allow killing it afterwards. See:
230-# https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
231-PROC_CREATIONFLAGS = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
232-else:
233-PROC_CREATIONFLAGS = 0
234-235-236291class Git(LazyMixin):
237292"""The Git class manages communication with the Git binary.
238293@@ -992,11 +1047,8 @@ def execute(
9921047redacted_command,
9931048'"kill_after_timeout" feature is not supported on Windows.',
9941049 )
995-# Only search PATH, not CWD. This must be in the *caller* environment. The "1" can be any value.
996-maybe_patch_caller_env = patch_env("NoDefaultCurrentDirectoryInExePath", "1")
9971050else:
9981051cmd_not_found_exception = FileNotFoundError
999-maybe_patch_caller_env = contextlib.nullcontext()
10001052# END handle
1001105310021054stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
@@ -1011,20 +1063,18 @@ def execute(
10111063universal_newlines,
10121064 )
10131065try:
1014-with maybe_patch_caller_env:
1015-proc = Popen(
1016-command,
1017-env=env,
1018-cwd=cwd,
1019-bufsize=-1,
1020-stdin=(istream or DEVNULL),
1021-stderr=PIPE,
1022-stdout=stdout_sink,
1023-shell=shell,
1024-universal_newlines=universal_newlines,
1025-creationflags=PROC_CREATIONFLAGS,
1026-**subprocess_kwargs,
1027- )
1066+proc = safer_popen(
1067+command,
1068+env=env,
1069+cwd=cwd,
1070+bufsize=-1,
1071+stdin=(istream or DEVNULL),
1072+stderr=PIPE,
1073+stdout=stdout_sink,
1074+shell=shell,
1075+universal_newlines=universal_newlines,
1076+**subprocess_kwargs,
1077+ )
10281078except cmd_not_found_exception as err:
10291079raise GitCommandNotFound(redacted_command, err) from err
10301080else: