Merge pull request #2204 from gitpython-developers/security-fixes

Harden config and Git option validation

Author: Byron

git/cmd.py

@@ -654,6 +654,12 @@ class Git(metaclass=_GitMeta):
         "--upload-pack",
     ]
 
+    unsafe_git_pathspec_from_file_options = [
+        # Reads pathspecs from a caller-controlled file. Some commands include an
+        # unmatched pathspec in their error output, which can disclose the file.
+        "--pathspec-from-file",
+    ]
+
     def __getstate__(self) -> Dict[str, Any]:
         return slots_to_dict(self, exclude=self._excluded_)
 
@@ -1044,13 +1050,21 @@ def _option_candidates(cls, args: Sequence[Any] = (), kwargs: Optional[Mapping[s
                 values = value if isinstance(value, (list, tuple)) else (value,)
                 if any(value is True or (value is not False and value is not None) for value in values):
                     key = str(key)
-                    options.append(f"-{key}" if len(key) == 1 else f"--{dashify(key)}")
-                    if len(key) == 1 and split_single_char_options:
+                    if len(key) != 1:
+                        options.append(f"--{dashify(key)}")
+                    elif split_single_char_options:
+                        options.append(f"-{key}")
                         options.extend(
                             str(value)
                             for value in values
                             if value is not True and value not in (False, None) and str(value).startswith("-")
                         )
+                    else:
+                        options.extend(
+                            f"-{key}" if value is True else f"-{key}{value}"
+                            for value in values
+                            if value is True or (value is not False and value is not None)
+                        )
         return options
 
     AutoInterrupt: TypeAlias = _AutoInterrupt

git/config.py

@@ -75,6 +75,9 @@
 UNSAFE_CONFIG_CHARS_RE = re.compile(r"[\r\n\x00]")
 """Characters that cannot be safely written in config names or values."""
 
+VALID_CONFIG_OPTION_NAME_RE = re.compile(r"^[A-Za-z0-9_.-]+$")
+"""Pattern for option names that can be written without changing config syntax."""
+
 
 class MetaParserBuilder(abc.ABCMeta):  # noqa: B024
     """Utility class wrapping base-class methods into decorators that assure read-only
@@ -900,6 +903,8 @@ def _value_to_string_safe(self, value: Union[str, bytes, int, float, bool]) -> s
     def _assure_config_name_safe(self, name: "cp._SectionName", label: str) -> None:
         if isinstance(name, str) and UNSAFE_CONFIG_CHARS_RE.search(name):
             raise ValueError("Git config %s names must not contain CR, LF, or NUL" % label)
+        if label == "option" and isinstance(name, str) and not VALID_CONFIG_OPTION_NAME_RE.fullmatch(name):
+            raise ValueError("Git config option names may contain only letters, digits, '-', '_', or '.'")
         if label == "section" and isinstance(name, str):
             in_quotes = False
             escaped = False

git/index/base.py

@@ -134,6 +134,7 @@ class IndexFile(LazyMixin, git_diff.Diffable, Serializable):
     """
 
     unsafe_git_checkout_index_options = ["--prefix"]
+    unsafe_git_read_tree_options = ["--index-output"]
 
     __slots__ = ("repo", "version", "entries", "_extension_data", "_file_path")
 
@@ -259,7 +260,12 @@ def write(
 
     @post_clear_cache
     @default_index
-    def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexFile":
+    def merge_tree(
+        self,
+        rhs: Treeish,
+        base: Union[None, Treeish] = None,
+        allow_unsafe_options: bool = False,
+    ) -> "IndexFile":
         """Merge the given `rhs` treeish into the current index, possibly taking
         a common base treeish into account.
 
@@ -273,6 +279,9 @@ def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexF
             Optional treeish reference pointing to the common base of `rhs` and this
             index which equals lhs.
 
+        :param allow_unsafe_options:
+            Allow options that may write to arbitrary paths.
+
         :return:
             self (containing the merge and possibly unmerged entries in case of
             conflicts)
@@ -283,6 +292,12 @@ def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexF
             yourself, you have to commit the changed index (or make a valid tree from
             it) and retry with a three-way :meth:`index.from_tree <from_tree>` call.
         """
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(
+                options=Git._option_candidates([base, rhs]),
+                unsafe_options=self.unsafe_git_read_tree_options,
+            )
+
         # -i : ignore working tree status
         # --aggressive : handle more merge cases
         # -m : do an actual merge
@@ -327,7 +342,13 @@ def new(cls, repo: "Repo", *tree_sha: Union[str, Tree]) -> "IndexFile":
         return inst
 
     @classmethod
-    def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile":
+    def from_tree(
+        cls,
+        repo: "Repo",
+        *treeish: Treeish,
+        allow_unsafe_options: bool = False,
+        **kwargs: Any,
+    ) -> "IndexFile":
         R"""Merge the given treeish revisions into a new index which is returned.
         The original index will remain unaltered.
 
@@ -351,6 +372,9 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
         :param kwargs:
             Additional arguments passed to :manpage:`git-read-tree(1)`.
 
+        :param allow_unsafe_options:
+            Allow options that may write to arbitrary paths.
+
         :return:
             New :class:`IndexFile` instance. It will point to a temporary index location
             which does not exist anymore. If you intend to write such a merged Index,
@@ -368,6 +392,12 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
         if len(treeish) == 0 or len(treeish) > 3:
             raise ValueError("Please specify between 1 and 3 treeish, got %i" % len(treeish))
 
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(
+                options=Git._option_candidates(treeish, kwargs),
+                unsafe_options=cls.unsafe_git_read_tree_options,
+            )
+
         arg_list: List[Union[Treeish, str]] = []
         # Ignore that the working tree and index possibly are out of date.
         if len(treeish) > 1:
@@ -994,6 +1024,7 @@ def remove(
         self,
         items: Union[PathLike, Sequence[Union[PathLike, Blob, BaseIndexEntry, "Submodule"]]],
         working_tree: bool = False,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> List[str]:
         R"""Remove the given items from the index and optionally from the working tree
@@ -1024,6 +1055,10 @@ def remove(
             physically removing the respective file. This may fail if there are
             uncommitted changes in it.
 
+        :param allow_unsafe_options:
+            Allow unsafe options such as ``--pathspec-from-file`` to be passed to
+            :manpage:`git-rm(1)`.
+
         :param kwargs:
             Additional keyword arguments to be passed to :manpage:`git-rm(1)`, such as
             ``r`` to allow recursive removal.
@@ -1035,6 +1070,11 @@ def remove(
             This is interesting to know in case you have provided a directory or globs.
             Paths are relative to the repository.
         """
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(
+                options=Git._option_candidates([], kwargs),
+                unsafe_options=Git.unsafe_git_pathspec_from_file_options,
+            )
         args = []
         if not working_tree:
             args.append("--cached")
@@ -1416,6 +1456,7 @@ def reset(
         working_tree: bool = False,
         paths: Union[None, Iterable[PathLike]] = None,
         head: bool = False,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> "IndexFile":
         """Reset the index to reflect the tree at the given commit. This will not adjust
@@ -1447,6 +1488,9 @@ def reset(
             The paths need to exist at the commit, otherwise an exception will be
             raised.
 
+        :param allow_unsafe_options:
+            Allow options that may write to arbitrary paths.
+
         :param kwargs:
             Additional keyword arguments passed to :manpage:`git-reset(1)`.
 
@@ -1463,7 +1507,7 @@ def reset(
         """
         # What we actually want to do is to merge the tree into our existing index,
         # which is what git-read-tree does.
-        new_inst = type(self).from_tree(self.repo, commit)
+        new_inst = type(self).from_tree(self.repo, commit, allow_unsafe_options=allow_unsafe_options)
         if not paths:
             self.entries = new_inst.entries
         else:

git/refs/head.py

@@ -19,6 +19,7 @@
 
 from typing import Any, Sequence, TYPE_CHECKING, Union
 
+from git.cmd import Git
 from git.types import Commit_ish, PathLike
 
 if TYPE_CHECKING:
@@ -62,6 +63,7 @@ def reset(
         index: bool = True,
         working_tree: bool = False,
         paths: Union[PathLike, Sequence[PathLike], None] = None,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> "HEAD":
         """Reset our HEAD to the given commit optionally synchronizing the index and
@@ -84,12 +86,21 @@ def reset(
             Single path or list of paths relative to the git root directory
             that are to be reset. This allows to partially reset individual files.
 
+        :param allow_unsafe_options:
+            Allow unsafe options such as ``--pathspec-from-file`` to be passed to
+            :manpage:`git-reset(1)`.
+
         :param kwargs:
             Additional arguments passed to :manpage:`git-reset(1)`.
 
         :return:
             self
         """
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(
+                options=Git._option_candidates([commit], kwargs),
+                unsafe_options=Git.unsafe_git_pathspec_from_file_options,
+            )
         mode: Union[str, None]
         mode = "--soft"
         if index:
@@ -234,7 +245,12 @@ def rename(self, new_path: PathLike, force: bool = False) -> "Head":
         self.path = "%s/%s" % (self._common_path_default, new_path)
         return self
 
-    def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]:
+    def checkout(
+        self,
+        force: bool = False,
+        allow_unsafe_options: bool = False,
+        **kwargs: Any,
+    ) -> Union["HEAD", "Head"]:
         """Check out this head by setting the HEAD to this reference, by updating the
         index to reflect the tree we point to and by updating the working tree to
         reflect the latest index.
@@ -246,6 +262,10 @@ def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]:
             If ``False``, :exc:`~git.exc.GitCommandError` will be raised in that
             situation.
 
+        :param allow_unsafe_options:
+            Allow unsafe options such as ``--pathspec-from-file`` to be passed to
+            :manpage:`git-checkout(1)`.
+
         :param kwargs:
             Additional keyword arguments to be passed to git checkout, e.g.
             ``b="new_branch"`` to create a new branch at the given spot.
@@ -261,6 +281,11 @@ def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]:
             the HEAD detached which is allowed and possible, but remains a special state
             that some tools might not be able to handle.
         """
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(
+                options=Git._option_candidates([], kwargs),
+                unsafe_options=Git.unsafe_git_pathspec_from_file_options,
+            )
         kwargs["f"] = force
         if kwargs["f"] is False:
             kwargs.pop("f")

git/repo/base.py

@@ -142,6 +142,14 @@ class Repo:
     re_author_committer_start = re.compile(r"^(author|committer)")
     re_tab_full_line = re.compile(r"^\t(.*)$")
 
+    unsafe_git_init_options = [
+        # Can install hooks that execute during later Git commands:
+        "--template",
+        # Redirects the repository metadata to a caller-controlled path:
+        "--separate-git-dir",
+    ]
+    """Options to :manpage:`git-init(1)` that permit unsafe code execution or I/O."""
+
     unsafe_git_clone_options = [
         # Executes arbitrary commands:
         "--upload-pack",
@@ -1398,6 +1406,7 @@ def init(
         mkdir: bool = True,
         odbt: Type[GitCmdObjectDB] = GitCmdObjectDB,
         expand_vars: bool = True,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> "Repo":
         """Initialize a git repository at the given path if specified.
@@ -1422,13 +1431,22 @@ def init(
             information disclosure, allowing attackers to access the contents of
             environment variables.
 
+        :param allow_unsafe_options:
+            Allow unsafe options to be used, such as ``--template`` and
+            ``--separate-git-dir``.
+
         :param kwargs:
             Keyword arguments serving as additional options to the
             :manpage:`git-init(1)` command.
 
         :return:
             :class:`Repo` (the newly created repo)
         """
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(
+                options=Git._option_candidates([], kwargs),
+                unsafe_options=cls.unsafe_git_init_options,
+            )
         if path:
             path = expand_path(path, expand_vars)
         if mkdir and path and not osp.exists(path):

test/test_config.py

@@ -194,6 +194,43 @@ def test_set_value_rejects_unsafe_section_and_option_names(self, rw_dir):
             self.assertEqual(git_config.get_value("user", "name"), "safe")
             self.assertFalse(git_config.has_section("core"))
 
+    @with_rw_directory
+    def test_writer_rejects_invalid_option_names(self, rw_dir):
+        config_path = osp.join(rw_dir, "config")
+        bad_options = (
+            "name=value",
+            "name#comment",
+            "name;comment",
+            "name with space",
+            "name\twith-tab",
+            "name[section",
+            "name]section",
+            "name:colon",
+            'name"quote',
+            "name\\escape",
+        )
+
+        with GitConfigParser(config_path, read_only=False) as git_config:
+            git_config.add_section("user")
+            for bad_option in bad_options:
+                with pytest.raises(ValueError, match="option name"):
+                    git_config.set("user", bad_option, "unsafe")
+                with pytest.raises(ValueError, match="option name"):
+                    git_config.set_value("user", bad_option, "unsafe")
+                with pytest.raises(ValueError, match="option name"):
+                    git_config.add_value("user", bad_option, "unsafe")
+
+            git_config.set_value("user", "safe-option1", "safe")
+            git_config.set_value("user", "safe_option2", "safe")
+            git_config.set_value("user", "3safe_option", "safe")
+            git_config.set_value("user", "safe.option3", "safe")
+
+        with GitConfigParser(config_path, read_only=True) as git_config:
+            self.assertEqual(git_config.get_value("user", "safe-option1"), "safe")
+            self.assertEqual(git_config.get_value("user", "safe_option2"), "safe")
+            self.assertEqual(git_config.get_value("user", "3safe_option"), "safe")
+            self.assertEqual(git_config.get_value("user", "safe.option3"), "safe")
+
     @with_rw_directory
     def test_writer_rejects_unquoted_section_terminators(self, rw_dir):
         config_path = osp.join(rw_dir, "config")

test/test_git.py

@@ -215,6 +215,18 @@ def test_option_candidates_ignore_untransformed_kwargs(self):
 
         self.assertEqual(options, ["--max-count"])
 
+    def test_option_candidates_include_falsey_non_boolean_values(self):
+        kwargs = {"pathspec_from_file": 0}
+        candidates = Git._option_candidates(kwargs=kwargs)
+
+        self.assertEqual(candidates, ["--pathspec-from-file"])
+        self.assertEqual(self.git.transform_kwargs(**kwargs), ["--pathspec-from-file=0"])
+        with self.assertRaises(UnsafeOptionError):
+            Git.check_unsafe_options(
+                options=candidates,
+                unsafe_options=Git.unsafe_git_pathspec_from_file_options,
+            )
+
     def test_option_candidates_include_split_single_char_option_values(self):
         cases = [
             ({"n": "--upload-pack=helper"}, ["-n", "--upload-pack=helper"], ["--upload-pack"]),
@@ -230,7 +242,15 @@ def test_option_candidates_include_split_single_char_option_values(self):
 
         unsplit_kwargs = {"n": "--upload-pack=helper", "split_single_char_options": False}
         self.assertEqual(self.git.transform_kwargs(**unsplit_kwargs), ["-n--upload-pack=helper"])
-        self.assertEqual(Git._option_candidates(kwargs=unsplit_kwargs), ["-n"])
+        self.assertEqual(Git._option_candidates(kwargs=unsplit_kwargs), ["-n--upload-pack=helper"])
+
+    def test_option_candidates_include_joined_single_char_option_values(self):
+        kwargs = {"n": "uhelper", "split_single_char_options": False}
+        candidates = Git._option_candidates(kwargs=kwargs)
+
+        self.assertEqual(candidates, ["-nuhelper"])
+        with self.assertRaises(UnsafeOptionError):
+            Git.check_unsafe_options(options=candidates, unsafe_options=["-u"])
 
     _shell_cases = (
         # value_in_call, value_from_class, expected_popen_arg