Replace some uses of the deprecated mktemp function by EliahKagan · Pull Request #1770 · gitpython-developers/GitPython
added 2 commits
December 12, 2023 20:42The tempfile.mktemp function is deprecated, because of a race condition where the file may be concurrently created between when its name is generated and when it is opened. Other faciliies in the tempfile module overcome this by generating a name, attempting to create the file or directory in a way that guarantees failure if it already existed, and, in the occasional case that it did already exist, generating another name and trying again (stopping after a predefined limit). For further information on mktemp deprecation: - https://docs.python.org/3/library/tempfile.html#tempfile.mktemp - gitpython-developers/smmap#41 The security risk of calls to mktemp in this project's test suite is low. However, it is still best to avoid using it, because it is deprecated, because it is (at least slightly) brittle, and because any use of mktemp looks like a potential security risk and thereby imposes a burden on working with the code (which could potentially be addressed with detailed comments analyzing why it is believed safe in particular cases, but this would typically be more verbose, and at least as challenging to add, as replacing mktemp with a better alternative). This commit replaces *some* uses of mktemp in the test suite: those where it is readily clear how to do so in a way that preserves the code's intent: - Where a name for a temporary directory is generated with mktemp and os.mkdir is called immediately, mkdtemp is now used. - Where a name for a temporary file that is not customized (such as with a prefix) is generated with mktemp, such that the code under test never uses the filename but only the already-open file-like object, TemporaryFile is now used. As the name isn't customized, the test code in these cases does not express an intent to allow the developer to inspect the file after a test failure, so even if the file wasn't guaranteed to be deleted with a finally block or context manager, it is fine to do so. TemporaryFile supports this use case well on all systems including Windows, and automatically deletes the file. - Where a name for a temporary file that *is* customized (such as with a prefix) to reflect the way the test uses it is generated with mktemp, and the test code does not attempt deterministic deletion of the file when an exception would make the test fail, NamedTemporaryFile with delete=False is now used. The original code to remove the file when the test succeeds is modified accordingly to do the same job, and also commented to explain that it is being handled this way to allow the file to be kept and examined when a test failure occurs. Other cases in the test suite should also be feasible to replace, but are left alone for now. Some of them are ambiguous in their intent, with respect to whether the file should be retained after a test failure. Others appear deliberately to avoid creating a file or directory where the code under test should do so, possibly to check that this is done properly. (One way to preserve that latter behavior, while avoiding the weakness of using mktemp and also avoiding inadverently reproducing that weakness by other means, could be to use a path in a temporary directory made for the test.) This commit also doesn't address the one use of mktemp in the code under test (i.e., outside the test suite, inside the git module).
This makes two related changes to git.index.util.TemporaryFileSwap: - Replace mktemp with mkstemp and then immediately closing the file. This avoids two possible name clashes: the usual name clash where the file may be created by another process between when mktemp generates the name and when the file is created, and the problem that mktemp does not check for files that contain the generated name as a part. The latter is specific to the use here, where a string generated by mktemp was manually concatenated to the base filename. This addresses that by passing the filename as the prefix for mkstemp to use. - Replace os.remove with os.replace and simplify. This is made necessary on Windows by the file already existing, due to mkstemp creating it. Deleting the file and allowing it to be recreated would reproduce the mktemp race condition in full (obscured and with extra steps). But os.replace supports an existing target file on all platforms. It is now also used in __exit__, where it allows the Windows check for the file to be removed, and (in any OS) better expresses the intent of the code to human readers. Furthermore, because one of the "look before you leap" checks in __exit__ is removed, the minor race condition in cleaning up the file is somewhat decreased. A small amount of related refactoring is included. The interface is not changed, even where it could be simplified (by letting __exit__ return None), and resource acquisition remains done on construction rather than in __enter__, because changing those aspects of the design could break some existing uses. Although the use of mktemp replaced here was in the git module and not the test suite, its use was to generate filenames for use in a directory that would ordinarily be under the user's control, such that the ability to carry out typical mktemp-related attacks would already require the ability to achieve the same goals more easily and reliably. Although TemporaryFileSwap is a public class that may be used directly by applications, it is only useful for making a temporary file in the same directory as an existing file. Thus the intended benefits of this change are mainly to code quality and a slight improvement in robustness.
This was referenced
Dec 13, 2023EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Dec 21, 2023This is a general test for TemporaryFileSwap, but by being parametrized by the type of file_path, it reveals a regression introduced in 9e86053 (gitpython-developers#1770). TemporaryFileSwap still works when file_path is a string, but is now broken when it is a Path. That worked before, and the type annotations document that it should be able to work. This is at least a bug because TemporaryFileSwap is public. (I am unsure whether, in practice, GitPython itself uses it in a way that sometimes passes a Path object as file_path. But code that uses GitPython may call it directly and pass Path.)
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Dec 21, 2023This fixes the regression introduced in 9e86053 (gitpython-developers#1770) where the file_path argument to TemporaryFileSwap.__init__ could no longer be a Path object. The change also makes this truer to the code from before gitpython-developers#1770, still without the race condition fixed there, in that str was called on file_path then as well. However, it is not clear that this is a good thing, because this is not an idiomatic use of mkstemp. The reason the `prefix` cannot be a Path is that it is expected to be a filename prefix, with leading directories given in the `dir` argument.
This was referenced
Dec 21, 2023lettuce-bot bot referenced this pull request in lettuce-financial/github-bot-signed-commit
Jan 10, 2024EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Feb 29, 2024This expands the comment added in 41fac85 (gitpython-developers#1770) to make more clear that this particular cleanup is deliberately done only when the operation was successful (and why).
otc-zuul bot pushed a commit to opentelekomcloud-infra/eyes_on_docs that referenced this pull request
Mar 6, 2024JoeWang1127 referenced this pull request in googleapis/sdk-platform-java
Apr 6, 2024[](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-->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters