-
-
Notifications
You must be signed in to change notification settings - Fork 954
Fix CVE-2023-41040 #1644
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CVE-2023-41040 #1644
Conversation
b2d3d01 to
a56113e
Compare
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 for the initiative! Could you add some tests that fail if the change is not applied?
Thanks
Thanks for the initiative! Could you add some tests that fail if the change is not applied? Thanks
Done!
git/refs/symbolic.py
Outdated
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.
Using absolute() instead of resolve() just in case the repodir is a symlink.
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.
Why not resolve the symlink?
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.
If repodir is a symlink, then full_ref_path will be able to escape the repodir path.
For example, if repodir is a symlink to /home/secrets/, then full_ref_path can create a symlink to /home/secrets/myscecrets, being able to skip the check.
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.
But repodir is not given by the user, rather it's computed using:
repodir = _git_dir(repo, ref_path)
which returns (if I understand it correctly) the repo's .git folder. Is there a scenario where that folder can be a symlink? Or rather, a malicious symlink?
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 guess my point is: if your .git directory is a symlink to /home/secrets, GitPython will have access to everything under /home/secrets, so no point in protecting against that in this very specific 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 for taking a look into this!
I have come up with a couple of suggestions and hope they make sense.
git/refs/symbolic.py
Outdated
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 think this will slow down each read of a loose ref, and maybe that can be avoided?
References mentioned in a symbolic ref are never absolute, and can be rejected on that ground. Then, when determined relative, one should normalize the ../ away to check if it would break out. So refs/../foo would be fine, but refs/../foo/../../bar would not be. All this can happen without resolving symlinks.
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 replaced the calls to resolve() with os.path.abspath, which normalizes .. but does not resolve symlinks.
a96ba17 to
5f2caa5
Compare
git/refs/symbolic.py
Outdated
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.
When looking at how gitoxide (and git) do it I noticed that this could be much simpler.
Ref-names can't be anything they want, they are very limited in what's allowed and what is not (see this validation code as reference).
It would be enough to reject any ref_path that has a parent-dir component in it.
What do you think?
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.
Ah yeah you're right! I fixed it so that now it only checks for .. in the ref_path
This change adds a check during reference resolving to see if it
contains an up-level reference ('..'). If it does, it raises an
exception.
This fixes CVE-2023-41040, which allows an attacker to access files
outside the repository's directory.
5f2caa5 to
65b8c6a
Compare
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 contributing a CVE-fix!
... via SR 1109413 https://build.opensuse.org/request/show/1109413 by user dgarcia + anag+factory - Add CVE-2023-41040.patch to fix directory traversal attack vulnerability gh#gitpython-developers/GitPython#1644 bsc#1214810 - Update _service to use manualrun, disabledrun is deprecated now. - Update to version 3.1.34.1693646983.2a2ae77: * prepare patch release * util: close lockfile after opening successfully * update instructions for how to create a release * prepare for next release * Skip now permanently failing test with note on how to fix it * Don't check form of version number * Add a unit test for CVE-2023-40590 * Fix CVE-2023-40590 * feat: full typing for "progress" parameter * Creating a lock now uses python built-in "open()" method to work around docker virtiofs issue * Disable merge_includes in config writers * Apply straight-forward typing fixes
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [GitPython](https://togithub.com/gitpython-developers/GitPython) | `==3.1.34` -> `==3.1.35` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>gitpython-developers/GitPython (GitPython)</summary> ### [`v3.1.35`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.35): - a fix for CVE-2023-41040 [Compare Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.34...3.1.35) #### What's Changed - Bump actions/checkout from 3 to 4 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/gitpython-developers/GitPython/pull/1643](https://togithub.com/gitpython-developers/GitPython/pull/1643) - Fix 'Tree' object has no attribute '\_name' when submodule path is normal path by [@​CosmosAtlas](https://togithub.com/CosmosAtlas) in [https://github.com/gitpython-developers/GitPython/pull/1645](https://togithub.com/gitpython-developers/GitPython/pull/1645) - Fix CVE-2023-41040 by [@​facutuesca](https://togithub.com/facutuesca) in [https://github.com/gitpython-developers/GitPython/pull/1644](https://togithub.com/gitpython-developers/GitPython/pull/1644) - Only make config more permissive in tests that need it by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1648](https://togithub.com/gitpython-developers/GitPython/pull/1648) - Added test for PR [#​1645](https://togithub.com/gitpython-developers/GitPython/issues/1645) submodule path by [@​CosmosAtlas](https://togithub.com/CosmosAtlas) in [https://github.com/gitpython-developers/GitPython/pull/1647](https://togithub.com/gitpython-developers/GitPython/pull/1647) - Fix Windows environment variable upcasing bug by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1650](https://togithub.com/gitpython-developers/GitPython/pull/1650) #### New Contributors - [@​CosmosAtlas](https://togithub.com/CosmosAtlas) made their first contribution in [https://github.com/gitpython-developers/GitPython/pull/1645](https://togithub.com/gitpython-developers/GitPython/pull/1645) - [@​facutuesca](https://togithub.com/facutuesca) made their first contribution in [https://github.com/gitpython-developers/GitPython/pull/1644](https://togithub.com/gitpython-developers/GitPython/pull/1644) **Full Changelog**: gitpython-developers/GitPython@3.1.34...3.1.35 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/allenporter/flux-local). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi44My4wIiwidXBkYXRlZEluVmVyIjoiMzYuODMuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
Bump gitpython from 3.1.32 to 3.1.35 Bumps gitpython from 3.1.32 to 3.1.35. Release notes Sourced from gitpython's releases. 3.1.35 - a fix for CVE-2023-41040 What's Changed Bump actions/checkout from 3 to 4 by @dependabot in gitpython-developers/GitPython#1643 Fix 'Tree' object has no attribute '_name' when submodule path is normal path by @CosmosAtlas in gitpython-developers/GitPython#1645 Fix CVE-2023-41040 by @facutuesca in gitpython-developers/GitPython#1644 Only make config more permissive in tests that need it by @EliahKagan in gitpython-developers/GitPython#1648 Added test for PR #1645 submodule path by @CosmosAtlas in gitpython-developers/GitPython#1647 Fix Windows environment variable upcasing bug by @EliahKagan in gitpython-developers/GitPython#1650 New Contributors @CosmosAtlas made their first contribution in gitpython-developers/GitPython#1645 @facutuesca made their first contribution in gitpython-developers/GitPython#1644 Full Changelog: gitpython-developers/GitPython@3.1.34...3.1.35 3.1.34 - fix resource leaking What's Changed util: close lockfile after opening successfully by @skshetry in gitpython-developers/GitPython#1639 New Contributors @skshetry made their first contribution in gitpython-developers/GitPython#1639 Full Changelog: gitpython-developers/GitPython@3.1.33...3.1.34 v3.1.33 - with security fix What's Changed WIP Quick doc by @LeoDaCoda in gitpython-developers/GitPython#1608 Partial clean up wrt mypy and black by @bodograumann in gitpython-developers/GitPython#1617 Disable merge_includes in config writers by @bodograumann in gitpython-developers/GitPython#1618 feat: full typing for "progress" parameter in Repo class by @madebylydia in gitpython-developers/GitPython#1634 Fix CVE-2023-40590 by @EliahKagan in gitpython-developers/GitPython#1636 #1566 Creating a lock now uses python built-in "open()" method to work arou... by @HageMaster3108 in gitpython-developers/GitPython#1619 New Contributors @LeoDaCoda made their first contribution in gitpython-developers/GitPython#1608 @bodograumann made their first contribution in gitpython-developers/GitPython#1617 @EliahKagan made their first contribution in gitpython-developers/GitPython#1636 @HageMaster3108 made their first contribution in gitpython-developers/GitPython#1619 Full Changelog: gitpython-developers/GitPython@3.1.32...3.1.33 Commits c8e303f prepare next release 09e1b3d Merge pull request #1650 from EliahKagan/envcase 8017421 Merge pull request #1647 from CosmosAtlas/master fafb4f6 updated docs to better describe testing procedure with new repo 9da24d4 add test for submodule path not owned by submodule case eebdb25 Eliminate duplication of git.util.cwd logic c7fad20 Fix Windows env var upcasing regression 7296e5c Make test helper script a file, for readability d88372a Add test for Windows env var upcasing regression 11839ab Merge pull request #1648 from EliahKagan/file-protocol Additional commits viewable in compare view Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase. Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: @dependabot rebase will rebase this PR @dependabot recreate will recreate this PR, overwriting any edits that have been made to it @dependabot merge will merge this PR after your CI passes on it @dependabot squash and merge will squash and merge this PR after your CI passes on it @dependabot cancel merge will cancel a previously requested merge and block automerging @dependabot reopen will reopen this PR if it is closed @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the Security Alerts page. Reviewed-by: Vladimir Vshivkov
doc-sheet
commented
Sep 19, 2023
@facutuesca, @Byron, wait but issue is still there.
Because of how os.path.join works it is possible to exploit it with just a passing absolute path.
Like
import git
r = git.Repo(".")
r.commit("/dev/null")
@facutuesca, @Byron, wait but issue is still there. Because of how os.path.join works it is possible to exploit it with just a passing absolute path.
@doc-sheet Ah I see. Would resolving repodir to an absolute path before calling os.path.join be enough to fix it?
abs_repodir = os.path.abspath(repodir) with open(os.path.join(abs_repodir, str(ref_path)), "rt", encoding="UTF-8") as fp:
This would certainly be feasible if the absolute version of a repository path is stored on the repository itself as a sort of cache.
It's definitely something that would need addressing.
CC @EliahKagan
Couldn't the ref_path check in _get_ref_info_helper that this PR added just be made more robust, so it rejects non-relative paths, as well as those that contain ..? Currently we have:
GitPython/git/refs/symbolic.py
Lines 171 to 172 in a5a6464
I think the new cases to reject include absolute paths, but also paths that are neither absolute nor relative. The latter is possible on Windows, where you can have a path like C:a that refers to a in the "current directory" on the C: drive even when C: is not the current drive (see the note on this page), as well as a path like /a or \a that refers to a in the root of the current drive.
Neither is consistently considered absolute by the Python standard library. Neither os.path.isabs nor the pathlib.Path.is_absolute method consider paths like C: or C:a absolute. Interestingly, they disagree on Windows about /a and \a. I don't mean the / and \ versions are treated differently--those are treated the same, that's not the issue. Rather, pathlib.Path is aware such paths are not, in the strictest sense, absolute paths on Windows, while os.path.isabs is treats them as absolute. Paths like C: or C:a, as well as those like /a or \a, have joining behavior similar to that which doc-sheet pointed out true absolute paths have.
I am reminded of some code I wrote a few months ago, in a different context but also about avoiding directory traversal:
path = Path(name) if path.is_absolute(): # Absolute paths can extract outside the target directory. raise zipfile.BadZipFile(f'archive has absolute path: {name!r}') if path.root or path.drive: # Non-relative non-absolute paths on Windows can do the same. raise zipfile.BadZipFile(f'archive has non-relative path: {name!r}') if '..' in name: # A ".." component, or "...", "....", etc. on some systems, can # traverse upward. Because we know what is reasonable in a USC # archive, broadly denying paths with ".." anywhere is okay. raise zipfile.BadZipFile(f'archive has name containing "..": {name!r}')
That would have to be adapted slightly--for example, we are not raising BadZipFile--but I think something like that could be done.
doc-sheet
commented
Sep 20, 2023
Would resolving repodir to an absolute path before calling os.path.join be enough to fix it?
I don't think so. As long as second+ argument to os.path.join starts from / (i don't know about windows) it will replace everything before it.
doc-sheet
commented
Sep 20, 2023
Maybe there is a cross-platform solution with commonpath / Path.is_relative_to
def is_safe_path(basedir, path, follow_symlinks=True):
# resolves symbolic links
if follow_symlinks:
matchpath = os.path.realpath(path)
else:
matchpath = os.path.abspath(path)
return basedir == os.path.commonpath((basedir, matchpath))
@EliahKagan @doc-sheet @Byron What about implementing the rules in https://git-scm.com/docs/git-check-ref-format/ and using them to check reference names? It looks like rules 3, 4, 6 and 10 would cover the problematic behaviors we're talking about, plus more:
(3). They cannot have two consecutive dots .. anywhere.
(4). They cannot have ASCII control characters (i.e. bytes whose values are lower than 040,円 or 177円 DEL), space, tilde ~, caret ^, or colon : anywhere.
(6). They cannot begin or end with a slash / or contain multiple consecutive slashes
(10). They cannot contain a \.
Thanks everyone!
GitPython/git/refs/symbolic.py
Lines 171 to 172 in a5a6464
It seems the above function could be upgraded to do more complete validation similar to https://git-scm.com/docs/git-check-ref-format/ .
gitoxide implements this here, and was itself written based on git's validation code - maybe it can be useful here when implementing a python version.
@Byron @doc-sheet @EliahKagan I have created a PR for this here: #1672
By the way, for the benefit of anyone who comes along to read this, the example in #1644 (comment) may not look like it shows anything, but this is because reading from /dev/null completes immediately and the resulting exception and traceback from GitPython doesn't obviously indicate that this happened. If a path like /dev/zero or /dev/random is passed, it keeps reading and reading, and using system resources, etc. (Thus, you may not want to try that on your system, at least if you're not prepared to kill the process.)
So in a situation where an application that uses GitPython exposes queries on refs in specific local repositories without the intention of allowing reads from elsewhere in the filesystem, using an absolute path rather than a relative one with .. components provided another way to exploit the vulnerability that was not prevented by the original patch. (At least that is my understanding of the situation.)
This change adds a check during reference resolving to see if the requested reference is inside the current repository folder. If it's ouside, it raises an exception.
This fixes CVE-2023-41040, which allows an attacker to access files outside the repository's directory.
This closes #1638.