diff --git a/doc/source/changes.rst b/doc/source/changes.rst index ffddf56a1..714cc7ffc 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,20 @@ 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 + +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/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/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..583e96ca4 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -199,6 +199,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 +1162,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 +1174,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 +1268,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 +1284,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_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..9a3f58c7b 100644 --- a/test/test_refs.py +++ b/test/test_refs.py @@ -70,6 +70,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):