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

[EliahKagan]

Overview

On Unix-like systems, git.util.rmtree will change permissions outside the tree being removed, under some circumstances. This occurs specifically with git.util.rmtree; it is not a flaw inherited from shutil.rmtree. It does not require a race condition, and the effect is easy to produce if one has control of the files in a directory that git.util.rmtree will later be used to remove, though this must include control over file permissions (or related metadata) somewhere within that tree. It could happen unintentionally as well as intentionally, but I believe unintentional occurrence is uncommon, at least in uses of git.util.rmtree from within GitPython.

This happens because, when git.util.rmtree cannot delete a file, it calls os.chmod(path, stat.S_IWUSR) on the file and tries again. But os.chmod dereferences symlinks when called that way on a Unix-like system. The effect is to change permissions on the target, which may be outside the tree. It adds write permissions, and removes some other permissions. Yet, while this seems like a case of CWE-59, I'm somewhat reluctant to consider this a security vulnerability, because it only adds permissions for the file's owner, who is already capable of using chmod to add them. But there are some other concerns, detailed below in "Is this a vulnerability?" This bug should not be confused with race conditions affecting rmtree on some systems that can cause the wrong files to be deleted; this bug is about the wrong files having their permissions changed.

I believe this is straightforward to fix, with no breaking changes. Although useful on Windows, on Unix-like systems that os.chmod call is unnecessary and, in cases where permissions do need to be changed to allow a file to be deleted, ineffective. So it can be run conditionally, on Windows only. Its ineffectiveness on Unix-like systems is thus a blessing in disguise, as otherwise making it run only on Windows could be a breaking change. I have proposed this fix, and regression tests, in #1739.

The cause

On all operating systems, git.util.rmtree currently calls shutil.rmtree with a callback that attempts to change a file's permissions to be writable and retry deletion:

GitPython/git/util.py

Lines 208 to 214 in 74cc671

	def handler(function: Callable, path: PathLike, _excinfo: Any) -> None:
	"""Callback for :func:`shutil.rmtree`. Works either as ``onexc`` or ``onerror``."""
	# Is the error an access error?
	os.chmod(path, stat.S_IWUSR)
	try:
	function(path)

The intent of the os.chmod call is to make the file at path writable. The problem is that, on Unix-like systems, calling os.chmod without passing follow_symlinks=False only changes the file at path when it is not a symbolic link. When the file at path is a symbolic link, it is dereferenced, and its (direct or indirect) target has its permissions changed instead.

os.chmod behaves this way on Unix-like systems because, on such systems, unless follow_symlinks=False is passed, os.chmod uses chmod(2), which follows symlinks:

chmod() changes the mode of the file specified whose pathname is given in pathname, which is dereferenced if it is a symbolic link.

That link is to a Linux manpage, but this holds across most, if not all, Unix-like systems. The macOS chmod(2) manpage is less explicit about this, but as noted in "Steps to reproduce," I have verified this bug on macOS as well as GNU/Linux.

In contrast, os.chmod with follow_symlink=False, or os.lchmod, use lchmod(2), and do not deference symlinks. However, they are not available on all systems. For example, Linux has no such system call.

This bug does not affect Windows. Although Windows has symbolic links, os.chmod does not dereference them on Windows. See "Conceptual examination" below for details.

Steps to reproduce

General approach

As detailed below in "Why this os.chmod call only helps on Windows", there are various scenarios in which rmtree will fail to delete a file on a Unix-like system, but the easiest to produce, which is also probably the most common in practice, is that the user lacks write permissions on the containing directory. If that directory contains a symbolic link to a file outside of it, then the failure to delete the directory will trigger an os.chmod affecting that out-of-tree file, in the callback git.util.rmtree passes to shutil.rmtree. So the steps, in brief, are:

1. Create a directory, which will be passed to git.util.rmtree.
2. Create a symbolic link in that directory, whose target is outside the directory. To observe the bug, the permissions of the target should not already be 0200, because that is what they will be changed to as a result of the bug.
3. Use chmod -w on the directory.
4. Pass the directory's name to git.util.rmtree.
5. Observe that the permissions on the symbolic link's target have been changed, even though it is outside the directory being deleted.

