Quote URLs in Repo.clone_from() #1516

s-t-e-v-e-n-k · 2022-12-13T08:12:55Z

Since the URL is passed directly to git clone, and the remote-ext helper will happily execute shell commands, so by default qoute URLs unless a (undocumented, on purpose) kwarg is passed. (CVE-2022-24439)

Fixes #1515

Byron

Thanks a lot, much appreciated. I don't know why quoting the URL prevents it from being effective when the ext protocol is allowed. There seem to be no internal quotes in the 'ext:…' url that would be quoted then.

In any case, I think it would be useful to have a test for the unsafe_protocols feature. My intuition here is also to document it to allow folks to bypass the quoting in case they relied on using the ext protocol.

Regrading CI, I have a feeling that the python action doesn't support older versions anymore - please feel free to remove them from the test suite so there is a chance for the tests to succeed.

s-t-e-v-e-n-k · 2022-12-13T11:05:41Z

Quoting removes URL unsafe characters, such as spaces, so we end up with:

>>> import tempfile
>>> import urllib.parse
>>> import uuid
>>> bad_filename = f'{tempfile.gettempdir()}/{uuid.uuid4()}'
>>> bad_url = f'ext::sh -c touch% {bad_filename}'
>>> urllib.parse.quote(bad_url)
'ext%3A%3Ash%20-c%20touch%25%20/tmp/280b1b72-b764-424e-aaf9-f1496b5e253d'

which will not execute. Successfully, anyway -- and should also leave most of the usual suspects https:// and git:// alone, but I will confirm when I fix up GitHub Actions.

s-t-e-v-e-n-k · 2022-12-13T11:06:35Z

I may have to parse it and then quote the host portion, depending on if it's overzealous, but I have it in hand. I will also add a test for unsafe_protocols.

s-t-e-v-e-n-k · 2022-12-15T08:03:30Z

@Byron Sadly, I've had to change this around to not quote, but this is ready for another look.

stsewd · 2022-12-15T16:24:58Z

In #1515 (comment) there is this case that should be fixed too

from git import Repo
r = Repo.init('', bare=True)
r.clone('foo', '/tmp/foo', multi_options=['--upload-pack="touch /tmp/pwn"'])

Here the URL can be anything. We could check/ban the unsafe options (--upload-pack/-c protocol.*) instead of (or in addition of) checking the protocol.

EDIT: This also works

r.clone('--upload-pack=touch /tmp/pwn')

s-t-e-v-e-n-k · 2022-12-16T06:39:00Z

This is sort of the problem -- git is designed to run arbitrary commands. However, I'll see if I can refactor the checks out to a function.

Byron

Thanks a lot for your help with CI!

I have a few comments and questions that I hope we can sort out quickly so we can get this PR merged and cut a new release right after that.

Thank you for your continued help.

Byron · 2022-12-16T07:57:00Z

git/repo/base.py

        :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)
+        if not unsafe_protocols and "::" in url:


Could we factor "::" in url out into its own publicly available and documented function to allow it to be used before trying to clone as well? Furthermore, this would allow to run unit-tests against it which I'd love to see. Particularly, are there ways to get false positives for legitimate cases, or false negatives in case the url is obfuscated somehow? My intuition here is that url.startswith('ext::') might reduce false positives, but specific tests should help find the sweet spot.

It would be interesting to find out if a url like ext::… (note the leading whitespace) also works. I'd guess it won't work as the command invocation doesn't use a shell by default. If that's indeed the case, one could make the url check more specific.

Indeed, and we probably should -- if other cases come up, we can add them there.

Byron · 2022-12-16T07:59:09Z

git/repo/base.py

        :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)
+        if not unsafe_protocols and "::" in url:
+            raise ValueError(f"{url} requires unsafe_protocols flag")


