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

Commitef3192c

Browse files
authored
Merge pull request#1792 from EliahKagan/popen
Fix two remaining Windows untrusted search path cases
2 parents32c02d1 +1f3caa3 commitef3192c

File tree

9 files changed

+289
-84
lines changed

9 files changed

+289
-84
lines changed

‎.pre-commit-config.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ repos:
2929
hooks:
3030
-id:shellcheck
3131
args:[--color]
32-
exclude:^git/ext/
32+
exclude:^test/fixtures/polyglot$|^git/ext/
3333

3434
-repo:https://github.com/pre-commit/pre-commit-hooks
3535
rev:v4.4.0

‎git/cmd.py

Lines changed: 76 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -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)
208209

209210

211+
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+
ifshell:
246+
safer_env= {}ifenvisNoneelsedict(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+
withpatch_env("NoDefaultCurrentDirectoryInExePath","1"):
258+
returnPopen(
259+
command,
260+
shell=shell,
261+
env=safer_env,
262+
creationflags=creationflags,
263+
**kwargs,
264+
)
265+
266+
267+
ifos.name=="nt":
268+
safer_popen=_safer_popen_windows
269+
else:
270+
safer_popen=Popen
271+
272+
210273
defdashify(string:str)->str:
211274
returnstring.replace("_","-")
212275

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

227290

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-
236291
classGit(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
10011053

10021054
stdout_sink=PIPEifwith_stdoutelsegetattr(subprocess,"DEVNULL",None)oropen(os.devnull,"wb")
@@ -1011,20 +1063,18 @@ def execute(
10111063
universal_newlines,
10121064
)
10131065
try:
1014-
withmaybe_patch_caller_env:
1015-
proc=Popen(
1016-
command,
1017-
env=env,
1018-
cwd=cwd,
1019-
bufsize=-1,
1020-
stdin=(istreamorDEVNULL),
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=(istreamorDEVNULL),
1072+
stderr=PIPE,
1073+
stdout=stdout_sink,
1074+
shell=shell,
1075+
universal_newlines=universal_newlines,
1076+
**subprocess_kwargs,
1077+
)
10281078
exceptcmd_not_found_exceptionaserr:
10291079
raiseGitCommandNotFound(redacted_command,err)fromerr
10301080
else:

‎git/index/fun.py

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
)
1919
importsubprocess
2020

21-
fromgit.cmdimportPROC_CREATIONFLAGS,handle_process_output
21+
fromgit.cmdimporthandle_process_output,safer_popen
2222
fromgit.compatimportdefenc,force_bytes,force_text,safe_decode
2323
fromgit.excimportHookExecutionError,UnmergedEntriesError
2424
fromgit.objects.funimport (
@@ -98,13 +98,12 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
9898
relative_hp=Path(hp).relative_to(index.repo.working_dir).as_posix()
9999
cmd= ["bash.exe",relative_hp]
100100

101-
process=subprocess.Popen(
101+
process=safer_popen(
102102
cmd+list(args),
103103
env=env,
104104
stdout=subprocess.PIPE,
105105
stderr=subprocess.PIPE,
106106
cwd=index.repo.working_dir,
107-
creationflags=PROC_CREATIONFLAGS,
108107
)
109108
exceptExceptionasex:
110109
raiseHookExecutionError(hp,ex)fromex

‎git/util.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,17 @@ def _get_exe_extensions() -> Sequence[str]:
327327

328328

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

‎test/fixtures/polyglot

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#!/usr/bin/env sh
2+
# Valid script in both Bash and Python, but with different behavior.
3+
""":"
4+
echo'Ran intended hook.'>output.txt
5+
exit
6+
""""
7+
from pathlib import Path
8+
Path('payload.txt').write_text('Ran impostor hook!', encoding='utf-8')

‎test/lib/helper.py

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
importtextwrap
1515
importtime
1616
importunittest
17+
importvenv
1718

1819
importgitdb
1920

@@ -36,6 +37,7 @@
3637
"with_rw_repo",
3738
"with_rw_and_rw_remote_repo",
3839
"TestBase",
40+
"VirtualEnvironment",
3941
"TestCase",
4042
"SkipTest",
4143
"skipIf",
@@ -88,11 +90,11 @@ def with_rw_directory(func):
8890
test succeeds, but leave it otherwise to aid additional debugging."""
8991

9092
@wraps(func)
91-
defwrapper(self):
93+
defwrapper(self,*args,**kwargs):
9294
path=tempfile.mkdtemp(prefix=func.__name__)
9395
keep=False
9496
try:
95-
returnfunc(self,path)
97+
returnfunc(self,path,*args,**kwargs)
9698
exceptException:
9799
log.info(
98100
"Test %s.%s failed, output is at %r\n",
@@ -390,3 +392,46 @@ def _make_file(self, rela_path, data, repo=None):
390392
withopen(abs_path,"w")asfp:
391393
fp.write(data)
392394
returnabs_path
395+
396+
397+
classVirtualEnvironment:
398+
"""A newly created Python virtual environment for use in a test."""
399+
400+
__slots__= ("_env_dir",)
401+
402+
def__init__(self,env_dir,*,with_pip):
403+
ifos.name=="nt":
404+
self._env_dir=osp.realpath(env_dir)
405+
venv.create(self.env_dir,symlinks=False,with_pip=with_pip)
406+
else:
407+
self._env_dir=env_dir
408+
venv.create(self.env_dir,symlinks=True,with_pip=with_pip)
409+
410+
@property
411+
defenv_dir(self):
412+
"""The top-level directory of the environment."""
413+
returnself._env_dir
414+
415+
@property
416+
defpython(self):
417+
"""Path to the Python executable in the environment."""
418+
returnself._executable("python")
419+
420+
@property
421+
defpip(self):
422+
"""Path to the pip executable in the environment, or RuntimeError if absent."""
423+
returnself._executable("pip")
424+
425+
@property
426+
defsources(self):
427+
"""Path to a src directory in the environment, which may not exist yet."""
428+
returnos.path.join(self.env_dir,"src")
429+
430+
def_executable(self,basename):
431+
ifos.name=="nt":
432+
path=osp.join(self.env_dir,"Scripts",basename+".exe")
433+
else:
434+
path=osp.join(self.env_dir,"bin",basename)
435+
ifosp.isfile(path)orosp.islink(path):
436+
returnpath
437+
raiseRuntimeError(f"no regular file or symlink{path!r}")

0 commit comments

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