Fix CVE-2023-41040

[facutuesca]

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.

[Byron]

Thanks for the initiative! Could you add some tests that fail if the change is not applied?
Thanks

[facutuesca]

Thanks for the initiative! Could you add some tests that fail if the change is not applied? Thanks

Done!

[stsewd]

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.

[facutuesca]

Why not resolve the symlink?

[stsewd]

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.

[facutuesca]

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?

[facutuesca]

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

[Byron]

Thanks for taking a look into this!

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

[Byron]

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.

[facutuesca]

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

[Byron]

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?

[facutuesca]

Ah yeah you're right! I fixed it so that now it only checks for .. in the ref_path

[Byron]

Thanks a lot for contributing a CVE-fix!

[doc-sheet]

@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]

@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:

[Byron]

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

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

[doc-sheet]

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]

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

[facutuesca]

@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 \.

[Byron]

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.

[facutuesca]

@Byron @doc-sheet @EliahKagan I have created a PR for this here: #1672

[EliahKagan]

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