Merge pull request #1792 from EliahKagan/popen · gitpython-developers/GitPython@ef3192c

@@ -46,6 +46,7 @@

4646

Iterator,

4747

List,

4848

Mapping,

49+

Optional,

4950

Sequence,

5051

TYPE_CHECKING,

5152

TextIO,

@@ -102,7 +103,7 @@ def handle_process_output(

102103

Callable[[bytes, "Repo", "DiffIndex"], None],

103104

],

104105

stderr_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,

106107

decode_streams: bool = True,

107108

kill_after_timeout: Union[None, float] = None,

108109

) -> None:

@@ -207,6 +208,68 @@ def pump_stream(

207208

finalizer(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+210273

def dashify(string: str) -> str:

211274

return 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-236291

class Git(LazyMixin):

237292

"""The Git class manages communication with the Git binary.

238293

@@ -992,11 +1047,8 @@ def execute(

9921047

redacted_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")

9971050

else:

9981051

cmd_not_found_exception = FileNotFoundError

999-

maybe_patch_caller_env = contextlib.nullcontext()

10001052

# END handle

1001105310021054

stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")

@@ -1011,20 +1063,18 @@ def execute(

10111063

universal_newlines,

10121064

)

10131065

try:

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+

)

10281078

except cmd_not_found_exception as err:

10291079

raise GitCommandNotFound(redacted_command, err) from err

10301080

else: