Block file-reading Git options

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
 ======
 

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)

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]] = []

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.

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"]

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: [<line>]]
@@ -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] = {}

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}")

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")
 

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):

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):