Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitc551e91

Browse files
committed
Extract shared logic for using Popen safely on Windows
This creates git.util.safer_popen that, on non-Windows systems, isbound to subprocess.Popen (to avoid introducing unnecessarylatency). On Windows, it is a function that wraps subprocess.Popen,consolidating two pieces of logic that had previously beenduplicated:1. Temporarily setting NoDefaultCurrentDirectoryInExePath in the calling environment and, when shell=True is used, setting it in the subprocess environment as well. This prevents executables specified as single names (which are mainly "git" and, for hooks, "bash.exe") from being searched for in the current working directory of GitPython or, when a shell is used, the current working directory of the shell used to run them.2. Passing the CREATE_NO_WINDOW and CREATE_NEW_PROCESS_GROUP flags as creationflags. This is not a security measure. It is indirectly related to safety in that CREATE_NO_WINDOW eliminated at least some, and possibly all, cases where calling Git.execute (directly, or indirectly via a dynamic method) with shell=True conferred an advantage over the inherently more secure default of shell=False; and CREATE_NEW_PROCESS facilitates some ways of terminating subprocesses that would otherwise be unavailable, thereby making resource exhaustion less likely. But really the reason I included creationflags here is that it seems it should always be used in the same situations as preventing the current directory from being searched (and always was), and including it further reduces code duplication and simplifies calling code.This commit does not improve security or robustness, because thesefeatures were already present. Instead, this moves them to a singlelocation. It also documents them by giving the function bound tosafer_popen on Windows, _safer_popen_windows, a detailed docstring.Because there would otherwise be potential for confusion on thedifferent ways to perform or customize path searches, I have alsoadded a doctring to py_where noting its limited use case and itsrelationship to shutil.which and non-shell search.(The search in _safer_popen_windows is typically a non-shellsearch, which is why it cannot be reimplemented to do its ownlookup by calling an only slightly modified version ofshutil.which, without a risk of breaking some currently workinguses. It may, however, be possible to fix the race condition bydoing something analogous for Windows non-shell search behavior,which is largely but not entirely described in the documentationfor CreateProcessW.)
1 parent15ebb25 commitc551e91

File tree

4 files changed

+106
-59
lines changed

4 files changed

+106
-59
lines changed

‎git/cmd.py

Lines changed: 15 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@
2929
cygpath,
3030
expand_path,
3131
is_cygwin_git,
32-
patch_env,
3332
remove_password_if_present,
33+
safer_popen,
3434
stream_copy,
3535
)
3636

@@ -225,14 +225,6 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc
225225
## -- End Utilities -- @}
226226

227227

