X Tutup
Skip to content

Commit 5388147

Browse files
rickpricerickprice
committed
CVE-2024-22190 Hand Cherry-Pick gitpython-developers/GitPython/commit/ef3192cc414f2fd9978908454f6fd95243784c7f
1 parent 2d71ce7 commit 5388147

File tree

7 files changed

+287
-56
lines changed

7 files changed

+287
-56
lines changed

‎git/cmd.py‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
is_win,
3636
)
3737
from git.exc import CommandError, UnsafeOptionError, UnsafeProtocolError
38-
from git.util import is_cygwin_git, cygpath, expand_path
38+
from git.util import is_cygwin_git, cygpath, expand_path, patch_env
3939

4040
from .exc import (
4141
GitCommandError,
@@ -132,7 +132,7 @@ def pump_stream(cmdline, name, stream, is_decode, handler):
132132
return finalizer(process)
133133

134134

135-
def _safer_popen_windows(command, shell, env=None, **kwargs):
135+
def _safer_popen_windows(command, shell=False, env=None, **kwargs):
136136
"""Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
137137
This avoids an untrusted search path condition where a file like ``git.exe`` in a
138138
malicious repository would be run when GitPython operates on the repository. The
@@ -154,15 +154,13 @@ def _safer_popen_windows(command, shell, env=None, **kwargs):
154154
# https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
155155
# https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
156156
creationflags = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
157-
158157
# When using a shell, the shell is the direct subprocess, so the variable must be
159158
# set in its environment, to affect its search behavior. (The "1" can be any value.)
160159
if shell:
161160
safer_env = {} if env is None else dict(env)
162161
safer_env["NoDefaultCurrentDirectoryInExePath"] = "1"
163162
else:
164163
safer_env = env
165-
166164
# When not using a shell, the current process does the search in a CreateProcessW
167165
# API call, so the variable must be set in our environment. With a shell, this is
168166
# unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
@@ -185,6 +183,12 @@ def _safer_popen_windows(command, shell, env=None, **kwargs):
185183
safer_popen = Popen
186184

187185

186+
if os.name == "nt":
187+
safer_popen = _safer_popen_windows
188+
else:
189+
safer_popen = Popen
190+
191+
188192
def dashify(string):
189193
return string.replace('_', '-')
190194

@@ -801,15 +805,11 @@ def execute(self, command,
801805
cmd_not_found_exception = OSError
802806
if kill_after_timeout:
803807
raise GitCommandError(command, '"kill_after_timeout" feature is not supported on Windows.')
804-
805-
# Only search PATH, not CWD. This must be in the *caller* environment. The "1" can be any value.
806-
patch_caller_env = unittest.mock.patch.dict(os.environ, {"NoDefaultCurrentDirectoryInExePath": "1"})
807808
else:
808809
if sys.version_info[0] > 2:
809810
cmd_not_found_exception = FileNotFoundError # NOQA # exists, flake8 unknown @UndefinedVariable
810811
else:
811812
cmd_not_found_exception = OSError
812-
patch_caller_env = nullcontext()
813813
# end handle
814814

815815
stdout_sink = (PIPE
@@ -829,7 +829,7 @@ def execute(self, command,
829829
stdin=istream,
830830
stderr=PIPE,
831831
stdout=stdout_sink,
832-
shell=shell is not None and shell or self.USE_SHELL,
832+
shell=shell,
833833
universal_newlines=universal_newlines,
834834
**subprocess_kwargs
835835
)

‎git/index/fun.py‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,11 @@ def run_commit_hook(name, index, *args):
7474
env["GIT_EDITOR"] = ":"
7575
try:
7676
cmd = safer_popen([hp] + list(args),
77-
env=env,
78-
stdout=subprocess.PIPE,
79-
stderr=subprocess.PIPE,
80-
cwd=index.repo.working_dir,
81-
close_fds=is_posix)
77+
env=env,
78+
stdout=subprocess.PIPE,
79+
stderr=subprocess.PIPE,
80+
cwd=index.repo.working_dir,
81+
close_fds=is_posix)
8282
except Exception as ex:
8383
raise HookExecutionError(hp, ex)
8484
else:

‎git/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')

‎git/test/test_git.py‎

Lines changed: 54 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
# the BSD License: http://www.opensource.org/licenses/bsd-license.php
77
from __future__ import print_function
88

9-
import contextlib
9+
import contextlib2 as contextlib
10+
import ddt
1011
import os
1112
import shutil
1213
import subprocess
@@ -31,10 +32,11 @@
3132
assert_equal,
3233
assert_true,
3334
assert_match,
34-
fixture_path
35+
fixture_path,
36+
with_rw_directory,
3537
)
36-
from git.test.lib import with_rw_directory
37-
from git.util import finalize_process
38+
39+
from git.util import finalize_process, cwd
3840

3941
import os.path as osp
4042

@@ -51,6 +53,7 @@
5153

5254
from git.compat import is_win
5355

56+
5457
@contextlib.contextmanager
5558
def _chdir(new_dir):
5659
"""Context manager to temporarily change directory. Not reentrant."""
@@ -77,7 +80,7 @@ def _patch_out_env(name):
7780
os.environ[name] = old_value
7881

7982

80-
83+
@ddt.ddt
8184
class TestGit(TestBase):
8285

8386
@classmethod
@@ -137,23 +140,46 @@ def test_it_transforms_kwargs_into_git_command_arguments(self):
137140
def test_it_executes_git_to_shell_and_returns_result(self):
138141
assert_match(r'^git version [\d\.]{2}.*$', self.git.execute(["git", "version"]))
139142

140-
def test_it_executes_git_not_from_cwd(self):
141-
with TemporaryDirectory() as tmpdir:
142-
if is_win:
143-
# Copy an actual binary executable that is not git.
144-
other_exe_path = os.path.join(os.getenv("WINDIR"), "system32", "hostname.exe")
145-
impostor_path = os.path.join(tmpdir, "git.exe")
146-
shutil.copy(other_exe_path, impostor_path)
147-
else:
148-
# Create a shell script that doesn't do anything.
149-
impostor_path = os.path.join(tmpdir, "git")
150-
with open(impostor_path, mode="w") as file:
151-
print("#!/bin/sh", file=file)
152-
os.chmod(impostor_path, 0o755)
153-
154-
with _chdir(tmpdir):
155-
# six.assertRegex(self.git.execute(["git", "version"]).encode("UTF-8"), r"^git version\b")
156-
self.assertRegexpMatches(self.git.execute(["git", "version"]), r"^git version\b")
143+
@ddt.data(
144+
# chdir_to_repo, shell, command, use_shell_impostor
145+
(False, False, ["git", "version"], False),
146+
(False, True, "git version", False),
147+
(False, True, "git version", True),
148+
(True, False, ["git", "version"], False),
149+
(True, True, "git version", False),
150+
(True, True, "git version", True),
151+
)
152+
def test_it_executes_git_not_from_cwd(self, case):
153+
self.internal_it_executes_git_not_from_cwd(case)
154+
155+
@with_rw_directory
156+
def internal_it_executes_git_not_from_cwd(self, rw_dir, case):
157+
chdir_to_repo, shell, command, use_shell_impostor = case
158+
repo = Repo.init(rw_dir)
159+
if os.name == "nt":
160+
# Copy an actual binary executable that is not git. (On Windows, running
161+
# "hostname" only displays the hostname, it never tries to change it.)
162+
other_exe_path = Path(os.environ["SystemRoot"], "system32", "hostname.exe")
163+
impostor_path = Path(rw_dir, "git.exe")
164+
shutil.copy(other_exe_path, str(impostor_path))
165+
else:
166+
# Create a shell script that doesn't do anything.
167+
impostor_path = Path(rw_dir, "git")
168+
impostor_path.write_text(u"#!/bin/sh\n", encoding="utf-8")
169+
os.chmod(str(impostor_path), 0o755)
170+
if use_shell_impostor:
171+
shell_name = "cmd.exe" if os.name == "nt" else "sh"
172+
shutil.copy(str(impostor_path), str(Path(rw_dir, shell_name)))
173+
with contextlib.ExitStack() as stack:
174+
if chdir_to_repo:
175+
stack.enter_context(cwd(rw_dir))
176+
if use_shell_impostor:
177+
stack.enter_context(_patch_out_env("ComSpec"))
178+
# Run the command without raising an exception on failure, as the exception
179+
# message is currently misleading when the command is a string rather than a
180+
# sequence of strings (it really runs "git", but then wrongly reports "g").
181+
output = repo.git.execute(command, with_exceptions=False, shell=shell)
182+
self.assertRegexpMatches(output, r"^git version\b")
157183

158184
def test_it_accepts_stdin(self):
159185
filename = fixture_path("cat_file_blob")
@@ -325,7 +351,7 @@ def test_environment(self, rw_dir):
325351
self.assertIn('FOO', str(err))
326352

327353
def test_handle_process_output(self):
328-
from git.cmd import handle_process_output
354+
from git.cmd import handle_process_output, safer_popen
329355

330356
line_count = 5002
331357
count = [None, 0, 0]
@@ -337,13 +363,11 @@ def counter_stderr(line):
337363
count[2] += 1
338364

339365
cmdline = [sys.executable, fixture_path('cat_file.py'), str(fixture_path('issue-301_stderr'))]
340-
proc = subprocess.Popen(cmdline,
341-
stdin=None,
342-
stdout=subprocess.PIPE,
343-
stderr=subprocess.PIPE,
344-
shell=False,
345-
# creationflags=cmd.PROC_CREATIONFLAGS,
346-
)
366+
proc = safer_popen(cmdline,
367+
stdin=None,
368+
stdout=subprocess.PIPE,
369+
stderr=subprocess.PIPE,
370+
shell=False)
347371

348372
handle_process_output(proc, counter_stdout, counter_stderr, finalize_process)
349373

0 commit comments

Comments
 (0)
X Tutup