Overview

On Unix-like systems,git.util.rmtree will change permissionsoutside the tree being removed, under some circumstances. This occurs specifically withgit.util.rmtree; it is not a flaw inherited fromshutil.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 thatgit.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 ofgit.util.rmtree from within GitPython.

This happens because, whengit.util.rmtree cannot delete a file, itcallsos.chmod(path, stat.S_IWUSR) on the file and tries again.Butos.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 ofCWE-59, I'msomewhat reluctant to consider this a security vulnerability, because it only adds permissions for the file's owner, who is already capable of usingchmod to add them. But there are some other concerns, detailed below in "Is this a vulnerability?" This bug should not be confused withrace conditions affectingrmtree 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 thatos.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 callsshutil.rmtree with a callback that attempts to change a file's permissions to be writable and retry deletion:

The intent of theos.chmod call is to make the file atpath writable. The problem is that, on Unix-like systems, callingos.chmod without passingfollow_symlinks=False only changes the file atpath when it is not a symbolic link. When the file atpath 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, unlessfollow_symlinks=False is passed,os.chmod useschmod(2), which follows symlinks:

chmod() changes the mode of the file specified whose pathname is given inpathname, 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 withfollow_symlink=False, oros.lchmod, use lchmod(2), and do not deference symlinks. However, they arenot available on all systems. For example, Linux has no such system call.

This bug does not affect Windows. Although Windowshas 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 thisos.chmod call only helps on Windows", there are various scenarios in whichrmtree 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 anos.chmod affecting that out-of-tree file, in the callbackgit.util.rmtree passes toshutil.rmtree. So the steps, in brief, are:

1. Create a directory, which will be passed togit.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 shouldnot already be 0200, because that is what they will be changed to as a result of the bug.
3. Usechmod -w on the directory.
4. Pass the directory's name togit.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 scriptperm.sh with the contents (alsoin this gist, for convenience):

#!/bin/shset -emkdir dir1touch dir1/filechmod -w dir1/fileprintf'Permissions BEFORE rmtree call:\n'ls -l dir1/fileprintf'\n'mkdir dir2ln -s ../dir1/file dir2/symlinkchmod -w dir2python -c'from git.util import rmtree; rmtree("dir2")'||trueprintf'\nPermissions AFTER rmtree call:\n'ls -l dir1/file

Then run the script to see how attempting to deletedir2 withgit.util.rmtree changes the permissions ofdir1/file, even thoughdir1/file is not insidedir2:

$ ./perm.shPermissions BEFORE rmtree call:-r--r--r-- 1 ek ek 0 Oct 30 05:13 dir1/fileTraceback (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 aPermissionError, because the reason it can't deletedir2/symlink is due to the permissions ondir2 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 ofos.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 -ltotal 0-r--r--r-- 1 ek ek 0 Nov 13 11:51 filelrwxrwxrwx 1 ek ek 4 Nov 13 11:52 symlink -> file

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

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

$ chmod 200 symlink$ ls -ltotal 0--w------- 1 ek ek 0 Nov 13 11:51 filelrwxrwxrwx 1 ek ek 4 Nov 13 11:52 symlink -> file

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

Windows - unaffected

In contrast, on Windows 10 (withdeveloper 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\tmpMode                 LastWriteTime         Length Name----                 -------------         ------ -----ar--          11/13/2023 12:42 PM              0 filela---          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\tmpMode                 LastWriteTime         Length Name----                 -------------         ------ -----ar--          11/13/2023 12:42 PM              0 filela---          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 replacepython3.12 withpy -3.12. As on Unix-like systems, one may change the version number as well.)

Observe thatfile is still read-only, even after operating onsymlink withos.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 allowingRemove-Item to delete the file).

Is this a vulnerability?

Outside of its tests, GitPython's only use ofgit.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. Callingos.chmod on such symlinks, in the waygit.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 duringrmtree 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 itsos.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 nonethelesscontribute 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 areremoved. 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 causeshutil.rmtree to fail when called bygit.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 forgit.util.rmtree to fail to perform an operation and call its callback. But Windows is unaffected by this bug. Ibelieve a failure of an operation attempted duringrmtree 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 andrmtree 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 achmod,chattr, orchflags 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 runchmod 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 leastas I understand it, that was the chief concern inCVE-2023-41040 (#1638), which was considered a vulnerability. But in that case, there were specific, identifiablegit 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 ofgit.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 thisos.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. Theos.chmod call ingit.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 actuallywrong 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, theos.chmod call used ingit.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 traversedto is never arrived at inshutil.rmtree and therefore is not passed to the callbackgit.util.rmtree passesshutil.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. Seechattr andchflags. But this is separate frompermissions, 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 thesticky 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 fileownership. 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 (seechmod(2), as cited in the "Solaris 11" row ofthis table). I have verified this behavior on OpenIndiana 2023.10.

The reason theos.chmod call ingit.util.rmtree is nonetheless ineffective at allowing such a file to be deleted is that settingS_IWUSR withos.chmod on a Unix-like system only confers write permissionsto 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 thatos.chmod call on any Unix-like system a breaking change in GitPython.

Alternative solutions

This last section covers why I believe skipping theos.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 theos.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 thegit.util.rmtree docstringexplains the distinction betweenshutil.rmtree andgit.util.rmtree in terms of Windows behavior, theos.chmod call does not appear to have been intended to have any important effect outside of Windows.

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

• Callingos.chmod withfollow_symlinks=False isnot supported on all systems. This includes popular Unix-like systems. For example,os.chmod in os.supports_follow_symlinks evaluates toFalse on my Ubuntu system (and other Linux-based systems).
• Theos.lchmod function is likewise not present on all systems.
• A similar situation exists with thepathlib alternativesPath.chmod withfollow_symlinks=False andPath.lchmod. These are also not available on all currently supported versions of Python.
• If I understand correctly, Unix-like systems on whichos.chmod does not supportfollow_symlinks may not have anyatomic 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 aPermissionError 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 wheregit.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, mightsomewhat resembleTemporaryDirectory._rmtree, but while I thinkTemporaryDirectory with its cleanup logic is reasonable to use in GitPython's unit tests, I think any logic along those lines ingit.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 thelower bound of how much more code it might take to do it.