From fbf9c7e72218e44bc29eb4907d5c00118370376b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 20 Dec 2022 20:26:37 -0500 Subject: [PATCH 1/4] Fix command injection Add `--` in some commands that receive user input and if interpreted as options could lead to remote code execution (RCE). There may be more commands that could benefit from `--` so the input is never interpreted as an option, but most of those aren't dangerous. Fixed commands: - push - pull - fetch - clone/clone_from and friends - archive (not sure if this one can be exploited, but it doesn't hurt adding `--` :)) For anyone using GitPython and exposing any of the GitPython methods to users, make sure to always validate the input (like if starts with `--`). And for anyone allowing users to pass arbitrary options, be aware that some options may lead fo RCE, like `--exc`, `--upload-pack`, `--receive-pack`, `--config` (https://github.com/gitpython-developers/GitPython/pull/1516). Ref https://github.com/gitpython-developers/GitPython/issues/1517 --- git/remote.py | 5 +++-- git/repo/base.py | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/git/remote.py b/git/remote.py index 7b44020c5..483d536ae 100644 --- a/git/remote.py +++ b/git/remote.py @@ -964,7 +964,7 @@ def fetch( args = [refspec] proc = self.repo.git.fetch( - self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs + "--", self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs ) res = self._get_fetch_info_from_stderr(proc, progress, kill_after_timeout=kill_after_timeout) if hasattr(self.repo.odb, "update_cache"): @@ -991,7 +991,7 @@ def pull( self._assert_refspec() kwargs = add_progress(kwargs, self.repo.git, progress) proc = self.repo.git.pull( - self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs + "--", self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs ) res = self._get_fetch_info_from_stderr(proc, progress, kill_after_timeout=kill_after_timeout) if hasattr(self.repo.odb, "update_cache"): @@ -1034,6 +1034,7 @@ def push( be 0.""" kwargs = add_progress(kwargs, self.repo.git, progress) proc = self.repo.git.push( + "--", self, refspec, porcelain=True, diff --git a/git/repo/base.py b/git/repo/base.py index c49c61184..49a3d5a16 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -1169,6 +1169,7 @@ def _clone( multi = shlex.split(" ".join(multi_options)) proc = git.clone( multi, + "--", Git.polish_url(str(url)), clone_path, with_extended_output=True, @@ -1305,7 +1306,7 @@ def archive( if not isinstance(path, (tuple, list)): path = [path] # end assure paths is list - self.git.archive(treeish, *path, **kwargs) + self.git.archive("--", treeish, *path, **kwargs) return self def has_separate_working_tree(self) -> bool: From 3c51865399ab7e4454d6d2568d30f9a10ed36f8d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 20 Dec 2022 22:11:12 -0500 Subject: [PATCH 2/4] Fix CI Taken from https://github.com/gitpython-developers/GitPython/pull/1516/ --- .github/workflows/pythonpackage.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 5c698bae1..5373dace6 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -18,7 +18,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - python-version: [3.7, 3.7.5, 3.7.12, 3.8, 3.8.0, 3.8.11, 3.8, 3.9, 3.9.0, 3.9.7, "3.10"] + python-version: [3.7, 3.8, 3.9, "3.10", "3.11"] steps: - uses: actions/checkout@v3 From 7918fccff8ba341a8747381162f587749f08d23a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 21 Dec 2022 22:15:55 -0500 Subject: [PATCH 3/4] Add test --- test/test_repo.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/test_repo.py b/test/test_repo.py index 703dbb438..6382db7e4 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -1180,3 +1180,29 @@ def test_do_not_strip_newline_in_stdout(self, rw_dir): r.git.add(Git.polish_url(fp)) r.git.commit(message="init") self.assertEqual(r.git.show("HEAD:hello.txt", strip_newline_in_stdout=False), "hello\n") + + @with_rw_repo("HEAD") + def test_clone_command_injection(self, rw_repo): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + unexpected_file = tmp_dir / "pwn" + assert not unexpected_file.exists() + + payload = f"--upload-pack=touch {unexpected_file}" + rw_repo.clone(payload) + + assert not unexpected_file.exists() + # A repo was cloned with the payload as name + assert pathlib.Path(payload).exists() + + @with_rw_repo("HEAD") + def test_clone_from_command_injection(self, rw_repo): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + temp_repo = Repo.init(tmp_dir / "repo") + unexpected_file = tmp_dir / "pwn" + + assert not unexpected_file.exists() + payload = f"--upload-pack=touch {unexpected_file}" + with self.assertRaises(GitCommandError): + rw_repo.clone_from(payload, temp_repo.common_dir) + + assert not unexpected_file.exists() From 2aae532a3993a100d5074cde70abe548cfc45861 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 22 Dec 2022 17:08:27 +0100 Subject: [PATCH 4/4] update changelog --- doc/source/changes.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/doc/source/changes.rst b/doc/source/changes.rst index d36194c86..a784a096a 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,18 @@ Changelog ========= +3.1.30 +====== + +- Make injections of command-invocations harder or impossible for clone and others. + See https://github.com/gitpython-developers/GitPython/pull/1518 for details. + Note that this might constitute a breaking change for some users, and if so please + let us know and we add an opt-out to this. + +See the following for all changes. +https://github.com/gitpython-developers/gitpython/milestone/60?closed=1 + + 3.1.29 ======