Skip to content

Commit

Permalink
Merge pull request #1636 from EliahKagan/cve-2023-40590
Browse files Browse the repository at this point in the history
  • Loading branch information
Byron committed Sep 1, 2023
2 parents e19abe7 + 7611cd9 commit 8b75434
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 18 deletions.
38 changes: 21 additions & 17 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# the BSD License: http://www.opensource.org/licenses/bsd-license.php
from __future__ import annotations
import re
from contextlib import contextmanager
import contextlib
import io
import logging
import os
Expand All @@ -14,6 +14,7 @@
import subprocess
import threading
from textwrap import dedent
import unittest.mock

from git.compat import (
defenc,
Expand Down Expand Up @@ -963,8 +964,11 @@ def execute(
redacted_command,
'"kill_after_timeout" feature is not supported on Windows.',
)
# Only search PATH, not CWD. This must be in the *caller* environment. The "1" can be any value.
patch_caller_env = unittest.mock.patch.dict(os.environ, {"NoDefaultCurrentDirectoryInExePath": "1"})
else:
cmd_not_found_exception = FileNotFoundError # NOQA # exists, flake8 unknown @UndefinedVariable
patch_caller_env = contextlib.nullcontext()
# end handle

stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
Expand All @@ -980,21 +984,21 @@ def execute(
istream_ok,
)
try:
proc = Popen(
command,
env=env,
cwd=cwd,
bufsize=-1,
stdin=istream or DEVNULL,
stderr=PIPE,
stdout=stdout_sink,
shell=shell is not None and shell or self.USE_SHELL,
close_fds=is_posix, # unsupported on windows
universal_newlines=universal_newlines,
creationflags=PROC_CREATIONFLAGS,
**subprocess_kwargs,
)

with patch_caller_env:
proc = Popen(
command,
env=env,
cwd=cwd,
bufsize=-1,
stdin=istream or DEVNULL,
stderr=PIPE,
stdout=stdout_sink,
shell=shell is not None and shell or self.USE_SHELL,
close_fds=is_posix, # unsupported on windows
universal_newlines=universal_newlines,
creationflags=PROC_CREATIONFLAGS,
**subprocess_kwargs,
)
except cmd_not_found_exception as err:
raise GitCommandNotFound(redacted_command, err) from err
else:
Expand Down Expand Up @@ -1144,7 +1148,7 @@ def update_environment(self, **kwargs: Any) -> Dict[str, Union[str, None]]:
del self._environment[key]
return old_env

@contextmanager
@contextlib.contextmanager
def custom_environment(self, **kwargs: Any) -> Iterator[None]:
"""
A context manager around the above ``update_environment`` method to restore the
Expand Down
32 changes: 31 additions & 1 deletion test/test_git.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
#
# This module is part of GitPython and is released under
# the BSD License: http://www.opensource.org/licenses/bsd-license.php
import contextlib
import os
import shutil
import subprocess
import sys
from tempfile import TemporaryFile
from tempfile import TemporaryDirectory, TemporaryFile
from unittest import mock

from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd
Expand All @@ -20,6 +22,17 @@
from git.compat import is_win


@contextlib.contextmanager
def _chdir(new_dir):
"""Context manager to temporarily change directory. Not reentrant."""
old_dir = os.getcwd()
os.chdir(new_dir)
try:
yield
finally:
os.chdir(old_dir)


class TestGit(TestBase):
@classmethod
def setUpClass(cls):
Expand Down Expand Up @@ -75,6 +88,23 @@ def test_it_transforms_kwargs_into_git_command_arguments(self):
def test_it_executes_git_to_shell_and_returns_result(self):
self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$")

def test_it_executes_git_not_from_cwd(self):
with TemporaryDirectory() as tmpdir:
if is_win:
# Copy an actual binary executable that is not git.
other_exe_path = os.path.join(os.getenv("WINDIR"), "system32", "hostname.exe")
impostor_path = os.path.join(tmpdir, "git.exe")
shutil.copy(other_exe_path, impostor_path)
else:
# Create a shell script that doesn't do anything.
impostor_path = os.path.join(tmpdir, "git")
with open(impostor_path, mode="w", encoding="utf-8") as file:
print("#!/bin/sh", file=file)
os.chmod(impostor_path, 0o755)

with _chdir(tmpdir):
self.assertRegex(self.git.execute(["git", "version"]), r"^git version\b")

This comment has been minimized.

Copy link
@prabhu

prabhu Sep 19, 2023

So if the imposter binary supports a version sub-command and returns a string beginning with "git version", it would be considered safe?

This comment has been minimized.

Copy link
@Byron

Byron Sep 20, 2023

Author Member

This validates that indeed git.exe isn't executed from the current working directory, but instead from PATH. The test validates that git is executed.
Preventing imposters that are located in PATH isn't anything this PR is solving or considering an issue. The reason executing git.exe from the CWD is that these files might be distributed with git repositories, which are typically under control of a third party.


def test_it_accepts_stdin(self):
filename = fixture_path("cat_file_blob")
with open(filename, "r") as fh:
Expand Down

0 comments on commit 8b75434

Please sign in to comment.