Block insecure options and protocols by default by stsewd · Pull Request #1521 · gitpython-developers/GitPython (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits7 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This got a little longer than expected 😮💨, there were other places where git accepted ext:: URLs, like git pull/push/fetch <URL> https://git-scm.com/docs/git-remote-ext#_examples
And there are other config options that can be harmful, so I think we should just forbid the --config option, if anyone is relying on that option, they can opt-out with allow_unsafe_options=True.
--*-pack and --exec are the options that I found that could lead to RCE, but anyone allowing users to pass arbitrary options should be aware that it may be more of these, don't know.
This is still missing adding/updating tests.
This is on top of #1516
Fixes #1515
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for having put in so much time and effort to help fixing this!
It mostly looks good to me, and once CI is working there shouldn't be much in the way of merging the PR.
I have added/updated the tests.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much, this is tremendous work and great value for GitPython and all of its users. If you would like more recognition for this, please feel free to add an entry to changes.rst and include your name if you want. The same goes for the authors file.
That said, and if you feel you have a little more time and patience, I generally thought that asserting that these pwn files exist or don't exists where it's easily possible would help readability a lot while adding some assurance that it's actually, effectively working like it should. After all, having an exception raised is a side-effect of us ideally stopping the git command to be executed, but we don't really know unless we fail to observe its side-effect that we are trying to prevent.
If you don't have time, that's alright as well, just let me know and I will merge as is and get a new release ready.
Thanks so much!
Thanks so much, this is tremendous work and great value for GitPython and all of its users. If you would like more recognition for this, please feel free to add an entry to changes.rst and include your name if you want. The same goes for the authors file.
Thank you!
If you don't have time, that's alright as well, just let me know and I will merge as is and get a new release ready.
I'll try to update the PR later today or tomorrow
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request
3.1.30
- Make injections of command-invocations harder or impossible for clone and others. See gitpython-developers/GitPython#1518 for details. Note that this might constitute a breaking change for some users, and if so please let us know and we add an opt-out to this.
- Prohibit insecure options and protocols by default, which is potentially a breaking change, but a necessary fix for gitpython-developers/GitPython#1515. Please take a look at the PR for more information and how to bypass these protections in case they cause breakage: gitpython-developers/GitPython#1521.
halstead pushed a commit to openembedded/openembedded-core that referenced this pull request
stefan-hartmann-lgs pushed a commit to hexagon-geo-surv/poky that referenced this pull request
All versions of package gitpython are vulnerable to Remote Code Execution (RCE) due to improper user input validation, which makes it possible to inject a maliciously crafted remote URL into the clone command. Exploiting this vulnerability is possible because the library makes external calls to git without sufficient sanitization of input arguments.
CVE: CVE-2022-24439
Upstream-Status: Backport
Reference: gitpython-developers/GitPython#1529 gitpython-developers/GitPython#1518 gitpython-developers/GitPython#1521
(From OE-Core rev: 55f93e3786290dfa5ac72b5969bb2793f6a98bde)
Signed-off-by: Narpat Mali narpat.mali@windriver.com Signed-off-by: Richard Purdie richard.purdie@linuxfoundation.org
jpuhlman pushed a commit to MontaVista-OpenSourceTechnology/poky that referenced this pull request
Source: poky MR: 124663 Type: Integration Disposition: Merged from poky ChangeID: 0721360 Description:
All versions of package gitpython are vulnerable to Remote Code Execution (RCE) due to improper user input validation, which makes it possible to inject a maliciously crafted remote URL into the clone command. Exploiting this vulnerability is possible because the library makes external calls to git without sufficient sanitization of input arguments.
CVE: CVE-2022-24439
Upstream-Status: Backport
Reference: gitpython-developers/GitPython#1529 gitpython-developers/GitPython#1518 gitpython-developers/GitPython#1521
(From OE-Core rev: 55f93e3786290dfa5ac72b5969bb2793f6a98bde)
Signed-off-by: Narpat Mali narpat.mali@windriver.com Signed-off-by: Richard Purdie richard.purdie@linuxfoundation.org Signed-off-by: Jeremy A. Puhlman jpuhlman@mvista.com
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
The tests of unsafe options are among those introduced originally in gitpython-developers#1521. They are regression tests for gitpython-developers#1515 (CVE-2022-24439). The unsafe options tests are paired: a test for the usual, default behavior of forbidding the option, and a test for the behavior when the option is explicitly allowed. Both tests use a payload that is intended to produce the side effect of a file of a specific name being created in a temporary directory.
All the tests work on Unix-like systems. On Windows, the tests of the allowed cases are broken, and this commit marks them xfail. However, this has implications for the tests of the default, secure behavior, because until the "allowed" versions work on Windows, it will be unclear if either are using a payload that is effective and that corresponds to the way its effect is examined. (Fortunately, all are working on other OSes, and the affected code under test does not appear highly dependent on OS, so the fix is probably fully working on Windows as well.)
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
The tests of unsafe options are among those introduced originally in gitpython-developers#1521. They are regression tests for gitpython-developers#1515 (CVE-2022-24439). The unsafe options tests are paired: a test for the usual, default behavior of forbidding the option, and a test for the behavior when the option is explicitly allowed. Both tests use a payload that is intended to produce the side effect of a file of a specific name being created in a temporary directory.
All the tests work on Unix-like systems. On Windows, the tests of the allowed cases are broken, and this commit marks them xfail. However, this has implications for the tests of the default, secure behavior, because until the "allowed" versions work on Windows, it will be unclear if either are using a payload that is effective and that corresponds to the way its effect is examined. (Fortunately, all are working on other OSes, and the affected code under test does not appear highly dependent on OS, so the fix is probably fully working on Windows as well.)
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
The tests of unsafe options are among those introduced originally in gitpython-developers#1521. They are regression tests for gitpython-developers#1515 (CVE-2022-24439). The unsafe options tests are paired: a test for the usual, default behavior of forbidding the option, and a test for the behavior when the option is explicitly allowed. In each such pair, both tests use a payload that is intended to produce the side effect of a file of a specific name being created in a temporary directory.
All the tests work on Unix-like systems. On Windows, the tests of the allowed cases are broken, and this commit marks them xfail. However, this has implications for the tests of the default, secure behavior, because until the "allowed" versions work on Windows, it will be unclear if either are using a payload that is effective and that corresponds to the way its effect is examined. (Fortunately, all are working on other OSes, and the affected code under test does not appear highly dependent on OS, so the fix is probably fully working on Windows as well.)
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
The tests of unsafe options are among those introduced originally in gitpython-developers#1521. They are regression tests for gitpython-developers#1515 (CVE-2022-24439). The unsafe options tests are paired: a test for the usual, default behavior of forbidding the option, and a test for the behavior when the option is explicitly allowed. In each such pair, both tests use a payload that is intended to produce the side effect of a file of a specific name being created in a temporary directory.
All the tests work on Unix-like systems. On Windows, the tests of the allowed cases are broken, and this commit marks them xfail. However, this has implications for the tests of the default, secure behavior, because until the "allowed" versions work on Windows, it will be unclear if either are using a payload that is effective and that corresponds to the way its effect is examined.
Specifically, the "" characters in the path seem to be treated as escape characters rather than literally. Also, "touch" is not a native Windows command, and the "touch" command in Git for Windows maps disallowed occurrences of ":" in filenames to a separate code point in the Private Use Area of the Basic Multilingual Plane.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
The tests of unsafe options are among those introduced originally in gitpython-developers#1521. They are regression tests for gitpython-developers#1515 (CVE-2022-24439). The unsafe options tests are paired: a test for the usual, default behavior of forbidding the option, and a test for the behavior when the option is explicitly allowed. In each such pair, both tests use a payload that is intended to produce the side effect of a file of a specific name being created in a temporary directory.
All the tests work on Unix-like systems. On Windows, the tests of the allowed cases are broken, and this commit marks them xfail. However, this has implications for the tests of the default, secure behavior, because until the "allowed" versions work on Windows, it will be unclear if either are using a payload that is effective and that corresponds to the way its effect is examined.
What seems to happen is this: The "" characters in the path are treated as shell escape characters rather than literally, with the effect of disappearing in most paths since most letters lack special meaning when escaped. Also, "touch" is not a native Windows command, and the "touch" command provided by Git for Windows is linked against MSYS2 libraries, causing it to map (some?) occurrences of ":" in filenames to a separate code point in the Private Use Area of the Basic Multilingual Plane. The result is a path with no directory separators or drive letter. It denotes a file of an unintended name in the current directory, which is never the intended location. The current directory depends on GitPython implementation details, but at present it's the top-level directory of the rw_repo working tree. A new unstaged file, named like "C\357\200\272UsersekAppDataLocalTemptmpc7x4xik5pwn", can be observed there (this is how "git status" will format the name).
Fortunately, this and all related tests are working on other OSes, and the affected code under test does not appear highly dependent on OS. So the fix is probably fully working on Windows as well.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
The tests of unsafe options are among those introduced originally in gitpython-developers#1521. They are regression tests for gitpython-developers#1515 (CVE-2022-24439). The unsafe options tests are paired: a test for the usual, default behavior of forbidding the option, and a test for the behavior when the option is explicitly allowed. In each such pair, both tests use a payload that is intended to produce the side effect of a file of a specific name being created in a temporary directory.
All the tests work on Unix-like systems. On Windows, the tests of the allowed cases are broken, and this commit marks them xfail. However, this has implications for the tests of the default, secure behavior, because until the "allowed" versions work on Windows, it will be unclear if either are using a payload that is effective and that corresponds to the way its effect is examined.
What seems to happen is this: The "" characters in the path are treated as shell escape characters rather than literally, with the effect of disappearing in most paths since most letters lack special meaning when escaped. Also, "touch" is not a native Windows command, and the "touch" command provided by Git for Windows is linked against MSYS2 libraries, causing it to map (some?) occurrences of ":" in filenames to a separate code point in the Private Use Area of the Basic Multilingual Plane. The result is a path with no directory separators or drive letter. It denotes a file of an unintended name in the current directory, which is never the intended location. The current directory depends on GitPython implementation details, but at present it's the top-level directory of the rw_repo working tree. A new unstaged file, named like "C\357\200\272UsersekAppDataLocalTemptmpc7x4xik5pwn", can be observed there (this is how "git status" will format the name).
Fortunately, this and all related tests are working on other OSes, and the affected code under test does not appear highly dependent on OS. So the fix is probably fully working on Windows as well.