Merge pull request #1521 from stsewd/block-insecure-options

Block insecure options and protocols by default

AUTHORS

@@ -50,4 +50,5 @@ Contributors are:
 -Patrick Gerard
 -Luke Twist <itsluketwist@gmail.com>
 -Joseph Hale <me _at_ jhale.dev>
+-Santos Gallegos <stsewd _at_ proton.me>
 Portions derived from other open source works and are clearly marked.

git/cmd.py

@@ -4,6 +4,7 @@
 # This module is part of GitPython and is released under
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 from __future__ import annotations
+import re
 from contextlib import contextmanager
 import io
 import logging
@@ -24,7 +25,7 @@
 from git.exc import CommandError
 from git.util import is_cygwin_git, cygpath, expand_path, remove_password_if_present
 
-from .exc import GitCommandError, GitCommandNotFound
+from .exc import GitCommandError, GitCommandNotFound, UnsafeOptionError, UnsafeProtocolError
 from .util import (
     LazyMixin,
     stream_copy,
@@ -262,6 +263,8 @@ class Git(LazyMixin):
 
     _excluded_ = ("cat_file_all", "cat_file_header", "_version_info")
 
+    re_unsafe_protocol = re.compile("(.+)::.+")
+
     def __getstate__(self) -> Dict[str, Any]:
         return slots_to_dict(self, exclude=self._excluded_)
 
@@ -454,6 +457,48 @@ def polish_url(cls, url: str, is_cygwin: Union[None, bool] = None) -> PathLike:
             url = url.replace("\\\\", "\\").replace("\\", "/")
         return url
 
+    @classmethod
+    def check_unsafe_protocols(cls, url: str) -> None:
+        """
+        Check for unsafe protocols.
+
+        Apart from the usual protocols (http, git, ssh),
+        Git allows "remote helpers" that have the form `<transport>::<address>`,
+        one of these helpers (`ext::`) can be used to invoke any arbitrary command.
+
+        See:
+
+        - https://git-scm.com/docs/gitremote-helpers
+        - https://git-scm.com/docs/git-remote-ext
+        """
+        match = cls.re_unsafe_protocol.match(url)
+        if match:
+            protocol = match.group(1)
+            raise UnsafeProtocolError(
+                f"The `{protocol}::` protocol looks suspicious, use `allow_unsafe_protocols=True` to allow it."
+            )
+
+    @classmethod
+    def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> None:
+        """
+        Check for unsafe options.
+
+        Some options that are passed to `git <command>` can be used to execute
+        arbitrary commands, this are blocked by default.
+        """
+        # Options can be of the form `foo` or `--foo bar` `--foo=bar`,
+        # so we need to check if they start with "--foo" or if they are equal to "foo".
+        bare_unsafe_options = [
+            option.lstrip("-")
+            for option in unsafe_options
+        ]
+        for option in options:
+            for unsafe_option, bare_option in zip(unsafe_options, bare_unsafe_options):
+                if option.startswith(unsafe_option) or option == bare_option:
+                    raise UnsafeOptionError(
+                        f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it."
+                    )
+
     class AutoInterrupt(object):
         """Kill/Interrupt the stored process instance once this instance goes out of scope. It is
         used to prevent processes piling up in case iterators stop reading.
@@ -1148,12 +1193,12 @@ def transform_kwargs(self, split_single_char_options: bool = True, **kwargs: Any
         return args
 
     @classmethod
-    def __unpack_args(cls, arg_list: Sequence[str]) -> List[str]:
+    def _unpack_args(cls, arg_list: Sequence[str]) -> List[str]:
 
         outlist = []
         if isinstance(arg_list, (list, tuple)):
             for arg in arg_list:
-                outlist.extend(cls.__unpack_args(arg))
+                outlist.extend(cls._unpack_args(arg))
         else:
             outlist.append(str(arg_list))
 
@@ -1238,7 +1283,7 @@ def _call_process(
         # Prepare the argument list
 
         opt_args = self.transform_kwargs(**opts_kwargs)
-        ext_args = self.__unpack_args([a for a in args if a is not None])
+        ext_args = self._unpack_args([a for a in args if a is not None])
 
         if insert_after_this_arg is None:
             args_list = opt_args + ext_args

git/exc.py

@@ -37,6 +37,14 @@ class NoSuchPathError(GitError, OSError):
     """Thrown if a path could not be access by the system."""
 
 
+class UnsafeProtocolError(GitError):
+    """Thrown if unsafe protocols are passed without being explicitly allowed."""
+
+
+class UnsafeOptionError(GitError):
+    """Thrown if unsafe options are passed without being explicitly allowed."""
+
+
 class CommandError(GitError):
     """Base class for exceptions thrown at every stage of `Popen()` execution.
 

git/objects/submodule/base.py

@@ -272,7 +272,16 @@ def _module_abspath(cls, parent_repo: "Repo", path: PathLike, name: str) -> Path
         # end
 
     @classmethod
-    def _clone_repo(cls, repo: "Repo", url: str, path: PathLike, name: str, **kwargs: Any) -> "Repo":
+    def _clone_repo(
+        cls,
+        repo: "Repo",
+        url: str,
+        path: PathLike,
+        name: str,
+        allow_unsafe_options: bool = False,
+        allow_unsafe_protocols: bool = False,
+        **kwargs: Any,
+    ) -> "Repo":
         """:return: Repo instance of newly cloned repository
         :param repo: our parent repository
         :param url: url to clone from
@@ -289,7 +298,13 @@ def _clone_repo(cls, repo: "Repo", url: str, path: PathLike, name: str, **kwargs
             module_checkout_path = osp.join(str(repo.working_tree_dir), path)
         # end
 
-        clone = git.Repo.clone_from(url, module_checkout_path, **kwargs)
+        clone = git.Repo.clone_from(
+            url,
+            module_checkout_path,
+            allow_unsafe_options=allow_unsafe_options,
+            allow_unsafe_protocols=allow_unsafe_protocols,
+            **kwargs,
+        )
         if cls._need_gitfile_submodules(repo.git):
             cls._write_git_file_and_module_config(module_checkout_path, module_abspath)
         # end
@@ -359,6 +374,8 @@ def add(
         depth: Union[int, None] = None,
         env: Union[Mapping[str, str], None] = None,
         clone_multi_options: Union[Sequence[TBD], None] = None,
+        allow_unsafe_options: bool = False,
+        allow_unsafe_protocols: bool = False,
     ) -> "Submodule":
         """Add a new submodule to the given repository. This will alter the index
         as well as the .gitmodules file, but will not create a new commit.
@@ -475,7 +492,16 @@ def add(
                 kwargs["multi_options"] = clone_multi_options
 
             # _clone_repo(cls, repo, url, path, name, **kwargs):
-            mrepo = cls._clone_repo(repo, url, path, name, env=env, **kwargs)
+            mrepo = cls._clone_repo(
+                repo,
+                url,
+                path,
+                name,
+                env=env,
+                allow_unsafe_options=allow_unsafe_options,
+                allow_unsafe_protocols=allow_unsafe_protocols,
+                **kwargs,
+            )
         # END verify url
 
         ## See #525 for ensuring git urls in config-files valid under Windows.
@@ -520,6 +546,8 @@ def update(
         keep_going: bool = False,
         env: Union[Mapping[str, str], None] = None,
         clone_multi_options: Union[Sequence[TBD], None] = None,
+        allow_unsafe_options: bool = False,
+        allow_unsafe_protocols: bool = False,
     ) -> "Submodule":
         """Update the repository of this submodule to point to the checkout
         we point at with the binsha of this instance.
@@ -643,6 +671,8 @@ def update(
                         n=True,
                         env=env,
                         multi_options=clone_multi_options,
+                        allow_unsafe_options=allow_unsafe_options,
+                        allow_unsafe_protocols=allow_unsafe_protocols,
                     )
                 # END handle dry-run
                 progress.update(

git/remote.py

@@ -539,6 +539,23 @@ class Remote(LazyMixin, IterableObj):
     __slots__ = ("repo", "name", "_config_reader")
     _id_attribute_ = "name"
 
+    unsafe_git_fetch_options = [
+        # This option allows users to execute arbitrary commands.
+        # https://git-scm.com/docs/git-fetch#Documentation/git-fetch.txt---upload-packltupload-packgt
+        "--upload-pack",
+    ]
+    unsafe_git_pull_options = [
+        # This option allows users to execute arbitrary commands.
+        # https://git-scm.com/docs/git-pull#Documentation/git-pull.txt---upload-packltupload-packgt
+        "--upload-pack"
+    ]
+    unsafe_git_push_options = [
+        # This option allows users to execute arbitrary commands.
+        # https://git-scm.com/docs/git-push#Documentation/git-push.txt---execltgit-receive-packgt
+        "--receive-pack",
+        "--exec",
+    ]
+
     def __init__(self, repo: "Repo", name: str) -> None:
         """Initialize a remote instance
 
@@ -615,7 +632,9 @@ def iter_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> Iterator["Remote
             yield Remote(repo, section[lbound + 1 : rbound])
         # END for each configuration section
 
-    def set_url(self, new_url: str, old_url: Optional[str] = None, **kwargs: Any) -> "Remote":
+    def set_url(
+        self, new_url: str, old_url: Optional[str] = None, allow_unsafe_protocols: bool = False, **kwargs: Any
+    ) -> "Remote":
         """Configure URLs on current remote (cf command git remote set_url)
 
         This command manages URLs on the remote.
@@ -624,15 +643,17 @@ def set_url(self, new_url: str, old_url: Optional[str] = None, **kwargs: Any) ->
         :param old_url: when set, replaces this URL with new_url for the remote
         :return: self
         """
+        if not allow_unsafe_protocols:
+            Git.check_unsafe_protocols(new_url)
         scmd = "set-url"
         kwargs["insert_kwargs_after"] = scmd
         if old_url:
-            self.repo.git.remote(scmd, self.name, new_url, old_url, **kwargs)
+            self.repo.git.remote(scmd, "--", self.name, new_url, old_url, **kwargs)
         else:
-            self.repo.git.remote(scmd, self.name, new_url, **kwargs)
+            self.repo.git.remote(scmd, "--", self.name, new_url, **kwargs)
         return self
 
-    def add_url(self, url: str, **kwargs: Any) -> "Remote":
+    def add_url(self, url: str, allow_unsafe_protocols: bool = False, **kwargs: Any) -> "Remote":
         """Adds a new url on current remote (special case of git remote set_url)
 
         This command adds new URLs to a given remote, making it possible to have
@@ -641,7 +662,7 @@ def add_url(self, url: str, **kwargs: Any) -> "Remote":
         :param url: string being the URL to add as an extra remote URL
         :return: self
         """
-        return self.set_url(url, add=True)
+        return self.set_url(url, add=True, allow_unsafe_protocols=allow_unsafe_protocols)
 
     def delete_url(self, url: str, **kwargs: Any) -> "Remote":
         """Deletes a new url on current remote (special case of git remote set_url)
@@ -733,7 +754,7 @@ def stale_refs(self) -> IterableList[Reference]:
         return out_refs
 
     @classmethod
-    def create(cls, repo: "Repo", name: str, url: str, **kwargs: Any) -> "Remote":
+    def create(cls, repo: "Repo", name: str, url: str, allow_unsafe_protocols: bool = False, **kwargs: Any) -> "Remote":
         """Create a new remote to the given repository
         :param repo: Repository instance that is to receive the new remote
         :param name: Desired name of the remote
@@ -743,7 +764,10 @@ def create(cls, repo: "Repo", name: str, url: str, **kwargs: Any) -> "Remote":
         :raise GitCommandError: in case an origin with that name already exists"""
         scmd = "add"
         kwargs["insert_kwargs_after"] = scmd
-        repo.git.remote(scmd, name, Git.polish_url(url), **kwargs)
+        url = Git.polish_url(url)
+        if not allow_unsafe_protocols:
+            Git.check_unsafe_protocols(url)
+        repo.git.remote(scmd, "--", name, url, **kwargs)
         return cls(repo, name)
 
     # add is an alias
@@ -925,6 +949,8 @@ def fetch(
         progress: Union[RemoteProgress, None, "UpdateProgress"] = None,
         verbose: bool = True,
         kill_after_timeout: Union[None, float] = None,
+        allow_unsafe_protocols: bool = False,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> IterableList[FetchInfo]:
         """Fetch the latest changes for this remote
@@ -967,6 +993,14 @@ def fetch(
         else:
             args = [refspec]
 
+        if not allow_unsafe_protocols:
+            for ref in args:
+                if ref:
+                    Git.check_unsafe_protocols(ref)
+
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_fetch_options)
+
         proc = self.repo.git.fetch(
             "--", self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs
         )
@@ -980,6 +1014,8 @@ def pull(
         refspec: Union[str, List[str], None] = None,
         progress: Union[RemoteProgress, "UpdateProgress", None] = None,
         kill_after_timeout: Union[None, float] = None,
+        allow_unsafe_protocols: bool = False,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> IterableList[FetchInfo]:
         """Pull changes from the given branch, being the same as a fetch followed
@@ -994,6 +1030,15 @@ def pull(
             # No argument refspec, then ensure the repo's config has a fetch refspec.
             self._assert_refspec()
         kwargs = add_progress(kwargs, self.repo.git, progress)
+
+        refspec = Git._unpack_args(refspec or [])
+        if not allow_unsafe_protocols:
+            for ref in refspec:
+                Git.check_unsafe_protocols(ref)
+
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_pull_options)
+
         proc = self.repo.git.pull(
             "--", self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs
         )
@@ -1007,6 +1052,8 @@ def push(
         refspec: Union[str, List[str], None] = None,
         progress: Union[RemoteProgress, "UpdateProgress", Callable[..., RemoteProgress], None] = None,
         kill_after_timeout: Union[None, float] = None,
+        allow_unsafe_protocols: bool = False,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> PushInfoList:
         """Push changes from source branch in refspec to target branch in refspec.
@@ -1037,6 +1084,15 @@ def push(
             be 0.
             Call ``.raise_if_error()`` on the returned object to raise on any failure."""
         kwargs = add_progress(kwargs, self.repo.git, progress)
+
+        refspec = Git._unpack_args(refspec or [])
+        if not allow_unsafe_protocols:
+            for ref in refspec:
+                Git.check_unsafe_protocols(ref)
+
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_push_options)
+
         proc = self.repo.git.push(
             "--",
             self,