Repository navigation
Commit 2b47933
committed
Correct the "not from cwd" test and add more cases
This shows that CVE-2023-40590 is only partially patched. This
commit does not fix the vulnerbility, only the test.
The problem is that, when shell=True, the environment of the shell
subprocess, instead of the GitPython process, determines whether
searching for the "git" command will use the current directory.
Currently NoDefaultCurrentDirectoryInExePath is set only in the
current process, and this is done after the environment for the
subprocess (env) is computed. So when git is an indirect subprocess
due to shell=True, Windows still checks a CWD when looking it up.
(Note that this should not be a problem for indirect subprocesses
started by git itself. When a standard git command is implemented
as an external executable, when git runs a custom command, and when
one git command delegates some of its work to another -- e.g.
"git clone" running git-remote-https -- Git for Windows already
does not search the current directory. Custom git commands that
start their own git subprocesses could have an analogous path
search bug, but this would be separate from the bug in GitPython.)
This is an exploitable vulnerability in GitPython. Although
shell=True is rarer and inherently less secure than the default of
shell=False, it still ought to avoid automatically running an
executable that may exist only due to having been cloned as part of
an untrusted repository. In addition, historically programs on
Windows had sometimes used shell=True as a workaround for console
windows being displayed for subprocesses, and some such code may
still be in use.
Furthermore, even when GitPython's current working directory is
outside the repository being worked on, the Git object in a Repo
instance's "git" attribute holds the repository directory as its
"_working_dir" attribute, which Git.execute consults to determine
the value of "cwd" to pass to subprocess.Popen. When the created
direct subprocess is really a shell, this is the CWD where git.exe
is looked for before searching PATH directories.
This is also why previous, more limited testing (including
accompanying manual testing with shell=True) didn't discover the
bug. Even when modified to test with shell=True, the old test had
the "impostor" git.exe in the GitPython process's own current
directory (which was changed to a temporary directory for the test,
where the "impostor" was created, but this was separate from the
working tree of the self.git repository). This was effective for
the shell=False case, but not the shell=True case where the
impostor would be found and run in the repository directory even
when it differs from the GitPython process's CWD.1 parent 1c65efb commit 2b47933
2 files changed
Lines changed: 32 additions & 23 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -88,11 +88,11 @@ def with_rw_directory(func): | |||
| 88 | 88 | test succeeds, but leave it otherwise to aid additional debugging.""" | |
| 89 | 89 | ||
| 90 | 90 | @wraps(func) | |
| 91 | - def wrapper(self): | ||
| 91 | + def wrapper(self, *args, **kwargs): | ||
| 92 | 92 | path = tempfile.mkdtemp(prefix=func.__name__) | |
| 93 | 93 | keep = False | |
| 94 | 94 | try: | |
| 95 | - return func(self, path) | ||
| 95 | + return func(self, path, *args, **kwargs) | ||
| 96 | 96 | except Exception: | |
| 97 | 97 | log.info( | |
| 98 | 98 | "Test %s.%s failed, output is at %r\n", | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -3,6 +3,7 @@ | |||
| 3 | 3 | # This module is part of GitPython and is released under the | |
| 4 | 4 | # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ | |
| 5 | 5 | ||
| 6 | + import contextlib | ||
| 6 | 7 | import gc | |
| 7 | 8 | import inspect | |
| 8 | 9 | import logging | |
@@ -12,7 +13,7 @@ | |||
| 12 | 13 | import shutil | |
| 13 | 14 | import subprocess | |
| 14 | 15 | import sys | |
| 15 | - from tempfile import TemporaryDirectory, TemporaryFile | ||
| 16 | + from tempfile import TemporaryFile | ||
| 16 | 17 | from unittest import skipUnless | |
| 17 | 18 | ||
| 18 | 19 | if sys.version_info >= (3, 8): | |
@@ -135,27 +136,35 @@ def test_it_executes_git_and_returns_result(self): | |||
| 135 | 136 | self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") | |
| 136 | 137 | ||
| 137 | 138 | @ddt.data( | |
| 138 | - (["git", "version"], False), | ||
| 139 | - ("git version", True), | ||
| 139 | + (False, False, ["git", "version"]), | ||
| 140 | + (False, True, "git version"), | ||
| 141 | + (True, False, ["git", "version"]), | ||
| 142 | + (True, True, "git version"), | ||
| 140 | 143 | ) | |
| 141 | - def test_it_executes_git_not_from_cwd(self, case): | ||
| 142 | - command, shell = case | ||
| 143 | - | ||
| 144 | - with TemporaryDirectory() as tmpdir: | ||
| 145 | - if os.name == "nt": | ||
| 146 | - # Copy an actual binary executable that is not git. | ||
| 147 | - other_exe_path = os.path.join(os.getenv("WINDIR"), "system32", "hostname.exe") | ||
| 148 | - impostor_path = os.path.join(tmpdir, "git.exe") | ||
| 149 | - shutil.copy(other_exe_path, impostor_path) | ||
| 150 | - else: | ||
| 151 | - # Create a shell script that doesn't do anything. | ||
| 152 | - impostor_path = os.path.join(tmpdir, "git") | ||
| 153 | - with open(impostor_path, mode="w", encoding="utf-8") as file: | ||
| 154 | - print("#!/bin/sh", file=file) | ||
| 155 | - os.chmod(impostor_path, 0o755) | ||
| 156 | - | ||
| 157 | - with cwd(tmpdir): | ||
| 158 | - output = self.git.execute(command, shell=shell) | ||
| 144 | + @with_rw_directory | ||
| 145 | + def test_it_executes_git_not_from_cwd(self, rw_dir, case): | ||
| 146 | + chdir_to_repo, shell, command = case | ||
| 147 | + | ||
| 148 | + repo = Repo.init(rw_dir) | ||
| 149 | + | ||
| 150 | + if os.name == "nt": | ||
| 151 | + # Copy an actual binary executable that is not git. (On Windows, running | ||
| 152 | + # "hostname" only displays the hostname, it never tries to change it.) | ||
| 153 | + other_exe_path = os.path.join(os.getenv("WINDIR"), "system32", "hostname.exe") | ||
| 154 | + impostor_path = os.path.join(rw_dir, "git.exe") | ||
| 155 | + shutil.copy(other_exe_path, impostor_path) | ||
| 156 | + else: | ||
| 157 | + # Create a shell script that doesn't do anything. | ||
| 158 | + impostor_path = os.path.join(rw_dir, "git") | ||
| 159 | + with open(impostor_path, mode="w", encoding="utf-8") as file: | ||
| 160 | + print("#!/bin/sh", file=file) | ||
| 161 | + os.chmod(impostor_path, 0o755) | ||
| 162 | + | ||
| 163 | + with cwd(rw_dir) if chdir_to_repo else contextlib.nullcontext(): | ||
| 164 | + # Run the command without raising an exception on failure, as the exception | ||
| 165 | + # message is currently misleading when the command is a string rather than a | ||
| 166 | + # sequence of strings (it really runs "git", but then wrongly reports "g"). | ||
| 167 | + output = repo.git.execute(command, with_exceptions=False, shell=shell) | ||
| 159 | 168 | ||
| 160 | 169 | self.assertRegex(output, r"^git version\b") | |
| 161 | 170 | ||
0 commit comments