Merge pull request #1792 from EliahKagan/popen

Fix two remaining Windows untrusted search path cases

Author: Byron

.pre-commit-config.yaml

@@ -29,7 +29,7 @@ repos:
 hooks:
  - id: shellcheck
 args: [--color]
-exclude: ^git/ext/
+exclude: ^test/fixtures/polyglot$|^git/ext/
 
 - repo: https://github.com/pre-commit/pre-commit-hooks
 rev: v4.4.0

git/cmd.py

@@ -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.)
+ifshell:
+safer_env={} ifenvisNoneelsedict(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.)
+withpatch_env("NoDefaultCurrentDirectoryInExePath", "1"):
+returnPopen(
+command,
+shell=shell,
+env=safer_env,
+creationflags=creationflags,
+**kwargs,
+ )
+
+
+ifos.name=="nt":
+safer_popen=_safer_popen_windows
+else:
+safer_popen=Popen
+
+
 defdashify(string: str) ->str:
 returnstring.replace("_", "-")
 
@@ -225,14 +288,6 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc
 ## -- End Utilities -- @}
 
 
-ifos.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
-
-
 classGit(LazyMixin):
 """The Git class manages communication with the Git binary.
 
@@ -992,11 +1047,8 @@ def execute(
 redacted_command,
 '"kill_after_timeout" feature is not supported on Windows.',
  )
-# Only search PATH, not CWD. This must be in the *caller* environment. The "1" can be any value.
-maybe_patch_caller_env=patch_env("NoDefaultCurrentDirectoryInExePath", "1")
 else:
 cmd_not_found_exception=FileNotFoundError
-maybe_patch_caller_env=contextlib.nullcontext()
 # END handle
 
 stdout_sink=PIPEifwith_stdoutelsegetattr(subprocess, "DEVNULL", None) oropen(os.devnull, "wb")
@@ -1011,20 +1063,18 @@ def execute(
 universal_newlines,
  )
 try:
-withmaybe_patch_caller_env:
-proc=Popen(
-command,
-env=env,
-cwd=cwd,
-bufsize=-1,
-stdin=(istreamorDEVNULL),
-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=(istreamorDEVNULL),
+stderr=PIPE,
+stdout=stdout_sink,
+shell=shell,
+universal_newlines=universal_newlines,
+**subprocess_kwargs,
+ )
 exceptcmd_not_found_exceptionaserr:
 raiseGitCommandNotFound(redacted_command, err) fromerr
 else:

git/index/fun.py

@@ -18,7 +18,7 @@
 )
 importsubprocess
 
-fromgit.cmdimportPROC_CREATIONFLAGS, handle_process_output
+fromgit.cmdimporthandle_process_output, safer_popen
 fromgit.compatimportdefenc, force_bytes, force_text, safe_decode
 fromgit.excimportHookExecutionError, UnmergedEntriesError
 fromgit.objects.funimport (
@@ -98,13 +98,12 @@ 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]
 
-process=subprocess.Popen(
+process=safer_popen(
 cmd+list(args),
 env=env,
 stdout=subprocess.PIPE,
 stderr=subprocess.PIPE,
 cwd=index.repo.working_dir,
-creationflags=PROC_CREATIONFLAGS,
  )
 exceptExceptionasex:
 raiseHookExecutionError(hp, ex) fromex

git/util.py

@@ -327,6 +327,17 @@ def _get_exe_extensions() -> Sequence[str]:
 
 
 defpy_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()
 

test/fixtures/polyglot

@@ -0,0 +1,8 @@
+#!/usr/bin/env sh
+# Valid script in both Bash and Python, but with different behavior.
+""":"
+echo'Ran intended hook.'>output.txt
+exit
+""""
+from pathlib import Path
+Path('payload.txt').write_text('Ran impostor hook!', encoding='utf-8')

test/lib/helper.py

@@ -14,6 +14,7 @@
 importtextwrap
 importtime
 importunittest
+importvenv
 
 importgitdb
 
@@ -36,6 +37,7 @@
 "with_rw_repo",
 "with_rw_and_rw_remote_repo",
 "TestBase",
+"VirtualEnvironment",
 "TestCase",
 "SkipTest",
 "skipIf",
@@ -88,11 +90,11 @@ def with_rw_directory(func):
  test succeeds, but leave it otherwise to aid additional debugging."""
 
 @wraps(func)
-defwrapper(self):
+defwrapper(self, *args, **kwargs):
 path=tempfile.mkdtemp(prefix=func.__name__)
 keep=False
 try:
-returnfunc(self, path)
+returnfunc(self, path, *args, **kwargs)
 exceptException:
 log.info(
 "Test %s.%s failed, output is at %r\n",
@@ -390,3 +392,46 @@ def _make_file(self, rela_path, data, repo=None):
 withopen(abs_path, "w") asfp:
 fp.write(data)
 returnabs_path
+
+
+classVirtualEnvironment:
+"""A newly created Python virtual environment for use in a test."""
+
+__slots__= ("_env_dir",)
+
+def__init__(self, env_dir, *, with_pip):
+ifos.name=="nt":
+self._env_dir=osp.realpath(env_dir)
+venv.create(self.env_dir, symlinks=False, with_pip=with_pip)
+else:
+self._env_dir=env_dir
+venv.create(self.env_dir, symlinks=True, with_pip=with_pip)
+
+@property
+defenv_dir(self):
+"""The top-level directory of the environment."""
+returnself._env_dir
+
+@property
+defpython(self):
+"""Path to the Python executable in the environment."""
+returnself._executable("python")
+
+@property
+defpip(self):
+"""Path to the pip executable in the environment, or RuntimeError if absent."""
+returnself._executable("pip")
+
+@property
+defsources(self):
+"""Path to a src directory in the environment, which may not exist yet."""
+returnos.path.join(self.env_dir, "src")
+
+def_executable(self, basename):
+ifos.name=="nt":
+path=osp.join(self.env_dir, "Scripts", basename+".exe")
+else:
+path=osp.join(self.env_dir, "bin", basename)
+ifosp.isfile(path) orosp.islink(path):
+returnpath
+raiseRuntimeError(f"no regular file or symlink {path!r}")