Fix CVE-2023-41040 by facutuesca · Pull Request #1644 · gitpython-developers/GitPython · GitHub

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.

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!

Suggested change

      
                        if Path(repodir).resolve() not in full_ref_path.parents:
          
                        if not full_ref_path.is_relative_to(Path(repodir).absolute()):

Using absolute() instead of resolve() just in case the repodir is a symlink.

Why not resolve the symlink?

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.

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?

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

Thanks for taking a look into this!

I have come up with a couple of suggestions and hope they make sense.

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.

I replaced the calls to resolve() with os.path.abspath, which normalizes .. but does not resolve symlinks.

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?

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.

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

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

@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

    
           if ".." in str(ref_path): 
        
               raise ValueError(f"Invalid reference '{ref_path}'")

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.

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.

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

    
           if ".." in str(ref_path): 
        
               raise ValueError(f"Invalid reference '{ref_path}'")

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.)

facutuesca mentioned this pull request Sep 5, 2023

CVE-2023-41040: Blind local file inclusion #1638

Closed

facutuesca force-pushed the fix-cve-2023-41040 branch from b2d3d01 to a56113e Compare September 5, 2023 08:09

Byron requested changes Sep 5, 2023

View reviewed changes

facutuesca requested a review from Byron September 5, 2023 11:55

stsewd reviewed Sep 5, 2023

View reviewed changes

Byron requested a review from empty September 6, 2023 05:32

Byron requested changes Sep 6, 2023

View reviewed changes

facutuesca force-pushed the fix-cve-2023-41040 branch from a96ba17 to 5f2caa5 Compare September 6, 2023 06:15

facutuesca requested a review from Byron September 6, 2023 06:25

Byron reviewed Sep 6, 2023

View reviewed changes

Byron added this to the v3.1.35 - Bugfixes milestone Sep 6, 2023

Byron added the waiting for feedback label Sep 6, 2023

facutuesca added 2 commits September 6, 2023 22:19

Fix CVE-2023-41040

64ebb9f

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.

Add test for CVE-2023-41040

65b8c6a

facutuesca force-pushed the fix-cve-2023-41040 branch from 5f2caa5 to 65b8c6a Compare September 6, 2023 20:22

facutuesca requested a review from Byron September 6, 2023 20:27

Byron removed the waiting for feedback label Sep 7, 2023

Byron approved these changes Sep 7, 2023

View reviewed changes

Byron merged commit 74e55ee into gitpython-developers:main Sep 7, 2023

facutuesca deleted the fix-cve-2023-41040 branch September 7, 2023 06:26

EliahKagan mentioned this pull request Sep 7, 2023

Fix CVE-2023-40590 #1636

Merged

facutuesca mentioned this pull request Sep 21, 2023

Add more checks for the validity of refnames #1672

Merged

chkno mentioned this pull request Sep 21, 2023

python3Packages.gitpython: 3.1.33 -> 3.1.37 NixOS/nixpkgs#256546

Merged

12 tasks

EliahKagan mentioned this pull request Nov 14, 2023

git.util.rmtree can change permissions outside tree on Unix (chmod traverses symlinks) #1738

Closed

data-sync-user mentioned this pull request Feb 9, 2025

Bump gitpython from 3.1.32 to 3.1.37 mozilla/opmon#168

Open

+                          # 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:

-                          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()

                       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:

	if Path(repodir).resolve() not in full_ref_path.parents:
	if not full_ref_path.is_relative_to(Path(repodir).absolute()):

Fix CVE-2023-41040 by facutuesca · Pull Request #1644 · gitpython-developers/GitPython · GitHub

Uh oh!

Uh oh!

Fix CVE-2023-41040 - #1644

Fix CVE-2023-41040#1644Byron merged 2 commits intogitpython-developers:maingitpython-developers/GitPython:mainfrom trail-of-forks:fix-cve-2023-41040trail-of-forks/GitPython:fix-cve-2023-41040Copy head branch name to clipboard

Conversation

facutuesca commented Sep 5, 2023

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

facutuesca commented Sep 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facutuesca Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

doc-sheet commented Sep 19, 2023

Uh oh!

facutuesca commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Byron commented Sep 20, 2023

Uh oh!

EliahKagan commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doc-sheet commented Sep 20, 2023

Uh oh!

doc-sheet commented Sep 20, 2023

Uh oh!

facutuesca commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Byron commented Sep 21, 2023

Uh oh!

facutuesca commented Sep 21, 2023

Uh oh!

EliahKagan commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

Fix CVE-2023-41040#1644
Byron merged 2 commits into
gitpython-developers:mainfrom
trail-of-forks:fix-cve-2023-41040

facutuesca Sep 5, 2023 •
edited
Loading

facutuesca commented Sep 19, 2023 •
edited
Loading

EliahKagan commented Sep 20, 2023 •
edited
Loading

facutuesca commented Sep 20, 2023 •
edited
Loading

EliahKagan commented Sep 22, 2023 •
edited
Loading