Script to reproduce the bug easily

More concretely, I have verified on Ubuntu 22.04.3 LTS and macOS 12.6.9 that the bug can be triggered by creating an executable script perm.sh with the contents (also in this gist, for convenience):

#!/bin/sh

set -e

mkdir dir1
touch dir1/file
chmod -w dir1/file
printf 'Permissions BEFORE rmtree call:\n'
ls -l dir1/file
printf '\n'

mkdir dir2
ln -s ../dir1/file dir2/symlink
chmod -w dir2
python -c 'from git.util import rmtree; rmtree("dir2")' || true
printf '\nPermissions AFTER rmtree call:\n'
ls -l dir1/file

Then run the script to see how attempting to delete dir2 with git.util.rmtree changes the permissions of dir1/file, even though dir1/file is not inside dir2:

$ ./perm.sh
Permissions BEFORE rmtree call:
-r--r--r-- 1 ek ek 0 Oct 30 05:13 dir1/file

Traceback (most recent call last):
  File "/usr/lib/python3.12/shutil.py", line 695, in _rmtree_safe_fd
    os.unlink(entry.name, dir_fd=topfd)
PermissionError: [Errno 13] Permission denied: 'symlink'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/ek/repos-wsl/GitPython/git/util.py", line 212, in rmtree
    shutil.rmtree(path, onexc=handler)
  File "/usr/lib/python3.12/shutil.py", line 769, in rmtree
    _rmtree_safe_fd(fd, path, onexc)
  File "/usr/lib/python3.12/shutil.py", line 697, in _rmtree_safe_fd
    onexc(os.unlink, fullname, err)
  File "/home/ek/repos-wsl/GitPython/git/util.py", line 203, in handler
    function(path)
PermissionError: [Errno 13] Permission denied: 'dir2/symlink'

Permissions AFTER rmtree call:
--w------- 1 ek ek 0 Oct 30 05:13 dir1/file

git.util.rmtree still failed with a PermissionError, because the reason it can't delete dir2/symlink is due to the permissions on dir2 itself, which it does not modify. But before it retries and fails, it has already changed the permissions of that symbolic link's target, dir1/file, from 0444 to 0200.

Conceptual examination

The above demonstrates the bug in GitPython itself, but I think it is also clarifying to see the behavior of os.chmod itself on various systems (or at least, I found this useful myself when investigating the bug).

Unix-like systems - affected

On a Unix-like system (as above, I also tested this on Ubuntu 22.04.3 LTS and macOS 12.6.9), run:

$ touch file
$ chmod -w file
$ ln -s file symlink
$ ls -l
total 0
-r--r--r-- 1 ek ek 0 Nov 13 11:51 file
lrwxrwxrwx 1 ek ek 4 Nov 13 11:52 symlink -> file

$ python3.12 -c 'import os, stat; os.chmod("symlink", stat.S_IWUSR)'
$ ls -l
total 0
--w------- 1 ek ek 0 Nov 13 11:57 file
lrwxrwxrwx 1 ek ek 4 Nov 13 11:57 symlink -> file

The core concept can alternatively be demonstrated, on the same systems, by the chmod(3) command, which also uses chmod(2), by replacing the second part above with:

$ chmod 200 symlink
$ ls -l
total 0
--w------- 1 ek ek 0 Nov 13 11:51 file
lrwxrwxrwx 1 ek ek 4 Nov 13 11:52 symlink -> file

