Fix two remaining Windows untrusted search path cases by EliahKagan · Pull Request #1792 · gitpython-developers/GitPython (original) (raw)

This fixes CVE-2024-22190.

Although #1636 caused GitPython to avoid using an untrusted search path on Windows under the most typical circumstances, it turns out that some cases remained. I believe only two remain at this time. This pull request fixes those. The CI status on all commits appears to be as I expected and intended.

This vulnerability was reported privately, initially without a full public writeup, since I wanted to get the pull request out as coordinated/requested, without extra delays. A summary of the vulnerability this fixes is now available in the CVE-2024-22190 advisory. Further information about these changes is present in the commit messages. For the benefit of future readers, I have also since added some more information below, lightly adapted from my original disclosure.

I have found that the fix for CVE-2023-40590 (#1635) that I contributed in #1636 was unfortunately incomplete, missing two cases.

When git is run with a shell, setting the NoDefaultCurrentDirectoryInExePath environment variable in the caller's environment is insufficient, because the shell subprocess is what performs the path search. When a shell is used, the child process's environment needs to have that variable set. This does not happen automatically, because while the env dict is based on os.environ, it is created before NoDefaultCurrentDirectoryInExePath is set in the caller environment. Omitting that variable from the child process's environment was intentional, to avoid modifying the operation of child processes more than necessary. (git itself does not need that variable to be set to avoid looking in the current directory to run its own subprocesses, including custom commands.) But it has to be done when a shell is used.

In addition, when GitPython uses bash.exe to run hooks, it does not do anything to avoid finding and running bash.exe in the current directory, which may be the working tree of an untrusted repository. Because in some reasonable workflows one checks out a branch from an untrusted remote to review it, and such a branch could contain a malicious bash.exe, this is exploitable. The description of CVE-2023-40590 had mentioned "And there are other commands executed that should probably be aware of this problem," but I had not given enough attention to that point when working on the fix, nor recognized its full ramifications.

Historically there have been other places in GitPython where a subprocess was created insecurely on Windows, but I believe that is no longer the case. The call to taskkill was removed (#1754), and a call to ps does not occur on Windows because it is part of the kill_after_timeout feature of Git.execute (#1756, #1761). This is not true of the test suite, but when running tests, a more trusted environment can reasonably be assumed: people trust the directory out of which they are intentionally running the test suite, and I don't think any test cases attempt git operations directly in /tmp anymore, as opposed to a subdirectory (#1744, #1759). Therefore I believe these are the only places that need to be changed. I searched for the names of all subprocess module functions and I did not find other problematic calls.

The shell=True bug was not detected before, due to a combination of two flaws in the regression tests I wrote in #1636 for CVE-2023-40590. First, I did not commit a test with shell=True. Second, although I had also tried it locally with shell=True, the test contained a shortcoming that prevented it from revealing the vulnerability in that case. It entered a new temporary directory with an impostor git.exe in it. But when shell=True, the relevant CWD is that of the cmd.exe shell subprocess, which was the rorepo working tree rather than the GitPython process's CWD. My patch clarifies the test, creating a repository explicitly so that all the test logic is plain, and it parameterizes the test to cover substantially more cases, including when the devious git.exe is in the repository and git is run with a shell.

I have likewise undertaken efforts to make the newly introduced bash.exe impostor test robust, tried it out with each of the WinBashStatus cases, and set up the impostor scenario in a way that allows the effect to be observed whether or not a real bash.exe is available and whether or not it is usable.

To avoid code duplication, make intent clearer, and make it easier to avoid introducing new untrusted search path bugs, this patch extracts both the shell and non-shell logic for preventing such bugs to a safer_popen function that can be used like Popen with no special forethought. This function also documents the relevant issues, including the strange but important distinction on Windows between shell and non-shell executable search behavior (i.e., why facilities such as shutil.which or the external where command often don't do what we need, on Windows). Having this in one place should also aid in future improvements to the technique used, such as to mitigate or eliminate the race condition on os.environ that irwand commented about in #1650. On non-Windows systems, safer_popen is simply an alias for Popen.

Because Git.USE_SHELL = True was useful for suppressing undesired console windows in graphical applications on Windows (#126, 1c2dd54, #360) prior to 2.0.8 (0d93908, #469), and is only much more recently longer documented as recommended for that (#1781, #1782, #1787), it is likely that a significant minority of applications use it on Windows. Searching on GitHub, and the web more broadly, turns up some uses and makes me think this may be so. In contrast, I doubt many people intentionally use GitPython to run hooks. But this happens automatically if it is used in a repository with a pre-commit hook enabled.