GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Quote URLs in Repo.clone_from() by s-t-e-v-e-n-k · Pull Request #1516 · gitpython-developers/GitPython

Conversation

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

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sagarvora reacted with thumbs up emoji

ayush-shah reacted with heart emoji

All reactions

👍 1 reaction
❤️ 1 reaction

Byron requested changes

Dec 13, 2022

View reviewed changes

Byron left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Copy link

Copy Markdown

Contributor Author

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.

Byron reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

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

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

stsewd commented Dec 15, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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')

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Byron left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

git/repo/base.py Outdated

		git = cls.GitCommandWrapperType(os.getcwd())
		if env is not None:
		git.update_environment(**env)
		if not unsafe_protocols and "::" in url:

Byron Dec 16, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Byron reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

git/repo/base.py Outdated

		if env is not None:
		git.update_environment(**env)
		if not unsafe_protocols and "::" in url:
		raise ValueError(f"{url} requires unsafe_protocols flag")

Byron Dec 16, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Byron Dec 19, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

h3nnn4n Dec 20, 2022

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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


          Forbid unsafe protocol URLs in Repo.clone{,_from}()

24215ac

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


          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

Byron left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

stsewd added a commit to stsewd/GitPython that referenced this pull request

Dec 21, 2022


          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.

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

stsewd added a commit to stsewd/GitPython that referenced this pull request

Dec 21, 2022


          Fix command injection

fbf9c7e

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

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 left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Byron reacted with heart emoji

All reactions

❤️ 1 reaction

Comment thread

git/repo/base.py

Comment on lines +1302 to +1303

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

stsewd Dec 21, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

stsewd mentioned this pull request

Dec 24, 2022

Block insecure options and protocols by default #1521

Merged

Byron commented Dec 29, 2022

Copy link

Copy Markdown

Member

Thanks for your help!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Byron closed this

Dec 29, 2022

Reviewers

Byron

Byron requested changes

+2 more reviewers

stsewd

stsewd left review comments

h3nnn4n

h3nnn4n left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Byron commented Dec 29, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

stsewd commented Dec 15, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.