GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||
GeezerVison is showing github.com — Fix two remaining Windows untrusted search path cases by EliahKagan · Pull Request #1792 · gitpython-developers/GitPython Conversation
|
|
I hope it's okay that I've made a small edit to the advisory to fix some confusing and slightly inaccurate wording that I had missed when first writing it. I'd be pleased to make any further changes if desired (or even undo this change if for some reason it is not considered beneficial). I've applied the change already at the advisory on this repository. For the global advisory, I cannot apply the change directly; I've submitted github/advisory-database#3290. I've noticed that these changes may be hard to spot quickly in the public web-based interface, so I've also made a gist whose revision history shows the proposed changes. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks a lot, and since the first time I wasn't able to suggest improvements, now that it is further improved, I am even more out of my ability to do so. I am sure, however, that the one reviewing the advisory PR will come to the same conclusion and merge the improvements. Thanks again for your relentless pursuit of excellence! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The global advisory has been successfully updated. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
On Windows, Python's subprocess module contains constants useful to pass to the `creationflags` parameter of subprocess.Popen. These are absent on other platforms, where they are not meaningful. The code was already safe at runtime from any AttributeError related to these constants, because they were only used in git.cmd._safer_popen_windows, which was defined on Windows. But in gitpython-developers#1792 I did not write the code in a way where mypy can verify its correctness. So if a regression of the kind mypy can in principle catch were to occur, it would be harder to notice it. This refactors the code, keeping the same behavior but expressing it in a way mypy can understand. This consists of two changes: 1. Only define _safer_popen_windows when the platform is Windows, placing it in the first branch of the `if` statement. This is needed because mypy will not take the only current call to that nonpublic function being on Windows as sufficient evidence that the platform is always Windows when it is run. 2. Determine the platform, for this purpose, using sys.platform instead of os.name. These are far from equivalent in general (see the deprecation rationale for is_<platform> in gitpython-developers#1732, revised in a0fa2bd in gitpython-developers#1787). However, in Python 3 (GitPython no longer supports Python 2), in the specific case of Windows, we have a choice of which to use, as both `sys.platform == "win32"` and `os.name == "nt"`. os.name is "nt" on native Windows, and "posix" on Cygwin. sys.platform is "win32" on native Windows (including 64-bit systems with 64-bit Python builds), and "cygwin" on Cygwin. See: https://docs.python.org/3/library/sys.html#sys.platform This is needed because the type stubs for the subprocess module use this sys.platform check (rather than an os.name check) to determine if the platform is Windows for the purpose of deciding which constants to say the subprocess module defines. I have verified that neither of these changes is enough by itself.
In the USE_SHELL docstring: - Restore the older wording "when executing git commands" rather than "to execute git commands". I've realized that longer phrase, which dates back to the introduction of USE_SHELL in 1c2dd54, is clearer, because readers less familiar with GitPython's overall design and operation will still not be misled into thinking USE_SHELL ever affects whether GitPython uses an external git command, versus some other mechanism, to do something. - Give some more information about why USE_SHELL = True is unsafe (though further revision or clarification may be called for). - Note some non-obvious aspects of USE_SHELL, that some of the way it interacts with other aspects of GitPython is not part of what is or has been documented about it, and in practice changes over time. The first example relates to gitpython-developers#1792; the second may help users understand why code that enables USE_SHELL on Windows, in addition to being unsafe there, often breaks immediately on other platforms; the third is included so the warnings in the expanded docstring are not interpreted as a new commitment that any shell syntax that may have a *desired* effect in some application will continue to have the same effect in the future. - Cover a second application that might lead, or have led, to setting USE_SHELL to True, and explain what to do instead. In test_successful_default_refresh_invalidates_cached_version_info: - Rewrite the commented explanation of a special variant of that second application, where the usual easy alternatives are not used because part of the goal of the test is to check a "default" scenario that does not include either of them. This better explains why that choice is made in the test, and also hopefully will prevent anyone from thinking that test is a model for another situation (which, as noted, is unlikely to be the case even in unit tests).
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [GitPython](https://togithub.com/gitpython-developers/GitPython) | `==3.1.40` -> `==3.1.41` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. ### GitHub Vulnerability Alerts #### [CVE-2024-22190](https://togithub.com/gitpython-developers/GitPython/security/advisories/GHSA-2mqj-m65w-jghx) ### Summary This issue exists because of an incomplete fix for CVE-2023-40590. On Windows, GitPython uses an untrusted search path if it uses a shell to run `git`, as well as when it runs `bash.exe` to interpret hooks. If either of those features are used on Windows, a malicious `git.exe` or `bash.exe` may be run from an untrusted repository. ### Details Although GitPython often avoids executing programs found in an untrusted search path since 3.1.33, two situations remain where this still occurs. Either can allow arbitrary code execution under some circumstances. #### When a shell is used GitPython can be told to run `git` commands through a shell rather than as direct subprocesses, by passing `shell=True` to any method that accepts it, or by both setting `Git.USE_SHELL = True` and not passing `shell=False`. Then the Windows `cmd.exe` shell process performs the path search, and GitPython does not prevent that shell from finding and running `git` in the current directory. When GitPython runs `git` directly rather than through a shell, the GitPython process performs the path search, and currently omits the current directory by setting `NoDefaultCurrentDirectoryInExePath` in its own environment during the `Popen` call. Although the `cmd.exe` shell will honor this environment variable when present, GitPython does not currently pass it into the shell subprocess's environment. Furthermore, because GitPython sets the subprocess CWD to the root of a repository's working tree, using a shell will run a malicious `git.exe` in an untrusted repository even if GitPython itself is run from a trusted location. This also applies if `Git.execute` is called directly with `shell=True` (or after `Git.USE_SHELL = True`) to run any command. #### When hook scripts are run On Windows, GitPython uses `bash.exe` to run hooks that appear to be scripts. However, unlike when running `git`, no steps are taken to avoid finding and running `bash.exe` in the current directory. This allows the author of an untrusted fork or branch to cause a malicious `bash.exe` to be run in some otherwise safe workflows. An example of such a scenario is if the user installs a trusted hook while on a trusted branch, then switches to an untrusted feature branch (possibly from a fork) to review proposed changes. If the untrusted feature branch contains a malicious `bash.exe` and the user's current working directory is the working tree, and the user performs an action that runs the hook, then although the hook itself is uncorrupted, it runs with the malicious `bash.exe`. Note that, while `bash.exe` is a shell, this is a separate scenario from when `git` is run using the unrelated Windows `cmd.exe` shell. ### PoC On Windows, create a `git.exe` file in a repository. Then create a `Repo` object, and call any method through it (directly or indirectly) that supports the `shell` keyword argument with `shell=True`: ```powershell mkdir testrepo git init testrepo cp ... testrepo git.exe # Replace "..." with any executable of choice. python -c "import git; print(git.Repo('testrepo').git.version(shell=True))" ``` The `git.exe` executable in the repository directory will be run. Or use no `Repo` object, but do it from the location with the `git.exe`: ```powershell cd testrepo python -c "import git; print(git.Git().version(shell=True))" ``` The `git.exe` executable in the current directory will be run. For the scenario with hooks, install a hook in a repository, create a `bash.exe` file in the current directory, and perform an operation that causes GitPython to attempt to run the hook: ```powershell mkdir testrepo cd testrepo git init mv .git/hooks/pre-commit.sample .git/hooks/pre-commit cp ... bash.exe # Replace "..." with any executable of choice. echo "Some text" >file.txt git add file.txt python -c "import git; git.Repo().index.commit('Some message')" ``` The `bash.exe` executable in the current directory will be run. ### Impact The greatest impact is probably in applications that set `Git.USE_SHELL = True` for historical reasons. (Undesired console windows had, in the past, been created in some kinds of applications, when it was not used.) Such an application may be vulnerable to arbitrary code execution from a malicious repository, even with no other exacerbating conditions. This is to say that, if a shell is used to run `git`, the full effect of CVE-2023-40590 is still present. Furthermore, as noted above, running the application itself from a trusted directory is not a sufficient mitigation. An application that does not direct GitPython to use a shell to run `git` subprocesses thus avoids most of the risk. However, there is no such straightforward way to prevent GitPython from running `bash.exe` to interpret hooks. So while the conditions needed for that to be exploited are more involved, it may be harder to mitigate decisively prior to patching. ### Possible solutions A straightforward approach would be to address each bug directly: - When a shell is used, pass `NoDefaultCurrentDirectoryInExePath` into the subprocess environment, because in that scenario the subprocess is the `cmd.exe` shell that itself performs the path search. - Set `NoDefaultCurrentDirectoryInExePath` in the GitPython process environment during the `Popen` call made to run hooks with a `bash.exe` subprocess. These need only be done on Windows. --- ### Release Notes <details> <summary>gitpython-developers/GitPython (GitPython)</summary> ### [`v3.1.41`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.41): - fix Windows security issue [Compare Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.40...3.1.41) The details about the Windows security issue [can be found in this advisory](https://togithub.com/gitpython-developers/GitPython/security/advisories/GHSA-2mqj-m65w-jghx). Special thanks go to [@​EliahKagan](https://togithub.com/EliahKagan) who reported the issue and fixed it in a single stroke, while being responsible for an incredible amount of improvements that he contributed over the last couple of months ❤️. #### What's Changed - Add `__all__` in git.exc by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1719](https://togithub.com/gitpython-developers/GitPython/pull/1719) - Set submodule update cadence to weekly by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1721](https://togithub.com/gitpython-developers/GitPython/pull/1721) - Never modify sys.path by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1720](https://togithub.com/gitpython-developers/GitPython/pull/1720) - Bump git/ext/gitdb from `8ec2390` to `ec58b7e` by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/gitpython-developers/GitPython/pull/1722](https://togithub.com/gitpython-developers/GitPython/pull/1722) - Revise comments, docstrings, some messages, and a bit of code by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1725](https://togithub.com/gitpython-developers/GitPython/pull/1725) - Use zero-argument super() by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1726](https://togithub.com/gitpython-developers/GitPython/pull/1726) - Remove obsolete note in \_iter_packed_refs by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1727](https://togithub.com/gitpython-developers/GitPython/pull/1727) - Reorganize test_util and make xfail marks precise by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1729](https://togithub.com/gitpython-developers/GitPython/pull/1729) - Clarify license and make module top comments more consistent by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1730](https://togithub.com/gitpython-developers/GitPython/pull/1730) - Deprecate compat.is\_<platform>, rewriting all uses by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1732](https://togithub.com/gitpython-developers/GitPython/pull/1732) - Revise and restore some module docstrings by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1735](https://togithub.com/gitpython-developers/GitPython/pull/1735) - Make the rmtree callback Windows-only by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1739](https://togithub.com/gitpython-developers/GitPython/pull/1739) - List all non-passing tests in test summaries by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1740](https://togithub.com/gitpython-developers/GitPython/pull/1740) - Document some minor subtleties in test_util.py by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1749](https://togithub.com/gitpython-developers/GitPython/pull/1749) - Always read metadata files as UTF-8 in setup.py by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1748](https://togithub.com/gitpython-developers/GitPython/pull/1748) - Test native Windows on CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1745](https://togithub.com/gitpython-developers/GitPython/pull/1745) - Test macOS on CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1752](https://togithub.com/gitpython-developers/GitPython/pull/1752) - Let close_fds be True on all platforms by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1753](https://togithub.com/gitpython-developers/GitPython/pull/1753) - Fix IndexFile.from_tree on Windows by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1751](https://togithub.com/gitpython-developers/GitPython/pull/1751) - Remove unused TASKKILL fallback in AutoInterrupt by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1754](https://togithub.com/gitpython-developers/GitPython/pull/1754) - Don't return with operand when conceptually void by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1755](https://togithub.com/gitpython-developers/GitPython/pull/1755) - Group .gitignore entries by purpose by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1758](https://togithub.com/gitpython-developers/GitPython/pull/1758) - Adding dubious ownership handling by [@​marioaag](https://togithub.com/marioaag) in [https://github.com/gitpython-developers/GitPython/pull/1746](https://togithub.com/gitpython-developers/GitPython/pull/1746) - Avoid brittle assumptions about preexisting temporary files in tests by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1759](https://togithub.com/gitpython-developers/GitPython/pull/1759) - Overhaul noqa directives by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1760](https://togithub.com/gitpython-developers/GitPython/pull/1760) - Clarify some Git.execute kill_after_timeout limitations by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1761](https://togithub.com/gitpython-developers/GitPython/pull/1761) - Bump actions/setup-python from 4 to 5 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/gitpython-developers/GitPython/pull/1763](https://togithub.com/gitpython-developers/GitPython/pull/1763) - Don't install black on Cygwin by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1766](https://togithub.com/gitpython-developers/GitPython/pull/1766) - Extract all "import gc" to module level by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1765](https://togithub.com/gitpython-developers/GitPython/pull/1765) - Extract remaining local "import gc" to module level by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1768](https://togithub.com/gitpython-developers/GitPython/pull/1768) - Replace xfail with gc.collect in TestSubmodule.test_rename by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1767](https://togithub.com/gitpython-developers/GitPython/pull/1767) - Enable CodeQL by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1769](https://togithub.com/gitpython-developers/GitPython/pull/1769) - Replace some uses of the deprecated mktemp function by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1770](https://togithub.com/gitpython-developers/GitPython/pull/1770) - Bump github/codeql-action from 2 to 3 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/gitpython-developers/GitPython/pull/1773](https://togithub.com/gitpython-developers/GitPython/pull/1773) - Run some Windows environment variable tests only on Windows by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1774](https://togithub.com/gitpython-developers/GitPython/pull/1774) - Fix TemporaryFileSwap regression where file_path could not be Path by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1776](https://togithub.com/gitpython-developers/GitPython/pull/1776) - Improve hooks tests by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1777](https://togithub.com/gitpython-developers/GitPython/pull/1777) - Fix if items of Index is of type PathLike by [@​stegm](https://togithub.com/stegm) in [https://github.com/gitpython-developers/GitPython/pull/1778](https://togithub.com/gitpython-developers/GitPython/pull/1778) - Better document IterableObj.iter_items and improve some subclasses by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1780](https://togithub.com/gitpython-developers/GitPython/pull/1780) - Revert "Don't install black on Cygwin" by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1783](https://togithub.com/gitpython-developers/GitPython/pull/1783) - Add missing pip in $PATH on Cygwin CI by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1784](https://togithub.com/gitpython-developers/GitPython/pull/1784) - Shorten Iterable docstrings and put IterableObj first by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1785](https://togithub.com/gitpython-developers/GitPython/pull/1785) - Fix incompletely revised Iterable/IterableObj docstrings by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1786](https://togithub.com/gitpython-developers/GitPython/pull/1786) - Pre-deprecate setting Git.USE_SHELL by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1782](https://togithub.com/gitpython-developers/GitPython/pull/1782) - Deprecate Git.USE_SHELL by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1787](https://togithub.com/gitpython-developers/GitPython/pull/1787) - In handle_process_output don't forward finalizer result by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1788](https://togithub.com/gitpython-developers/GitPython/pull/1788) - Fix mypy warning "Missing return statement" by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1789](https://togithub.com/gitpython-developers/GitPython/pull/1789) - Fix two remaining Windows untrusted search path cases by [@​EliahKagan](https://togithub.com/EliahKagan) in [https://github.com/gitpython-developers/GitPython/pull/1792](https://togithub.com/gitpython-developers/GitPython/pull/1792) #### New Contributors - [@​marioaag](https://togithub.com/marioaag) made their first contribution in [https://github.com/gitpython-developers/GitPython/pull/1746](https://togithub.com/gitpython-developers/GitPython/pull/1746) - [@​stegm](https://togithub.com/stegm) made their first contribution in [https://github.com/gitpython-developers/GitPython/pull/1778](https://togithub.com/gitpython-developers/GitPython/pull/1778) **Full Changelog**: gitpython-developers/GitPython@3.1.40...3.1.41 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/googleapis/sdk-platform-java). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.