(One would replace python3.12 with one's actual Python command, if different. This is not limited to Python 3.12.)

Windows - unaffected

In contrast, on Windows 10 (with developer mode enabled), in PowerShell 7:

> New-Item -Path file -ItemType File | Out-Null
> Set-ItemProperty -Path file -Name IsReadOnly -Value $true
> New-Item -Path symlink -ItemType SymbolicLink -Value file | Out-Null
> Get-ChildItem

    Directory: C:\Users\ek\tmp

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-ar--          11/13/2023 12:42 PM              0 file
la---          11/13/2023 12:42 PM              0 symlink -> file

> python3.12 -c 'import os, stat; os.chmod("symlink", stat.S_IWUSR)'
> Get-ChildItem

    Directory: C:\Users\ek\tmp

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-ar--          11/13/2023 12:42 PM              0 file
la---          11/13/2023 12:42 PM              0 symlink -> file

(That is on a system where Python was installed from the Windows Store. If Python was installed from an .exe file downloaded from python.org, then one must usually replace python3.12 with py -3.12. As on Unix-like systems, one may change the version number as well.)

Observe that file is still read-only, even after operating on symlink with os.chmod. In contrast--though not shown above--changing "symlink" to "file" in the Python command does work, changing the displayed mode from -ar-- to -a--- (and allowing Remove-Item to delete the file).

Is this a vulnerability?

Disclosure considerations (click to expand, if interested)

My interpretation of the security policy is that it does not express a preference against opening a public bug report like this one in this situation, but I would be pleased to receive feedback in this area. The specific factors I considered in deciding to report the bug this way were that (a) it might be a security bug, but (b) this project's security policy does not request strict coordinated vulnerability disclosure, (c) I am unaware of a likely method to exploit it, (d) I have been, perhaps unwisely, developing and testing the fix on a public branch of my fork before thinking deeply about its security implications, so I may have, in effect, already disclosed the issue publicly, (e) I believe my fix and tests are complete, and by reporting publicly I can offer them in a way that is either faster or much more convenient to review than if I disclosed by email and waited to open a PR, (f) I may soon be unavailable to contribute for a few days or weeks, and if I take the time to research exploitability further, the effects of d would be exacerbated and the benefits of e diminished, and (g) the note in CVE-2023-40590 suggests to me that the security policy does not intend to discourage public disclosure that is based on such factors.

Outside of its tests, GitPython's only use of git.util.rmtree is to delete submodules. This sounds concerning, since cloning an untrusted repository, including its submodules, and doing git operations in it, without running code in it, should ideally be safe. Such a repository could certainly have arbitrary symbolic links in a submodule that are checked out into its working tree, including symlinks to absolute paths, or relative paths like ../.., that would cause upward traversal. Calling os.chmod on such symlinks, in the way git.util.rmtree does, would change permissions on the target (if owned by the user running the process) to 0200, adding write permissions for the owning user, and removing all other permissions.

This is assuming, however, that a failure during rmtree occurs. As discussed below, I believe this is hard for an attacker to produce deliberately on any affected system. Without it, the callback, and thus its os.chmod call, is never run.

For the problem that write permissions are added, it is a major mitigation that they are added only for the owner. But this could nonetheless contribute to a loss of integrity if, for example, a non-robust script is employed that goes by access alone to mutate some files in a directory but not others.

I believe a larger concern is actually the permissions that are removed. In particular, if the untrusted symlink is to a directory outside the working tree, the read permission needed to list the directory and the executable permission needed to traverse into the directory are removed. Therefore, if an attacker who does not already have access to the system but controls the contents of an untrusted repository that is locally cloned with submodules can cause shutil.rmtree to fail when called by git.util.rmtree, then the attacker can carry out a denial of service attack.

But I do not know of a way a malicious submodule author could deliberately bring this about, because:

• On Windows, where deletion fails if a file is open, I believe it is not rare for git.util.rmtree to fail to perform an operation and call its callback. But Windows is unaffected by this bug. I believe a failure of an operation attempted during rmtree is actually rare on Unix-like systems (outside of GitPython's own tests, some of which deliberately produce it). So simply waiting for it to happen may not be realistic.
• Git tracks file modes, but only in a limited way, distinguishing between symlinks and regular files, and tracking executable permissions. If executable permissions are absent on a directory, it cannot be traversed and rmtree would fail, but the dangerous symlink would not be reached. More importantly, git does not track directories themselves, so the file modes it stores do not directly create directories that cannot be traversed.
• If an attacker can trick the user into running a chmod, chattr, or chflags command, then it can easily be done. But running such commands from a malicious source is already unsafe with or without GitPython. It is probably easier for an attacker to fool a user into running them on a file inside a cloned repository than elsewhere, but then the attacker could urge the user to run chmod directly on the cloned symlink.

A secondary concern is that this could lead to denial of service against an application that uses GitPython but intends to expose only some of the capabilities of the user account running it. At least as I understand it, that was the chief concern in CVE-2023-41040 (#1638), which was considered a vulnerability. But in that case, there were specific, identifiable git operations an application could be requested to perform that would trigger an unexpected access to another part of the filesystem and resource exhaustion. In this case, I am unaware of such a condition. However, I admit such a problem might be discovered with further research.

Even if GitPython's use of git.util.rmtree to remove submodules is not to be considered vulnerable, git.util.rmtree is public, being listed in __all__. So it may be (or have been) a reasonable design choice for an application to make direct use of it, which some applications may be doing in an inadvertently vulnerable way. This would not be limited to situations involving submodules.

Why this os.chmod call only helps on Windows

On Windows, attempting to change a file's permissions to be writable and retrying deletion of the file can help, because of how file permissions and deletion interact on Windows: attempting to delete a read-only file will fail on Windows, even if the user could delete the file if it were not read-only. The os.chmod call in git.util.rmtree appears to have been introduced specifically to handle that situation on Windows.

In contrast, on a Unix-like system, it probably can never help. On the one hand, the widespread belief that access to a file's containing directory, and never the file itself, is all that is relevant to the ability to delete a file on a Unix-like system... is actually wrong on some Unix-like systems. There are specific circumstances, on some Unix-like systems, where a user gaining write access to a file they cannot currently delete, with no other changes, would cause the user to be able to delete it. Nonetheless, the os.chmod call used in git.util.rmtree does not help in those situations either. Roughly speaking, on Unix-like systems:

• If the filesystem is mounted read-only, then obviously the file cannot be removed regardless of its permissions.
• If the user cannot traverse into the containing directory (due to lacking executable permissions on the directory or its ancestors), the user cannot delete the file. But a file that can't be traversed to is never arrived at in shutil.rmtree and therefore is not passed to the callback git.util.rmtree passes shutil.rmtree.
• If the user lacks write access to the directory, the user cannot delete a file in it, and adding write permissions to the file will not help.
• Some operating systems, including the most popular Unix-like operating systems (those based on Linux or Darwin), support additional attributes or other flags--separate from Unix permissions and access control lists--that can render a file "immutable" in such a way that causes deletion to fail. See chattr and chflags. But this is separate from permissions, so adding write permissions to the file wouldn't help.
• Otherwise, if the user has write access to the containing directory, and that directory does not have the sticky bit set or the user owns the directory, then the user can delete the file. This is the case whether or not the file is writable, so adding write permissions to the file does not help, but also is not attempted.
• If, instead, the directory (which the user has write access to) does have the sticky bit set, then on most Unix-like systems, the user's ability to delete the file is contingent on file ownership. Typically, the user can delete the file if the user owns the directory or--more often relevant--owns the file. This allows users to create and delete their own files in locations like /tmp but not delete other users' files there. But this is where things get tricky. On a minority of Unix-like systems, including some that continue in significant use today such as Solaris (and illumos systems such as OpenIndiana), having write access to the file suffices as an alternative to owning it (see chmod(2), as cited in the "Solaris 11" row of this table). I have verified this behavior on OpenIndiana 2023.10.

The reason the os.chmod call in git.util.rmtree is nonetheless ineffective at allowing such a file to be deleted is that setting S_IWUSR with os.chmod on a Unix-like system only confers write permissions to the file's owner. But owning the file is already sufficient to allow deletion when one has write access on the containing sticky directory.

I only say "probably" because of the possibility that some system may facilitate expressing strange access restrictions that would make deleting a file one owns contingent on having write access to it on a Unix-like system. I don't think ACLs would express this, but perhaps something like AppArmor could be used to achieve it. If this were done, I don't think whoever did it would expect GitPython or any other software to take special advantage of it, and I would be inclined to think that would still not make the removal of that os.chmod call on any Unix-like system a breaking change in GitPython.

Alternative solutions

This last section covers why I believe skipping the os.chmod call when not running on Windows (as proposed in #1739) is better than the other possible solutions that I am aware of.

I believe it would be worthwhile to skip the os.chmod call outside Windows even if this symlink-related bug were absent and the only problem with the call outside Windows were its ineffectiveness, because:

• As detailed above, the likelihood that it is helping on any non-Windows system is very low.
• Given that the git.util.rmtree docstring explains the distinction between shutil.rmtree and git.util.rmtree in terms of Windows behavior, the os.chmod call does not appear to have been intended to have any important effect outside of Windows.

However, even if having git.util.rmtree attempt to change file permissions on non-Windows systems were to be considered worthwhile, fixing that os.chmod call to do so safely would be nontrivial, because:

• Calling os.chmod with follow_symlinks=False is not supported on all systems. This includes popular Unix-like systems. For example, os.chmod in os.supports_follow_symlinks evaluates to False on my Ubuntu system (and other Linux-based systems).
• The os.lchmod function is likewise not present on all systems.
• A similar situation exists with the pathlib alternatives Path.chmod with follow_symlinks=False and Path.lchmod. These are also not available on all currently supported versions of Python.
• If I understand correctly, Unix-like systems on which os.chmod does not support follow_symlinks may not have any atomic way to change permissions without dereferencing a symlink. So checking and then performing the operation risks the file being replaced by a symlink at the last moment. Even if the risk is small, the reward, in this case, is surely smaller.

The other alternative that may seem appealing is to attempt to actually solve plausible causes of a PermissionError on a Unix-like system, by making the logic more expansive and complicated, changing write permissions on containing directories, adding executable permissions to directories during traversal to reach and delete files in them, unsetting immutable flags/attributes on systems that support those, and so forth. But:

• git.util.rmtree doesn't seem to need to do this--as far as I can tell, the only known problems with it failing to delete files in actual GitPython use are on Windows. So based on complexity alone, it might not be worthwhile.
• Getting it right is hard, and some ways of getting it wrong would be security bugs, due to added race conditions or otherwise.
• File permissions and other metadata that can stymie file deletion exist for a reason, and in some uses a user may intend them to prevent deletion, possibly including of a file in a submodule's working tree. So at least in the absence of known real-world cases where git.util.rmtree is seen to benefit from working around them, it would be difficult to know that doing so would really be an overall improvement.

Such logic, if added, might somewhat resemble TemporaryDirectory._rmtree, but while I think TemporaryDirectory with its cleanup logic is reasonable to use in GitPython's unit tests, I think any logic along those lines in git.util.rmtree might have to differ substantially from it to avoid security-related problems. I cite it not to recommend the technique, but instead as a rough guess at the lower bound of how much more code it might take to do it.

[Byron]

Thank you for this, once again, incredibly thorough analysis that seems to leave nothing I can think of to chance.

Reading I have verified this behavior on OpenIndiana 2023.10. is particularly stunning, as most people probably don't have access to such a system and at least have to install a VM to try it, yet you do even that!

[EliahKagan]

Thank you for this, once again, incredibly thorough analysis that seems to leave nothing I can think of to chance.

Thanks! I'd say I've tried to characterize what is left to chance, in terms of the impact of the bug. But in terms of fixing it without introducing a breaking change, I do think the approach argued for here and merged in #1739 of doing the chmod-and-retry only on Windows is safe.

Reading I have verified this behavior on OpenIndiana 2023.10. is particularly stunning, as most people probably don't have access to such a system and at least have to install a VM to try it, yet you do even that!

I'm flattered to hear you say that. I did use a VM to run it. Ironically, I found that much easier than testing on macOS, even though macOS is far more common. In part this was because it was fairly late in the process that I realized I really should test on macOS, since my understanding of its manpages was not necessarily correct. (It happened to be correct, in this case; I'm still glad I checked.) But in part it was that I have no good way to run macOS, which in some way doesn't play well with VMs where the host runs on non-Apple hardware, or so I recall from the last time I looked into it. I ended up finding a cloud-based way.

[Byron]

Oh, it's true! GitPython doesn't even run CI on MacOS (yet) - otherwise I would have suggested to just use these machines for testing. What I tend to do is to enable CI on a fork to be able to use it privately outside of a PR. Maybe one day some tests will also be run on MacOS, for completeness, and I'd expect them to just work the way they do on Linux.

[EliahKagan]

I've opened #1752 to add macOS CI test jobs.