diff --git a/VERSION b/VERSION index dfabc766a..e5c812e68 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.1.58 +3.1.59 diff --git a/doc/source/changes.rst b/doc/source/changes.rst index ffddf56a1..1a1b8fa12 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,23 @@ Changelog ========= +3.1.59 +====== + +Security fixes for + +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-5xxx-qhh7-9287 +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-3wxw-xv34-2frg +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-8mcc-hrx5-hvxc +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-284h-m62q-gf8w +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-7833-fr7j-v32q + +If you can, also try and provide feedback on the upcoming v4 branch +https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. + +See the following for all changes. +https://github.com/gitpython-developers/GitPython/releases/tag/3.1.59 + 3.1.58 ====== diff --git a/git/cmd.py b/git/cmd.py index 03ecd13f5..193dfd4f6 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -652,6 +652,7 @@ class Git(metaclass=_GitMeta): unsafe_git_ls_remote_options = [ # This option allows arbitrary command execution in git-ls-remote. "--upload-pack", + "--exec", ] unsafe_git_pathspec_from_file_options = [ @@ -976,7 +977,9 @@ def _canonicalize_option_name(cls, option: str) -> str: return dashify(option_tokens[0]) @classmethod - def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> None: + def check_unsafe_options( + cls, options: List[str], unsafe_options: List[str], clusterable_short_options: str = "46flnqsv" + ) -> None: """Raise :class:`~git.exc.UnsafeOptionError` for blocked option spellings. In addition to exact matches, this rejects abbreviated long options accepted @@ -1011,7 +1014,7 @@ def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> # These value-less Git flags can be clustered before another short option # (for example, ``-fuVALUE``). Stop at any other character because it may # begin an attached value, as ``o`` does in the safe option ``-oupstream``. - clusterable_short_options = frozenset("46flnqsv") + clusterable_short_options_set = frozenset(clusterable_short_options) options_are_kwargs = all(not option.startswith("-") for option in options) for option in options: candidate = cls._canonicalize_option_name(option) @@ -1028,7 +1031,7 @@ def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> raise UnsafeOptionError( f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it." ) - if option_char not in clusterable_short_options: + if option_char not in clusterable_short_options_set: break if not (option.startswith("--") or (options_are_kwargs and len(candidate) > 1)): continue @@ -1133,7 +1136,7 @@ def ls_remote( """List references in a remote repository. :param allow_unsafe_options: - Allow unsafe options, like ``--upload-pack``. + Allow unsafe options, like ``--upload-pack`` or ``--exec``. """ if not allow_unsafe_options: candidate_options = self._option_candidates(args, kwargs) diff --git a/git/config.py b/git/config.py index c300de499..6f26e58fc 100644 --- a/git/config.py +++ b/git/config.py @@ -705,7 +705,11 @@ def write_section(name: str, section_dict: _OMD) -> None: continue for v in values: - fp.write(("\t%s = %s\n" % (key, self._value_to_string(v).replace("\n", "\n\t"))).encode(defenc)) + value = self._value_to_string(v) + if any(char in value for char in '\n\t\b\\"'): + value = value.replace("\\", "\\\\").replace('"', '\\"') + value = '"%s\\\n"' % value.replace("\n", "\\n").replace("\t", "\\t").replace("\b", "\\b") + fp.write(("\t%s = %s\n" % (key, value)).encode(defenc)) # END if key is not __name__ # END section writing diff --git a/git/db.py b/git/db.py index cacd030d0..bd68a5157 100644 --- a/git/db.py +++ b/git/db.py @@ -5,10 +5,14 @@ __all__ = ["GitCmdObjectDB", "GitDB"] -from gitdb.base import OInfo, OStream +from subprocess import PIPE + +from gitdb.base import IStream, OInfo, OStream from gitdb.db import GitDB, LooseObjectDB from gitdb.exc import BadObject +from gitdb.fun import stream_copy +from git.compat import force_text from git.util import bin_to_hex, hex_to_bin from git.exc import GitCommandError @@ -46,6 +50,25 @@ def stream(self, binsha: bytes) -> OStream: hexsha, typename, size, stream = self._git.stream_object_data(bin_to_hex(binsha)) return OStream(hex_to_bin(hexsha), typename, size, stream) + def store(self, istream: IStream) -> IStream: + """Store an object using git itself.""" + if istream.binsha is not None or self.ostream() is not None: + return super().store(istream) + + proc = self._git.hash_object( + "-t", force_text(istream.type), "-w", "--stdin", "--literally", as_process=True, istream=PIPE + ) + assert proc.stdin is not None + try: + stream_copy(istream.read, proc.stdin.write, istream.size, self.stream_chunk_size) + finally: + proc.stdin.close() + assert proc.stdout is not None + hexsha = proc.stdout.read().strip() + proc.wait() + istream.binsha = hex_to_bin(hexsha) + return istream + # { Interface def partial_to_complete_sha_hex(self, partial_hexsha: str) -> bytes: diff --git a/git/diff.py b/git/diff.py index 3628c815a..d1963b84f 100644 --- a/git/diff.py +++ b/git/diff.py @@ -220,8 +220,8 @@ def diff( to be read and diffed. :param allow_unsafe_options: - If ``True``, allow options such as ``--output`` that can write to arbitrary - filesystem paths. + If ``True``, allow options such as ``--output`` and ``-O`` that can write to + or read from arbitrary filesystem paths. :param kwargs: Additional arguments passed to :manpage:`git-diff(1)`, such as ``R=True`` to @@ -238,7 +238,8 @@ def diff( if not allow_unsafe_options: Git.check_unsafe_options( options=Git._option_candidates([other], kwargs), - unsafe_options=self.repo.unsafe_git_revision_options, + unsafe_options=self.repo.unsafe_git_diff_options, + clusterable_short_options="46abceflmnpqrstuvwzBCDMNRW", ) args: List[Union[PathLike, Diffable]] = [] diff --git a/git/index/base.py b/git/index/base.py index 0e7b5f918..a3c915242 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -1567,7 +1567,8 @@ def diff( if not allow_unsafe_options: Git.check_unsafe_options( options=Git._option_candidates([other], kwargs), - unsafe_options=self.repo.unsafe_git_revision_options, + unsafe_options=self.repo.unsafe_git_diff_options, + clusterable_short_options="46abceflmnpqrstuvwzBCDMNRW", ) # Only run if we are the default repository index. diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index da0e09af4..39e912321 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -9,6 +9,7 @@ import ntpath import os import os.path as osp +import shlex import stat import sys import uuid @@ -270,7 +271,7 @@ def _config_parser( raise ValueError("Cannot write blobs of 'historical' submodule configurations") # END handle writes of historical submodules - return SubmoduleConfigParser(fp_module, read_only=read_only) + return SubmoduleConfigParser(fp_module, read_only=read_only, merge_includes=False) def _clear_cache(self) -> None: """Clear the possibly changed values.""" @@ -363,6 +364,15 @@ def _clone_repo( module_abspath = cls._module_abspath(repo, path, name) module_checkout_path = module_abspath if cls._need_gitfile_submodules(repo.git): + if not allow_unsafe_options: + Git.check_unsafe_options(Git._option_candidates([], kwargs), repo.unsafe_git_clone_options) + multi_options = kwargs.get("multi_options") + if multi_options: + Git.check_unsafe_options( + shlex.split(" ".join(cast("Sequence[str]", multi_options))), + repo.unsafe_git_clone_options, + ) + allow_unsafe_options = True kwargs["separate_git_dir"] = module_abspath module_abspath_dir = osp.dirname(module_abspath) if not osp.isdir(module_abspath_dir): diff --git a/git/refs/tag.py b/git/refs/tag.py index 055722e3b..3a7d946c1 100644 --- a/git/refs/tag.py +++ b/git/refs/tag.py @@ -134,15 +134,17 @@ def create( :return: A new :class:`TagReference`. """ + legacy_ref = kwargs.pop("ref", None) + if legacy_ref: + reference = legacy_ref + if not allow_unsafe_options: Git.check_unsafe_options( - options=Git._option_candidates([], kwargs), + options=Git._option_candidates([path, reference], kwargs), unsafe_options=cls.unsafe_git_tag_options, + clusterable_short_options="46adefilnqsv", ) - if "ref" in kwargs and kwargs["ref"]: - reference = kwargs["ref"] - if "message" in kwargs and kwargs["message"]: kwargs["m"] = kwargs["message"] del kwargs["message"] diff --git a/git/repo/base.py b/git/repo/base.py index df61e2d28..d0ec00ec9 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -159,6 +159,8 @@ class Repo: "-c", # Can install hooks that execute during clone: "--template", + # Redirects the repository metadata to a caller-controlled path: + "--separate-git-dir", # Fetches from an additional caller-controlled URI: "--bundle-uri", ] @@ -199,6 +201,19 @@ class Repo: "-o", ] + unsafe_git_blame_options = unsafe_git_revision_options + [ + # These options read from arbitrary files and expose their contents through blame output. + "--contents", + "-S", + "--ignore-revs-file", + ] + + unsafe_git_diff_options = unsafe_git_revision_options + [ + # Reads caller-controlled order patterns from an arbitrary file. + "-O", + "--orderfile", + ] + # Invariants config_level: ConfigLevels_Tup = ("system", "user", "global", "repository") """Represents the configuration level of a configuration file.""" @@ -1149,7 +1164,7 @@ def blame_incremental( :manpage:`git-rev-parse(1)` is a valid option. :param allow_unsafe_options: - Allow unsafe options in revision argument, like ``--output``. + Allow unsafe options in revision argument, like ``--output`` or ``--contents``. :return: Lazy iterator of :class:`BlameEntry` tuples, where the commit indicates the @@ -1161,7 +1176,9 @@ def blame_incremental( """ if not allow_unsafe_options: Git.check_unsafe_options( - options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options + options=Git._option_candidates([rev], kwargs), + unsafe_options=self.unsafe_git_blame_options, + clusterable_short_options="46bceflnpqstvw", ) data: bytes = self.git.blame(rev, "--", file, p=True, incremental=True, stdout_as_string=False, **kwargs) @@ -1253,7 +1270,7 @@ def blame( :manpage:`git-rev-parse(1)` is a valid option. :param allow_unsafe_options: - Allow unsafe options in revision argument, like ``--output``. + Allow unsafe options in revision argument, like ``--output`` or ``--contents``. :return: list: [git.Commit, list: []] @@ -1269,7 +1286,8 @@ def blame( if not allow_unsafe_options: Git.check_unsafe_options( options=Git._option_candidates([rev, rev_opts_list], kwargs), - unsafe_options=self.unsafe_git_revision_options, + unsafe_options=self.unsafe_git_blame_options, + clusterable_short_options="46bceflnpqstvw", ) data: bytes = self.git.blame(rev, *rev_opts_list, "--", file, p=True, stdout_as_string=False, **kwargs) commits: Dict[str, Commit] = {} diff --git a/test/test_clone.py b/test/test_clone.py index 5af67613a..a6c3db6f9 100644 --- a/test/test_clone.py +++ b/test/test_clone.py @@ -133,6 +133,7 @@ def test_clone_unsafe_options(self, rw_repo): "-vcprotocol.ext.allow=always", f"--template={tmp_dir}", f"--bundle-uri=file://{tmp_dir}", + f"--separate-git-dir={tmp_dir / 'git-dir'}", ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): @@ -149,6 +150,7 @@ def test_clone_unsafe_options(self, rw_repo): {"c": "protocol.ext.allow=always"}, {"template": tmp_dir}, {"bundle_uri": f"file://{tmp_dir}"}, + {"separate_git_dir": tmp_dir / "git-dir"}, ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): @@ -258,6 +260,7 @@ def test_clone_from_unsafe_options(self, rw_repo): "-c protocol.ext.allow=always", "-cprotocol.ext.allow=always", "-vcprotocol.ext.allow=always", + f"--separate-git-dir={tmp_dir / 'git-dir'}", ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): @@ -270,6 +273,7 @@ def test_clone_from_unsafe_options(self, rw_repo): {"u": f"touch {tmp_file}"}, {"config": "protocol.ext.allow=always"}, {"c": "protocol.ext.allow=always"}, + {"separate_git_dir": tmp_dir / "git-dir"}, ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): diff --git a/test/test_config.py b/test/test_config.py index fd0d347a4..d664fdb6f 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -7,6 +7,7 @@ import io import os import os.path as osp +import subprocess import sys from unittest import mock @@ -15,7 +16,6 @@ from git import GitConfigParser from git.config import _OMD, cp from git.util import cwd, rmfile - from test.lib import SkipTest, TestCase, fixture_path, with_rw_directory _tc_lock_fpaths = osp.join(osp.dirname(__file__), "fixtures/*.lock") @@ -150,6 +150,46 @@ def test_config_value_with_trailing_new_line(self): git_config = GitConfigParser(config_file) git_config.read() # This should not throw an exception + @with_rw_directory + def test_rewriting_multiline_value_does_not_create_option(self, rw_dir): + config_path = osp.join(rw_dir, "config") + with open(config_path, "wb") as config_file: + config_file.write(b'[core]\n\tzzz = "A\\nhooksPath = ../evil-hooks\\\n"\n') + + with GitConfigParser(config_path, read_only=False) as git_config: + self.assertEqual(git_config.get_value("core", "zzz"), "A\nhooksPath = ../evil-hooks") + git_config.set_value("user", "name", "Test User") + + with GitConfigParser(config_path, read_only=True) as git_config: + self.assertEqual(git_config.get_value("core", "zzz"), "A\nhooksPath = ../evil-hooks") + self.assertFalse(git_config.has_option("core", "hooksPath")) + self.assertEqual( + subprocess.run(["git", "config", "--file", config_path, "--get", "core.hooksPath"]).returncode, 1 + ) + + @with_rw_directory + def test_writer_escapes_special_characters_without_newline(self, rw_dir): + config_path = osp.join(rw_dir, "config") + values = {"tab": "\tvalue\t", "backspace": "a\bb", "quote": 'a"b', "backslash": "a\\qb"} + + with GitConfigParser(config_path, read_only=False) as git_config: + for key, value in values.items(): + git_config.set_value("section", key, value) + + with GitConfigParser(config_path, read_only=True) as git_config: + for key, value in values.items(): + self.assertEqual(git_config.get_value("section", key), value) + self.assertEqual( + subprocess.run( + ["git", "config", "--file", config_path, "--get", "section.%s" % key], + stdout=subprocess.PIPE, + check=True, + ).stdout, + value.encode() + b"\n", + ) + with open(config_path, "rb") as config_file: + self.assertNotIn(b"\x08", config_file.read()) + @with_rw_directory def test_set_value_rejects_config_injection(self, rw_dir): config_path = osp.join(rw_dir, "config") diff --git a/test/test_db.py b/test/test_db.py index 72d63b44b..46580d84b 100644 --- a/test/test_db.py +++ b/test/test_db.py @@ -3,16 +3,30 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ +from io import BytesIO import os.path as osp +from unittest import mock + +from gitdb import IStream +from gitdb.db import LooseObjectDB +from gitdb.typ import str_blob_type from git.db import GitCmdObjectDB from git.exc import BadObject from git.util import bin_to_hex -from test.lib import TestBase +from test.lib import TestBase, with_rw_repo class TestDB(TestBase): + @with_rw_repo("HEAD") + def test_store_uses_hash_object(self, rw_repo): + data = b"hello world" + with mock.patch.object(LooseObjectDB, "store", side_effect=AssertionError("unexpected loose-object write")): + istream = rw_repo.odb.store(IStream(str_blob_type, len(data), BytesIO(data))) + + assert rw_repo.odb.stream(istream.binsha).read() == data + def test_base(self): gdb = GitCmdObjectDB(osp.join(self.rorepo.git_dir, "objects"), self.rorepo.git) diff --git a/test/test_diff.py b/test/test_diff.py index 7f2275f55..d5e14f3de 100644 --- a/test/test_diff.py +++ b/test/test_diff.py @@ -376,11 +376,25 @@ def test_diff_submodule(self): def test_diff_rejects_unsafe_output_options(self): commit = self.rorepo.head.commit + commit.diff(S="needle") + calls = ( lambda target: commit.diff(output=target), lambda target: commit.diff(other=f"--output={target}"), + lambda target: commit.diff(O=target), + lambda target: commit.diff(orderfile=target), + lambda target: commit.diff(other=f"--orderfile={target}"), + lambda target: commit.diff(other=f"-pO{target}"), + lambda target: commit.diff(other=f"-uO{target}"), + lambda target: commit.diff(other=f"-DO{target}"), lambda target: self.rorepo.index.diff(NULL_TREE, output=target), lambda target: self.rorepo.index.diff(f"--output={target}"), + lambda target: self.rorepo.index.diff(NULL_TREE, O=target), + lambda target: self.rorepo.index.diff(NULL_TREE, orderfile=target), + lambda target: self.rorepo.index.diff(f"--orderfile={target}"), + lambda target: self.rorepo.index.diff(f"-pO{target}"), + lambda target: self.rorepo.index.diff(f"-uO{target}"), + lambda target: self.rorepo.index.diff(f"-DO{target}"), ) for index, call in enumerate(calls): target = osp.join(self.repo_dir, f"diff-output-{index}") diff --git a/test/test_refs.py b/test/test_refs.py index a87134ab8..32c8fbe34 100644 --- a/test/test_refs.py +++ b/test/test_refs.py @@ -26,7 +26,7 @@ from git.exc import UnsafeOptionError from git.objects.tag import TagObject import git.refs as refs -from git.util import Actor +from git.util import Actor, rmtree from test.lib import TestBase, requires_symlinks, with_rw_repo, PathLikeMock @@ -43,6 +43,7 @@ def _repo_with_initial_commit(self, base_dir): yield repo finally: repo.git.clear_cache() + rmtree(repo_dir) def test_from_path(self): # Should be able to create any reference directly. @@ -70,6 +71,21 @@ def test_tag_create_rejects_unsafe_file_options(self, rw_repo): with self.assertRaises(UnsafeOptionError): TagReference.create(rw_repo, f"unsafe-{index}", **option) + for args in ( + ("unsafe-reference", f"--file={message.name}"), + (f"--file={message.name}", "HEAD"), + (f"-eF{message.name}", "HEAD"), + (f"-iF{message.name}", "HEAD"), + ): + with self.assertRaises(UnsafeOptionError): + TagReference.create(rw_repo, *args) + + with self.assertRaises(UnsafeOptionError): + TagReference.create(rw_repo, "unsafe-ref-kwarg", ref=f"--file={message.name}") + + tag = TagReference.create(rw_repo, "legacy-ref", ref="HEAD", allow_unsafe_options=True) + self.assertEqual(tag.commit, rw_repo.head.commit) + tag = TagReference.create(rw_repo, "allowed-file", F=message.name, allow_unsafe_options=True) self.assertEqual(tag.tag.message, "private tag message") diff --git a/test/test_remote.py b/test/test_remote.py index 505d283af..e1793214c 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -1035,6 +1035,7 @@ def test_ls_remote_unsafe_options(self, rw_repo): {"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}, {"upl": f"touch {tmp_file}"}, + {"exec": f"touch {tmp_file}"}, ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): @@ -1047,6 +1048,8 @@ def test_ls_remote_unsafe_options(self, rw_repo): rw_repo.git.ls_remote(f"--upload-pack={tmp_file}", ".") with self.assertRaises(UnsafeOptionError): rw_repo.git.ls_remote(f"--upl={tmp_file}", ".") + with self.assertRaises(UnsafeOptionError): + rw_repo.git.ls_remote(f"--exec={tmp_file}", ".") with self.assertRaises(UnsafeOptionError): rw_repo.git.ls_remote("--upload-pack", "touch", ".") with self.assertRaises(UnsafeOptionError): diff --git a/test/test_repo.py b/test/test_repo.py index 0c97041f9..1dfec951a 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -590,8 +590,11 @@ def test_blame_real(self): def test_blame_rejects_unsafe_revision(self): with tempfile.TemporaryDirectory() as tdir: output_marker = osp.join(tdir, "pwn") - with self.assertRaises(UnsafeOptionError): - self.rorepo.blame(f"--output={output_marker}", "README.md") + for option in ("--output", "--contents", "-S", "-wS", "--ignore-revs-file"): + with self.assertRaises(UnsafeOptionError): + self.rorepo.blame(f"{option}={output_marker}", "README.md") + with self.assertRaises(UnsafeOptionError): + list(self.rorepo.blame_incremental(f"{option}={output_marker}", "README.md")) assert not osp.exists(output_marker) def test_blame_rejects_unsafe_options(self): diff --git a/test/test_submodule.py b/test/test_submodule.py index 287986059..d545cc8d5 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -954,6 +954,7 @@ def test_update_rejects_parent_component_in_name(self, rwdir): source.working_tree_dir, osp.join(clone.working_tree_dir, "module"), separate_git_dir=osp.join(rwdir, "escaped", "module"), + allow_unsafe_options=True, ) with pytest.raises(ValueError, match="submodule name"): clone.submodules[0].update(init=True) @@ -1207,6 +1208,22 @@ def test_ignore_non_submodule_file(self, rwdir): assert len(parent.submodules) == 0 + @with_rw_directory + def test_gitmodules_does_not_merge_includes(self, rwdir): + parent = git.Repo.init(rwdir) + secret_path = osp.join(rwdir, "secret") + with open(secret_path, "w", encoding="utf-8") as secret: + secret.write("not git config\n") + with open(osp.join(rwdir, ".gitmodules"), "w", encoding="utf-8") as modules: + modules.write('[submodule "module"]\n') + modules.write("\tpath = module\n") + modules.write("\turl = https://example.com/module.git\n") + modules.write("[include]\n") + modules.write("\tpath = %s\n" % secret_path) + + parser = Submodule._config_parser(parent, None, read_only=True) + self.assertEqual(parser.get_value('submodule "module"', "path"), "module") + @with_rw_directory def test_remove_norefs(self, rwdir): parent = git.Repo.init(osp.join(rwdir, "parent"))