Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Quote URLs in Repo.clone_from()#1516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Quote URLs in Repo.clone_from()#1516
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 theext 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 theunsafe_protocols feature. My intuition here is also to document it to allow folks to bypass the quoting in case they relied on using theext 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.
Quoting removes URL unsafe characters, such as spaces, so we end up with: 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. |
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 |
a3bc689 to0cd4e4dCompare@Byron Sadly, I've had to change this around to not quote, but this is ready for another look. |
stsewd commentedDec 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In#1515 (comment) there is this case that should be fixed too fromgitimportRepor=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') |
This is sort of the problem -- git isdesigned to run arbitrary commands. However, I'll see if I can refactor the checks out to a function. |
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.
git/repo/base.py Outdated
| :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: |
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 thaturl.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.
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.
git/repo/base.py Outdated
| :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") |
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? MaybeValueError is already specific enough compared to the errors that are usually thrown from this function (but I don't know).
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?
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.
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
0cd4e4d toe526cbbCompareSince the URL is passed directly to git clone, and the remote-ext helperwill happily execute shell commands, so by default disallow URLs thatcontain a "::" unless a new unsafe_protocols kwarg is passed.(CVE-2022-24439)Fixesgitpython-developers#1515
With a large number of the versions of Python specified not beingsupported with GitHub Actions, trim it down to the major versions, andadd in 3.11 for good measure.
e526cbb toce529b9CompareThere 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 thinkthe CI failure is legit as the sanity check is done in a way that assumes a string even thoughPosixPath 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 inputand if interpreted as options could lead to remotecode 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 awarethat some options may lead fo RCE, like `--exc`, `--upload-pack`,`--receive-pack`, `--config` (gitpython-developers#1516).Refgitpython-developers#1517
Add `--` in some commands that receive user inputand if interpreted as options could lead to remotecode 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 awarethat some options may lead fo RCE, like `--exc`, `--upload-pack`,`--receive-pack`, `--config` (gitpython-developers#1516).Refgitpython-developers#1517
Taken fromgitpython-developers#1516
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 fromhttps://git-scm.com/docs/gitremote-helpers
A URL of the form
<transport>::<address>explicitly instructs Git to invokegit 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 disallowfoo::bar like URLs,
| if not unsafe_protocols and cls.unsafe_options(url, multi_options): | ||
| raise UnsafeOptionsUsedError(f"{url} requires unsafe_protocols flag") |
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.
Thanks for your help! Due to the time-sensitive nature,#1521 was merged first and includes the changes from this PR. |
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