Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix CVE-2023-41040#1644
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b2d3d01 toa56113eCompareThere 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
Done! |
git/refs/symbolic.py Outdated
| # Make path absolute, resolving any symlinks, and check that we are still | ||
| # inside the repository | ||
| full_ref_path = Path(repodir, str(ref_path)).resolve() | ||
| if Path(repodir).resolve() not in full_ref_path.parents: |
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.
| ifPath(repodir).resolve()notinfull_ref_path.parents: | |
| ifnotfull_ref_path.is_relative_to(Path(repodir).absolute()): |
Usingabsolute() instead ofresolve() 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.
facutuescaSep 5, 2023 • 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.
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,GitPythonwill 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
| with open(os.path.join(repodir, str(ref_path)), "rt", encoding="UTF-8") as fp: | ||
| # Make path absolute, resolving any symlinks, and check that we are still | ||
| # inside the repository | ||
| full_ref_path = Path(repodir, str(ref_path)).resolve() |
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. Sorefs/../foo would be fine, butrefs/../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 toresolve() withos.path.abspath, which normalizes.. but does not resolve symlinks.
Uh oh!
There was an error while loading.Please reload this page.
a96ba17 to5f2caa5Comparegit/refs/symbolic.py Outdated
| @@ -171,7 +172,14 @@ def _get_ref_info_helper( | |||
| tokens: Union[None, List[str], Tuple[str, str]] = None | |||
| repodir = _git_dir(repo, ref_path) | |||
| try: | |||
| with open(os.path.join(repodir, str(ref_path)), "rt", encoding="UTF-8") as fp: | |||
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 howgitoxide (andgit) 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 (seethis validation code as reference).
It would be enough to reject anyref_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 theref_path
This change adds a check during reference resolving to see if itcontains an up-level reference ('..'). If it does, it raises anexception.This fixesCVE-2023-41040, which allows an attacker to access filesoutside the repository's directory.5f2caa5 to65b8c6aCompareThere 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 1109413https://build.opensuse.org/request/show/1109413by user dgarcia + anag+factory- AddCVE-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 forCVE-2023-40590 * FixCVE-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 forCVE-2023-41040[CompareSource](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 isnormal path by [@​CosmosAtlas](https://togithub.com/CosmosAtlas)in[https://github.com/gitpython-developers/GitPython/pull/1645](https://togithub.com/gitpython-developers/GitPython/pull/1645)- FixCVE-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 theirfirst 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 firstcontribution 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 therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository 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>
%5d(https%3a%2f%2frenovatebot.com){kind=link}
Bump gitpython from 3.1.32 to 3.1.35Bumps gitpython from 3.1.32 to 3.1.35.Release notesSourced from gitpython's releases.3.1.35 - a fix forCVE-2023-41040What's ChangedBump actions/checkout from 3 to 4 by @dependabot ingitpython-developers/GitPython#1643Fix 'Tree' object has no attribute '_name' when submodule path is normal path by @CosmosAtlas ingitpython-developers/GitPython#1645FixCVE-2023-41040 by @facutuesca ingitpython-developers/GitPython#1644Only make config more permissive in tests that need it by @EliahKagan ingitpython-developers/GitPython#1648Added test for PR #1645 submodule path by @CosmosAtlas ingitpython-developers/GitPython#1647Fix Windows environment variable upcasing bug by @EliahKagan ingitpython-developers/GitPython#1650New Contributors@CosmosAtlas made their first contribution ingitpython-developers/GitPython#1645@facutuesca made their first contribution ingitpython-developers/GitPython#1644Full Changelog: gitpython-developers/GitPython@3.1.34...3.1.353.1.34 - fix resource leakingWhat's Changedutil: close lockfile after opening successfully by @skshetry ingitpython-developers/GitPython#1639New Contributors@skshetry made their first contribution ingitpython-developers/GitPython#1639Full Changelog: gitpython-developers/GitPython@3.1.33...3.1.34v3.1.33 - with security fixWhat's ChangedWIP Quick doc by @LeoDaCoda ingitpython-developers/GitPython#1608Partial clean up wrt mypy and black by @bodograumann ingitpython-developers/GitPython#1617Disable merge_includes in config writers by @bodograumann ingitpython-developers/GitPython#1618feat: full typing for "progress" parameter in Repo class by @madebylydia ingitpython-developers/GitPython#1634FixCVE-2023-40590 by @EliahKagan ingitpython-developers/GitPython#1636#1566 Creating a lock now uses python built-in "open()" method to work arou… by @HageMaster3108 ingitpython-developers/GitPython#1619New Contributors@LeoDaCoda made their first contribution ingitpython-developers/GitPython#1608@bodograumann made their first contribution ingitpython-developers/GitPython#1617@EliahKagan made their first contribution ingitpython-developers/GitPython#1636@HageMaster3108 made their first contribution ingitpython-developers/GitPython#1619Full Changelog: gitpython-developers/GitPython@3.1.32...3.1.33Commitsc8e303f prepare next release09e1b3d Merge pull request #1650 from EliahKagan/envcase8017421 Merge pull request #1647 from CosmosAtlas/masterfafb4f6 updated docs to better describe testing procedure with new repo9da24d4 add test for submodule path not owned by submodule caseeebdb25 Eliminate duplication of git.util.cwd logicc7fad20 Fix Windows env var upcasing regression7296e5c Make test helper script a file, for readabilityd88372a Add test for Windows env var upcasing regression11839ab Merge pull request #1648 from EliahKagan/file-protocolAdditional commits viewable in compare viewDependabot 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 optionsYou 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 commentedSep 19, 2023
@facutuesca,@Byron, wait but issue is still there. Like |
facutuesca commentedSep 19, 2023 • 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.
@doc-sheet Ah I see. Would resolving abs_repodir=os.path.abspath(repodir)withopen(os.path.join(abs_repodir,str(ref_path)),"rt",encoding="UTF-8")asfp: |
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. |
EliahKagan commentedSep 20, 2023 • 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.
Couldn't the GitPython/git/refs/symbolic.py Lines 171 to 172 ina5a6464
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 Neither is consistently considered absolute by the Python standard library. Neither I am reminded ofsome code I wrote a few months ago, in a different context but also about avoiding directory traversal: path=Path(name)ifpath.is_absolute():# Absolute paths can extract outside the target directory.raisezipfile.BadZipFile(f'archive has absolute path:{name!r}')ifpath.rootorpath.drive:# Non-relative non-absolute paths on Windows can do the same.raisezipfile.BadZipFile(f'archive has non-relative path:{name!r}')if'..'inname:# 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.raisezipfile.BadZipFile(f'archive has name containing "..":{name!r}') That would have to be adapted slightly--for example, we are not raising |
doc-sheet commentedSep 20, 2023
I don't think so. As long as second+ argument to os.path.join starts from |
doc-sheet commentedSep 20, 2023
Maybe there is across-platform solution with commonpath / Path.is_relative_to |
facutuesca commentedSep 20, 2023 • 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.
@EliahKagan@doc-sheet@Byron What about implementing the rules inhttps://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 |
Thanks everyone! GitPython/git/refs/symbolic.py Lines 171 to 172 ina5a6464
It seems the above function could be upgraded to do more complete validation similar tohttps://git-scm.com/docs/git-check-ref-format/ .
|
@Byron@doc-sheet@EliahKagan I have created a PR for this here:#1672 |
EliahKagan commentedSep 22, 2023 • 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.
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 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 |
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 fixesCVE-2023-41040, which allows an attacker to access files outside the repository's directory.
Thiscloses#1638.