From fe9b71628767610a238e47cd46b82d411a7e871a Mon Sep 17 00:00:00 2001 From: Narpat Mali Date: Sat, 7 Jan 2023 17:16:57 +0000 Subject: [PATCH] python3-git: CVE-2022-24439 fix from PR 1521 Forbid unsafe protocol URLs in Repo.clone{,_from}() Since the URL is passed directly to git clone, and the remote-ext helper will happily execute shell commands, so by default disallow URLs that contain a "::" unless a new unsafe_protocols kwarg is passed. (CVE-2022-24439) Fixes #1515 CVE: CVE-2022-24439 Upstream-Status: Backport [https://github.com/gitpython-developers/GitPython/pull/1521] Signed-off-by: Narpat Mali --- git/cmd.py | 51 ++++++++++++++++++++++++-- git/exc.py | 8 ++++ git/objects/submodule/base.py | 19 ++++++---- git/remote.py | 69 +++++++++++++++++++++++++++++++---- git/repo/base.py | 44 ++++++++++++++++++---- 5 files changed, 166 insertions(+), 25 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 4f05698..77026d6 100644 --- a/git/cmd.py +++ b/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 @@ -31,7 +32,9 @@ from git.util import is_cygwin_git, cygpath, expand_path, remove_password_if_pre from .exc import ( GitCommandError, - GitCommandNotFound + GitCommandNotFound, + UnsafeOptionError, + UnsafeProtocolError ) from .util import ( LazyMixin, @@ -225,6 +228,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_) @@ -400,6 +405,44 @@ class Git(LazyMixin): 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 `::
`, + 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 ` 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. @@ -1068,12 +1111,12 @@ class Git(LazyMixin): 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)) @@ -1154,7 +1197,7 @@ class Git(LazyMixin): # 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 diff --git a/git/exc.py b/git/exc.py index e8ff784..5c96db2 100644 --- a/git/exc.py +++ b/git/exc.py @@ -36,6 +36,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. diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index f782045..deb224e 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -264,7 +264,8 @@ class Submodule(IndexObject, TraversableIterableObj): # 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 @@ -281,7 +282,8 @@ class Submodule(IndexObject, TraversableIterableObj): 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 @@ -338,8 +340,8 @@ class Submodule(IndexObject, TraversableIterableObj): @classmethod def add(cls, repo: 'Repo', name: str, path: PathLike, url: Union[str, None] = None, branch: Union[str, None] = None, no_checkout: bool = False, depth: Union[int, None] = None, - env: Union[Mapping[str, str], None] = None, clone_multi_options: Union[Sequence[TBD], None] = None - ) -> 'Submodule': + 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. If the submodule already exists, no matter if the configuration differs @@ -447,7 +449,8 @@ class Submodule(IndexObject, TraversableIterableObj): 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. @@ -484,7 +487,8 @@ class Submodule(IndexObject, TraversableIterableObj): def update(self, recursive: bool = False, init: bool = True, to_latest_revision: bool = False, progress: Union['UpdateProgress', None] = None, dry_run: bool = False, force: bool = False, keep_going: bool = False, env: Union[Mapping[str, str], None] = None, - clone_multi_options: Union[Sequence[TBD], None] = None) -> 'Submodule': + 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. @@ -585,7 +589,8 @@ class Submodule(IndexObject, TraversableIterableObj): (self.url, checkout_module_abspath, self.name)) if not dry_run: mrepo = self._clone_repo(self.repo, self.url, self.path, self.name, n=True, env=env, - multi_options=clone_multi_options) + multi_options=clone_multi_options, allow_unsafe_options=allow_unsafe_options, + allow_unsafe_protocols=allow_unsafe_protocols) # END handle dry-run progress.update(END | CLONE, 0, 1, prefix + "Done cloning to %s" % checkout_module_abspath) diff --git a/git/remote.py b/git/remote.py index 59681bc..cea6b99 100644 --- a/git/remote.py +++ b/git/remote.py @@ -473,6 +473,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 @@ -549,7 +566,8 @@ class Remote(LazyMixin, IterableObj): 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. @@ -558,15 +576,17 @@ class Remote(LazyMixin, IterableObj): :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 @@ -575,7 +595,7 @@ class Remote(LazyMixin, IterableObj): :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) @@ -667,7 +687,7 @@ class Remote(LazyMixin, IterableObj): 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 @@ -677,7 +697,10 @@ class Remote(LazyMixin, IterableObj): :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 @@ -840,6 +863,8 @@ class Remote(LazyMixin, IterableObj): 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 @@ -881,6 +906,14 @@ class Remote(LazyMixin, IterableObj): 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) res = self._get_fetch_info_from_stderr(proc, progress, @@ -892,6 +925,8 @@ class Remote(LazyMixin, IterableObj): def pull(self, 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 by a merge of branch with your local branch. @@ -905,6 +940,15 @@ class Remote(LazyMixin, IterableObj): # 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) res = self._get_fetch_info_from_stderr(proc, progress, @@ -916,6 +960,8 @@ class Remote(LazyMixin, IterableObj): def push(self, 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) -> IterableList[PushInfo]: """Push changes from source branch in refspec to target branch in refspec. @@ -945,6 +991,15 @@ class Remote(LazyMixin, IterableObj): If the operation fails completely, the length of the returned IterableList will be 0.""" 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, refspec, porcelain=True, as_process=True, universal_newlines=True, kill_after_timeout=kill_after_timeout, diff --git a/git/repo/base.py b/git/repo/base.py index f14f929..7b3565b 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -24,7 +24,11 @@ from git.compat import ( ) from git.config import GitConfigParser from git.db import GitCmdObjectDB -from git.exc import InvalidGitRepositoryError, NoSuchPathError, GitCommandError +from git.exc import ( + GitCommandError, + InvalidGitRepositoryError, + NoSuchPathError, +) from git.index import IndexFile from git.objects import Submodule, RootModule, Commit from git.refs import HEAD, Head, Reference, TagReference @@ -97,6 +101,18 @@ class Repo(object): re_author_committer_start = re.compile(r'^(author|committer)') re_tab_full_line = re.compile(r'^\t(.*)$') + unsafe_git_clone_options = [ + # This option allows users to execute arbitrary commands. + # https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---upload-packltupload-packgt + "--upload-pack", + "-u", + # Users can override configuration variables + # like `protocol.allow` or `core.gitProxy` to execute arbitrary commands. + # https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---configltkeygtltvaluegt + "--config", + "-c", + ] + # invariants # represents the configuration level of a configuration file config_level: ConfigLevels_Tup = ("system", "user", "global", "repository") @@ -1049,7 +1065,8 @@ class Repo(object): @ classmethod def _clone(cls, git: 'Git', url: PathLike, path: PathLike, odb_default_type: Type[GitCmdObjectDB], progress: Union['RemoteProgress', 'UpdateProgress', Callable[..., 'RemoteProgress'], None] = None, - multi_options: Optional[List[str]] = None, **kwargs: Any + multi_options: Optional[List[str]] = None, allow_unsafe_protocols: bool = False, + allow_unsafe_options: bool = False, **kwargs: Any ) -> 'Repo': odbt = kwargs.pop('odbt', odb_default_type) @@ -1072,6 +1089,12 @@ class Repo(object): multi = None if multi_options: multi = shlex.split(' '.join(multi_options)) + + if not allow_unsafe_protocols: + Git.check_unsafe_protocols(str(url)) + if not allow_unsafe_options and multi_options: + Git.check_unsafe_options(options=multi_options, unsafe_options=cls.unsafe_git_clone_options) + proc = git.clone("--", multi, Git.polish_url(str(url)), clone_path, with_extended_output=True, as_process=True, v=True, universal_newlines=True, **add_progress(kwargs, git, progress)) if progress: @@ -1107,7 +1130,9 @@ class Repo(object): return repo def clone(self, path: PathLike, progress: Optional[Callable] = None, - multi_options: Optional[List[str]] = None, **kwargs: Any) -> 'Repo': + multi_options: Optional[List[str]] = None, unsafe_protocols: bool = False, + allow_unsafe_protocols: bool = False, allow_unsafe_options: bool = False, + **kwargs: Any) -> 'Repo': """Create a clone from this repository. :param path: is the full path of the new repo (traditionally ends with ./.git). @@ -1116,18 +1141,21 @@ class Repo(object): option per list item which is passed exactly as specified to clone. For example ['--config core.filemode=false', '--config core.ignorecase', '--recurse-submodule=repo1_path', '--recurse-submodule=repo2_path'] + :param unsafe_protocols: Allow unsafe protocols to be used, like ex :param kwargs: * odbt = ObjectDatabase Type, allowing to determine the object database implementation used by the returned Repo instance * All remaining keyword arguments are given to the git-clone command :return: ``git.Repo`` (the newly cloned repo)""" - return self._clone(self.git, self.common_dir, path, type(self.odb), progress, multi_options, **kwargs) + return self._clone(self.git, self.common_dir, path, type(self.odb), progress, multi_options, + allow_unsafe_protocols=allow_unsafe_protocols, allow_unsafe_options=allow_unsafe_options, **kwargs) @ classmethod def clone_from(cls, url: PathLike, to_path: PathLike, progress: Optional[Callable] = None, - env: Optional[Mapping[str, str]] = None, - multi_options: Optional[List[str]] = None, **kwargs: Any) -> 'Repo': + env: Optional[Mapping[str, str]] = None, multi_options: Optional[List[str]] = None, + unsafe_protocols: bool = False, allow_unsafe_protocols: bool = False, + allow_unsafe_options: bool = False, **kwargs: Any) -> 'Repo': """Create a clone from the given URL :param url: valid git url, see http://www.kernel.org/pub/software/scm/git/docs/git-clone.html#URLS @@ -1140,12 +1168,14 @@ class Repo(object): If you want to unset some variable, consider providing empty string as its value. :param multi_options: See ``clone`` method + :param unsafe_protocols: Allow unsafe protocols to be used, like ext :param kwargs: see the ``clone`` method :return: Repo instance pointing to the cloned directory""" git = cls.GitCommandWrapperType(os.getcwd()) if env is not None: git.update_environment(**env) - return cls._clone(git, url, to_path, GitCmdObjectDB, progress, multi_options, **kwargs) + return cls._clone(git, url, to_path, GitCmdObjectDB, progress, multi_options, + allow_unsafe_protocols=allow_unsafe_protocols, allow_unsafe_options=allow_unsafe_options, **kwargs) def archive(self, ostream: Union[TextIO, BinaryIO], treeish: Optional[str] = None, prefix: Optional[str] = None, **kwargs: Any) -> Repo: -- 2.34.1