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

[mmuehlenhoff]

This appeared in the CVE feed today: https://security.snyk.io/vuln/SNYK-PYTHON-GITPYTHON-3113858

Not sure if this was reported to you before or not, reporting it here just in case.

[Byron]

I am aware, have been informed and we can track it here. Thanks a lot for setting up this issue so timely.

[ajakk]

@Byron, had Snyk or Sam Wheating (@SamWheating?) contacted you previously about this or did you learn about it independently? I'm curious given there's no reference to any upstream report in the above Snyk report.

[SamWheating]

I reached out to Snyk, who I believe got in touch with the maintainers.

[ajakk]

I reached out to Snyk, who I believe got in touch with the maintainers.

What makes you think that? Again, just curious given there doesn't seem to be any indication of that happening according to their report. Also, why did you go to Snyk rather than to upstream?

[Byron]

Snyk did reach out to me by email, to my mind all this went pretty well. By publishing the issue the community can contribute a mitigation.

[ajakk]

Yeah, seems fine to me. It would have been nice to have an existing public report to go with the public release of the CVE so that all the people who handle CVEs (myself, and the reporter of this issue, for example) would know that the issue is already known to upstream and we don't have to spend time extracting that information via issues like this.

Thank you for the insight.

[stsewd]

Looks like this same vulnerability has been reported in another git library (simple-git), https://www.cve.org/CVERecord?id=CVE-2022-25912.

They added an option to opt-in in to the insecure behavior

• https://github.com/steveukx/git-js/blob/main/docs/PLUGIN-UNSAFE-ACTIONS.md#overriding-allowed-protocols
• https://github.com/steveukx/git-js/blob/de570acd052660bad0165347de5d2f86a494ae1b/simple-git/src/lib/plugins/block-unsafe-operations-plugin.ts

And this should probably check for https://www.cve.org/CVERecord?id=CVE-2022-24433 too (git clone file:///tmp/zero123 /tmp/example-new-repo --upload-pack='touch /tmp/pwn')

https://github.com/steveukx/git-js/blob/fc10fbe1cba2dedfccd8e5507b61fc979addb252/simple-git/src/lib/tasks/clone.ts#L46-L49

[SamWheating]

What makes you think that? Again, just curious given there doesn't seem to be any indication of that happening according to their report. Also, why did you go to Snyk rather than to upstream?

Snyk's vulnerability program is fantastic - you can report an issue to them and they will review it, triage it, try to get in contact with the maintainers and then register the CVE if applicable. It eliminates a lot of the overhead on my end and helps to ensure that a vulnerability is handled appropriately.

https://docs.snyk.io/more-info/disclosing-vulnerabilities/disclose-a-vulnerability-in-an-open-source-package

I didn't have a direct line to the maintainers and I didn't want to open a public issue explaining a potentially sensitive vulnerability. In this case it sounds like Snyk was able to get in touch on my behalf and handle this disclosure responsibly.

[jacwalte]

Just thought I would ping to keep this issue active. This is a critical issue in my org. Can we get a status update? Is a fix expected soon?

[Byron]

No fix is planned I don't plan to work on this directly, and this issue is triaged as 'help wanted'. Indirectly I am working on it by answering here and following up on the PR which might alleviate the problem.

[stsewd]

To avoid part of the problem, gitpython should also avoid interpreting positional arguments as option arguments. First, I was thinking in adding -- after the option args https://github.com/gitpython-developers/GitPython/blob/17ff2630af26b37f82ac1158ee3495c4390da699/git/cmd.py#L1244-L1244

but there are commands like git checkout that make special use of --, git mentions the --end-of-options option https://git-scm.com/docs/gitcli/ as an alias for -- for cases like that, but that option isn't available for git checkout :upside_down_face:

Other options could be:

• To have a list of commands (like checkout) to not add --
• Check all command calls and add -- manually to each one, for example https://github.com/gitpython-developers/GitPython/blob/17ff2630af26b37f82ac1158ee3495c4390da699/git/repo/base.py#L1170-L1180 that would be git.clone(multi, '--', ...)

[Byron]

Am I the only one confused by trying to fix a problem in the underlying git executable by trying to prevent all misuse in the layer above?

Trying to do that, to my mind, will not deter an attacker who wants to find a bypass, and is much more likely to break downstream legitimate code, or use-cases for the now deemed unsafe features. There is clearly a tradeoff that seems impossible to get right.

git by now has a history of making backwards incompatible changes by forbidding certain protocols or checking for ownership of repositories on shared file systems. Thus my preference here is to let it fix itself.

Those who pass user-input to GitPython or the git binary for that matter are obliged to validate it, and I would be all for such code to be shared in GitPython as well. Validating or sanitizing a URL seems useful, for example. If it's most definitely not breaking current legitimate uses, this could be the default like attempted in #1516 which also allows to opt-out from the potentially breaking feature.

[Anthony-Mckale]

we're using this in eze

https://github.com/RiverSafeUK/eze-cli