Is there a more specific error type that could be used to better classify the issue? Maybe ValueError is already specific enough compared to the errors that are usually thrown from this function (but I don't know).

I'm happy to add a UnsafeOptionsException or something similar, but ValueError is described as when a function receives an argument that has the right type but an inappropriate value, which feels right too?

In this particular case, having something uniquely identifiable as this particular error is probably helpful for folks who would use it to learn about the kind of URLs thrown at them. ValueError might be unique enough, but it's hard to prove it can't be raised elsewhere either.

random 2 cents from a random guy: If I was using a library and it found some dangerous input, I would prefer to have an exception that clearly says so

While ValueError fits the description, adding the "this is can be dangerous" bit would help I think

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 gitpython-developers#1515

With a large number of the versions of Python specified not being supported with GitHub Actions, trim it down to the major versions, and add in 3.11 for good measure.

Byron

Thanks a lot for taking it to the next level, I love the additional tests for what constitutes unsafe options.

I think the CI failure is legit as the sanity check is done in a way that assumes a string even though PosixPath isn't particularly string like.

Something I am not so sure about is if this PR should try to address #1517, and I'd clearly prefer that to happen in another PR. Probably @stsewd could provide valuable feedback here as well.

Thanks for taking another look.

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. 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` (gitpython-developers#1516). Ref gitpython-developers#1517

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` (gitpython-developers#1516). Ref gitpython-developers#1517

Taken from gitpython-developers#1516

stsewd

Taking a look at this, if gitpython is going to disallow passing the --upload-pack/--receive-pack and friends as options, the check should probably be in all other commands where those options are allowed (and should also check for their short form -u).

And reading from https://git-scm.com/docs/gitremote-helpers

A URL of the form <transport>::<address> explicitly instructs Git to invoke git remote-<transport> with <address> as the second argument. If such a URL is encountered directly on the command line, the first argument is <address>, and if it is encountered in a configured remote, the first argument is the name of that remote.

Maybe is safer to disallow foo::bar like URLs,

stsewd · 2022-12-21T03:20:19Z

git/repo/base.py

+        if not unsafe_protocols and cls.unsafe_options(url, multi_options):
+            raise UnsafeOptionsUsedError(f"{url} requires unsafe_protocols flag")


these checks could probably be moved to the _clone() method, so you don't need to add them everywhere where that is called.

Byron · 2022-12-29T07:11:02Z

Thanks for your help!

Due to the time-sensitive nature, #1521 was merged first and includes the changes from this PR.

Byron requested changes Dec 13, 2022

View reviewed changes

s-t-e-v-e-n-k force-pushed the quote-clone-urls-by-default branch 2 times, most recently from a3bc689 to 0cd4e4d Compare December 15, 2022 04:59

Byron mentioned this pull request Dec 16, 2022

CVE-2022-24439: <gitpython::clone> 'ext::sh -c touch% /tmp/pwned' for remote code execution #1515

Closed

Byron requested changes Dec 16, 2022

View reviewed changes

s-t-e-v-e-n-k force-pushed the quote-clone-urls-by-default branch from 0cd4e4d to e526cbb Compare December 20, 2022 06:14

s-t-e-v-e-n-k added 2 commits December 20, 2022 17:41

Trim down list of python-version

ce529b9

With a large number of the versions of Python specified not being supported with GitHub Actions, trim it down to the major versions, and add in 3.11 for good measure.

s-t-e-v-e-n-k force-pushed the quote-clone-urls-by-default branch from e526cbb to ce529b9 Compare December 20, 2022 06:42

Byron requested changes Dec 20, 2022

View reviewed changes

stsewd mentioned this pull request Dec 21, 2022

Fix command injection #1518

Merged

stsewd added a commit to stsewd/GitPython that referenced this pull request Dec 21, 2022

Fix CI

3c51865

Taken from gitpython-developers#1516

stsewd reviewed Dec 21, 2022

View reviewed changes

stsewd mentioned this pull request Dec 24, 2022

Block insecure options and protocols by default #1521

Merged

Byron closed this Dec 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quote URLs in Repo.clone_from() #1516

Quote URLs in Repo.clone_from() #1516

s-t-e-v-e-n-k commented Dec 13, 2022

Byron left a comment

s-t-e-v-e-n-k commented Dec 13, 2022

s-t-e-v-e-n-k commented Dec 13, 2022

s-t-e-v-e-n-k commented Dec 15, 2022

stsewd commented Dec 15, 2022 •

edited

Loading

s-t-e-v-e-n-k commented Dec 16, 2022

Byron left a comment

Byron Dec 16, 2022

s-t-e-v-e-n-k Dec 19, 2022

Byron Dec 16, 2022

s-t-e-v-e-n-k Dec 19, 2022

Byron Dec 19, 2022

h3nnn4n Dec 20, 2022

Byron left a comment

stsewd left a comment

stsewd Dec 21, 2022

Byron commented Dec 29, 2022

		if not unsafe_protocols and cls.unsafe_options(url, multi_options):
		raise UnsafeOptionsUsedError(f"{url} requires unsafe_protocols flag")

Quote URLs in Repo.clone_from() #1516

Quote URLs in Repo.clone_from() #1516

Conversation

s-t-e-v-e-n-k commented Dec 13, 2022

Byron left a comment

Choose a reason for hiding this comment

s-t-e-v-e-n-k commented Dec 13, 2022

s-t-e-v-e-n-k commented Dec 13, 2022

s-t-e-v-e-n-k commented Dec 15, 2022

stsewd commented Dec 15, 2022 • edited Loading

s-t-e-v-e-n-k commented Dec 16, 2022

Byron left a comment

Choose a reason for hiding this comment

Byron Dec 16, 2022

Choose a reason for hiding this comment

s-t-e-v-e-n-k Dec 19, 2022

Choose a reason for hiding this comment

Byron Dec 16, 2022

Choose a reason for hiding this comment

s-t-e-v-e-n-k Dec 19, 2022

Choose a reason for hiding this comment

Byron Dec 19, 2022

Choose a reason for hiding this comment

h3nnn4n Dec 20, 2022

Choose a reason for hiding this comment

Byron left a comment

Choose a reason for hiding this comment

stsewd left a comment

Choose a reason for hiding this comment

stsewd Dec 21, 2022

Choose a reason for hiding this comment

Byron commented Dec 29, 2022

stsewd commented Dec 15, 2022 •

edited

Loading