Quote URLs in Repo.clone_from()#1516

s-t-e-v-e-n-k

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

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

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

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

stsewd

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

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.

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,

Byron

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 wants to merge 2 commits into
gitpython-developers:mainfrom
s-t-e-v-e-n-k:quote-clone-urls-by-default

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

Uh oh!

Byron left a comment

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

stsewd commented Dec 15, 2022 •

edited

Loading

Uh oh!

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

Uh oh!

Byron left a comment

Uh oh!

Byron left a comment

Uh oh!

stsewd left a comment

Uh oh!

Byron commented Dec 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

stsewd commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

stsewd left a comment

Choose a reason for hiding this comment

Uh oh!

Byron commented Dec 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

stsewd commented Dec 15, 2022 •

edited

Loading