def sanitise_repo_url(repo_url: str) -> str:
    """
    CVE-2022-24439: sanitization of url to prevent bash script injection attacks in underlying GitPythion implementation

    warning: git clone has bug https://github.com/gitpython-developers/GitPython/issues/1515
    """
    cleaned_url = re.sub(" ", "_", repo_url)
    cleaned_url = re.sub("--", "__", cleaned_url)
    return cleaned_url

url_test_data = [
    ("Do Nothing", "https://clean.already.com/repo", "https://clean.already.com/repo"),
    (
        "Remove Injection attack",
        "https://clean.already.com/repo --hahaha='exploit town'",
        "https://clean.already.com/repo___hahaha='exploit_town'",
    ),
    (
        "Remove Double Injection attack",
        "https://clean.already.com/repo --hahaha1='exploit town' --hahaha2='exploit town'",
        "https://clean.already.com/repo___hahaha1='exploit_town'___hahaha2='exploit_town'",
    ),
]

@pytest.mark.parametrize("title,test_input,expected_output", url_test_data)
def test_sanitise_repo_url(title, test_input, expected_output):
    output = sanitise_repo_url(test_input)
    assert output == expected_output

[Byron]

Thanks for sharing!

Have you validated that URL injection works? After all the shell seems to be disabled by default and isn't enabled when calling clone either.

Without shell in the middle a url like this should be just a single string passed to git as URL and it clearly detects something is up.

✦ ❯ git clone  "https://clean.already.com/repo --hahaha='exploit town'",
Cloning into 'repo --hahaha='exploit town','...
fatal: unable to access 'https://clean.already.com/repo --hahaha='exploit town',/': URL using bad/illegal format or missing URL

For a moment I thought maybe git doesn't validate enough and passes on abusive URLs to sub-programs through a shell, but that fortunately doesn't seem to be the case.

[stsewd]

Those who pass user-input to GitPython or the git binary for that matter are obliged to validate it, and I would be all for such code to be shared in GitPython as well

Don't you agree that methods like clone that are expecting a path/url https://github.com/gitpython-developers/GitPython/blob/17ff2630af26b37f82ac1158ee3495c4390da699/git/repo/base.py#L1219 should always treat those parameters as such? This is, if you pass --foo and that's interpreted as an option, that is unexpected/wrong.

[Byron]

I agree, and now I understand what the prior comment is about as well. And it seems not too unlikely that url and path could be used to construct a command-line which uses --upload-pack='attack vector'. Once URL is a little more validated, that should get harder as well. Using --upload-pack is only possible with the ssh scheme and I'd expect only personal-use grade servers to expose more than just git-upload-pack and git-receive-pack in their PATH.

I think once it can be demonstrated that one can indeed execute programs on the server by manipulating the URL or maybe even the URL + destination path, that would be an issue specific to GitPython and worth its own issue.

[stsewd]

I think once it can be demonstrated that one can indeed execute programs on the server by manipulating the URL or maybe even the URL + destination path, that would be an issue specific to GitPython and worth its own issue.

That's already possible, and with normal repos, not only ssh repos. The command is executed on the machine that executes gitpython.

import git
r = git.Repo.init('/tmp/test', bare=True)
r.clone("--upload-pack=touch /tmp/pwn")

[Byron]

That I find really bad, and even though GitPython doesn't do word splitting, such an argument is neatly parsed by git leading to the execution of a program. As it stands, that wouldn't be caught by #1516 either. Maybe we should have a separate issue for that - please feel free to create one if you agree @stsewd .

[stsewd]

@Byron https://github.com/gitpython-developers/GitPython/issues/1517

[Byron]

The library for git is on the way, it's called gitoxide and I will find a way to transition GitPython to using gitoxide's python bindings one day, when these exists. I definitely can't maintain GitPython for another decade if it stays like this 😅.

[mcepl]

I did want to offer a lamentation that git doesn't have a C library

Well, it does (https://libgit2.org/), doesn’t it?

[Byron]

I assume @blade2005 meant that there is no native git library that git itself would be using, hence git provides an API via the command-line exclusively. libgit2 is a separate effort which, by now, does not provide feature and performance parity with git itself anymore.

[nrpt-m]

@Byron Could you please confirm if this PR #1518 is fixing CVE-2022-24439 issue as it has been added to 3.1.30 ?

[stsewd]

@nrpt-m that PR fixes a similar vulnerability which doesn't require passing any extra options to multi_options to be able to exploit it, https://github.com/gitpython-developers/GitPython/pull/1521 will fix the vulnerability when users allow passing any options to multi_options.

[Byron]

@nrpt-m I second that.

[Byron]

A new release was created to incorporate many security related fixes.

A special thanks goes to @stsewd who was a driving force behind implementing the fixes, and to the fine folks who discovered it.

I hope this makes the upcoming year 2023 a little more secure for everyone 🎉.

[ajakk]

A new release was created to incorporate many security related fixes.

A special thanks goes to @stsewd who was a driving force behind implementing the fixes, and to the fine folks who discovered it.

I hope this makes the upcoming year 2023 a little more secure for everyone tada.

Can a release be made in Github? I imagine a nonzero number of people are watching for releases in this repository who aren't subscribed to this issue to be aware of the security impact of this release.

[Byron]

Thanks for the hint. That should be done now.