228-
ifos.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-
236228
classGit(LazyMixin):
237229
"""The Git class manages communication with the Git binary.
238230
@@ -985,28 +977,20 @@ def execute(
985977
ifinline_envisnotNone:
986978
env.update(inline_env)
987979

988-
ifshellisNone:
989-
shell=self.USE_SHELL
990-
991980
ifos.name=="nt":
992981
cmd_not_found_exception=OSError
993982
ifkill_after_timeoutisnotNone:
994983
raiseGitCommandError(
995984
redacted_command,
996985
'"kill_after_timeout" feature is not supported on Windows.',
997986
)
998-
# Search PATH but not CWD. The "1" can be any value. We'll patch just before
999-
# the Popen call and unpatch just after, or we get a worse race condition.
1000-
maybe_patch_caller_env=patch_env("NoDefaultCurrentDirectoryInExePath","1")
1001-
ifshell:
1002-
# Modify the direct shell subprocess's own search behavior accordingly.
1003-
env["NoDefaultCurrentDirectoryInExePath"]="1"
1004987
else:
1005988
cmd_not_found_exception=FileNotFoundError
1006-
maybe_patch_caller_env=contextlib.nullcontext()
1007989
# END handle
1008990

1009991
stdout_sink=PIPEifwith_stdoutelsegetattr(subprocess,"DEVNULL",None)oropen(os.devnull,"wb")
992+
ifshellisNone:
993+
shell=self.USE_SHELL
1010994
log.debug(
1011995
"Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)",
1012996
redacted_command,
@@ -1016,20 +1000,18 @@ def execute(
10161000
universal_newlines,
10171001
)
10181002
try:
1019-
withmaybe_patch_caller_env:
1020-
proc=Popen(
1021-
command,
1022-
env=env,
1023-
cwd=cwd,
1024-
bufsize=-1,
1025-
stdin=(istreamorDEVNULL),
1026-
stderr=PIPE,
1027-
stdout=stdout_sink,
1028-
shell=shell,
1029-
universal_newlines=universal_newlines,
1030-
creationflags=PROC_CREATIONFLAGS,
1031-
**subprocess_kwargs,
1032-
)
1003+
proc=safer_popen(
1004+
command,
1005+
env=env,
1006+
cwd=cwd,
1007+
bufsize=-1,
1008+
stdin=(istreamorDEVNULL),
1009+
stderr=PIPE,
1010+
stdout=stdout_sink,
1011+
shell=shell,
1012+
universal_newlines=universal_newlines,
1013+
**subprocess_kwargs,
1014+
)
10331015
exceptcmd_not_found_exceptionaserr:
10341016
raiseGitCommandNotFound(redacted_command,err)fromerr
10351017
else:

‎git/index/fun.py

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
"""Standalone functions to accompany the index implementation and make it more versatile."""
55

6-
importcontextlib
76
fromioimportBytesIO
87
importos
98
importos.pathasosp
@@ -19,15 +18,15 @@
1918
)
2019
importsubprocess
2120

22-
fromgit.cmdimportPROC_CREATIONFLAGS,handle_process_output
21+
fromgit.cmdimporthandle_process_output
2322
fromgit.compatimportdefenc,force_bytes,force_text,safe_decode
2423
fromgit.excimportHookExecutionError,UnmergedEntriesError
2524
fromgit.objects.funimport (
2625
traverse_tree_recursive,
2726
traverse_trees_recursive,
2827
tree_to_stream,
2928
)
30-
fromgit.utilimportIndexFileSHA1Writer,finalize_process,patch_env
29+
fromgit.utilimportIndexFileSHA1Writer,finalize_process,safer_popen
3130
fromgitdb.baseimportIStream
3231
fromgitdb.typimportstr_tree_type
3332

@@ -91,10 +90,6 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
9190
env=os.environ.copy()
9291
env["GIT_INDEX_FILE"]=safe_decode(str(index.path))
9392
env["GIT_EDITOR"]=":"
94-
ifos.name=="nt":
95-
maybe_patch_caller_env=patch_env("NoDefaultCurrentDirectoryInExePath","1")
96-
else:
97-
maybe_patch_caller_env=contextlib.nullcontext()
9893
cmd= [hp]
9994
try:
10095
ifos.name=="nt"andnot_has_file_extension(hp):
@@ -103,15 +98,13 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
10398
relative_hp=Path(hp).relative_to(index.repo.working_dir).as_posix()
10499
cmd= ["bash.exe",relative_hp]
105100

106-
withmaybe_patch_caller_env:
107-
process=subprocess.Popen(
108-
cmd+list(args),
109-
env=env,
110-
stdout=subprocess.PIPE,
111-
stderr=subprocess.PIPE,
112-
cwd=index.repo.working_dir,
113-
creationflags=PROC_CREATIONFLAGS,
114-
)
101+
process=safer_popen(
102+
cmd+list(args),
103+
env=env,
104+
stdout=subprocess.PIPE,
105+
stderr=subprocess.PIPE,
106+
cwd=index.repo.working_dir,
107+
)
115108
exceptExceptionasex:
116109
raiseHookExecutionError(hp,ex)fromex
117110
else:

‎git/util.py

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
IO,
3434
Iterator,
3535
List,
36+
Mapping,
3637
Optional,
3738
Pattern,
3839
Sequence,
@@ -327,6 +328,17 @@ def _get_exe_extensions() -> Sequence[str]:
327328

328329

329330
defpy_where(program:str,path:Optional[PathLike]=None)->List[str]:
331+
"""Perform a path search to assist :func:`is_cygwin_git`.
332+
333+
This is not robust for general use. It is an implementation detail of
334+
:func:`is_cygwin_git`. When a search following all shell rules is needed,
335+
:func:`shutil.which` can be used instead.
336+
337+
:note: Neither this function nor :func:`shutil.which` will predict the effect of an
338+
executable search on a native Windows system due to a :class:`subprocess.Popen`
339+
call without ``shell=True``, because shell and non-shell executable search on
340+
Windows differ considerably.
341+
"""
330342
# From: http://stackoverflow.com/a/377028/548792
331343
winprog_exts=_get_exe_extensions()
332344

@@ -524,6 +536,67 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]:
524536
returnnew_cmdline
525537

526538

539+
def_safer_popen_windows(
540+
command:Union[str,Sequence[Any]],
541+
*,
542+
shell:bool=False,
543+
env:Optional[Mapping[str,str]]=None,
544+
**kwargs:Any,
545+
)->subprocess.Popen:
546+
"""Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
547+
548+
This avoids an untrusted search path condition where a file like ``git.exe`` in a
549+
malicious repository would be run when GitPython operates on the repository. The
550+
process using GitPython may have an untrusted repository's working tree as its
551+
current working directory. Some operations may temporarily change to that directory
552+
before running a subprocess. In addition, while by default GitPython does not run
553+
external commands with a shell, it can be made to do so, in which case the CWD of
554+
the subprocess, which GitPython usually sets to a repository working tree, can
555+
itself be searched automatically by the shell. This wrapper covers all those cases.
556+
557+
:note: This currently works by setting the ``NoDefaultCurrentDirectoryInExePath``
558+
environment variable during subprocess creation. It also takes care of passing
559+
Windows-specific process creation flags, but that is unrelated to path search.
560+
561+
:note: The current implementation contains a race condition on :attr:`os.environ`.
562+
GitPython isn't thread-safe, but a program using it on one thread should ideally
563+
be able to mutate :attr:`os.environ` on another, without unpredictable results.
564+
See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
565+
"""
566+
# CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
567+
# https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
568+
# https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
569+
creationflags=subprocess.CREATE_NO_WINDOW|subprocess.CREATE_NEW_PROCESS_GROUP
570+
571+
# When using a shell, the shell is the direct subprocess, so the variable must be
572+
# set in its environment, to affect its search behavior. (The "1" can be any value.)
573+
ifshell:
574+
safer_env= {}ifenvisNoneelsedict(env)
575+
safer_env["NoDefaultCurrentDirectoryInExePath"]="1"
576+
else:
577+
safer_env=env
578+
579+
# When not using a shell, the current process does the search in a CreateProcessW
580+
# API call, so the variable must be set in our environment. With a shell, this is
581+
# unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
582+
# patched. If not, in the rare case the ComSpec environment variable is unset, the
583+
# shell is searched for unsafely. Setting NoDefaultCurrentDirectoryInExePath in all
584+
# cases, as here, is simpler and protects against that. (The "1" can be any value.)
585+
withpatch_env("NoDefaultCurrentDirectoryInExePath","1"):
586+
returnsubprocess.Popen(
587+
command,
588+
shell=shell,
589+
env=safer_env,
590+
creationflags=creationflags,
591+
**kwargs,
592+
)
593+
594+
595+
ifos.name=="nt":
596+
safer_popen=_safer_popen_windows
597+
else:
598+
safer_popen=subprocess.Popen
599+
527600
# } END utilities
528601

529602
# { Classes

‎test/test_git.py

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
importddt
2626

2727
fromgitimportGit,refresh,GitCommandError,GitCommandNotFound,Repo,cmd
28-
fromgit.utilimportcwd,finalize_process
28+
fromgit.utilimportcwd,finalize_process,safer_popen
2929
fromtest.libimportTestBase,fixture_path,with_rw_directory
3030

3131

@@ -115,28 +115,28 @@ def test_it_transforms_kwargs_into_git_command_arguments(self):
115115
def_do_shell_combo(self,value_in_call,value_from_class):
116116
withmock.patch.object(Git,"USE_SHELL",value_from_class):
117117
# git.cmd gets Popen via a "from" import, so patch it there.
118-
withmock.patch.object(cmd,"Popen",wraps=cmd.Popen)asmock_popen:
118+
withmock.patch.object(cmd,"safer_popen",wraps=cmd.safer_popen)asmock_safer_popen:
119119
# Use a command with no arguments (besides the program name), so it runs
120120
# with or without a shell, on all OSes, with the same effect.
121121
self.git.execute(["git"],with_exceptions=False,shell=value_in_call)
122122

123-
returnmock_popen
123+
returnmock_safer_popen
124124

125125
@ddt.idata(_shell_cases)
126126
deftest_it_uses_shell_or_not_as_specified(self,case):
127127
"""A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`."""
128128
value_in_call,value_from_class,expected_popen_arg=case
129-
mock_popen=self._do_shell_combo(value_in_call,value_from_class)
130-
mock_popen.assert_called_once()
131-
self.assertIs(mock_popen.call_args.kwargs["shell"],expected_popen_arg)
129+
mock_safer_popen=self._do_shell_combo(value_in_call,value_from_class)
130+
mock_safer_popen.assert_called_once()
131+
self.assertIs(mock_safer_popen.call_args.kwargs["shell"],expected_popen_arg)
132132

133133
@ddt.idata(full_case[:2]forfull_casein_shell_cases)
134134
deftest_it_logs_if_it_uses_a_shell(self,case):
135135
"""``shell=`` in the log message agrees with what is passed to `Popen`."""
136136
value_in_call,value_from_class=case
137137
withself.assertLogs(cmd.log,level=logging.DEBUG)aslog_watcher:
138-
mock_popen=self._do_shell_combo(value_in_call,value_from_class)
139-
self._assert_logged_for_popen(log_watcher,"shell",mock_popen.call_args.kwargs["shell"])
138+
mock_safer_popen=self._do_shell_combo(value_in_call,value_from_class)
139+
self._assert_logged_for_popen(log_watcher,"shell",mock_safer_popen.call_args.kwargs["shell"])
140140

141141
@ddt.data(
142142
("None",None),
@@ -405,13 +405,12 @@ def counter_stderr(line):
405405
fixture_path("cat_file.py"),
406406
str(fixture_path("issue-301_stderr")),
407407
]
408-
proc=subprocess.Popen(
408+
proc=safer_popen(
409409
cmdline,
410410
stdin=None,
411411
stdout=subprocess.PIPE,
412412
stderr=subprocess.PIPE,
413413
shell=False,
414-
creationflags=cmd.PROC_CREATIONFLAGS,
415414
)
416415

417416
handle_process_output(proc,counter_stdout,counter_stderr,finalize_process)

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp