Better document IterableObj.iter_items and improve some subclasses by EliahKagan · Pull Request #1780 · gitpython-developers/GitPython (original) (raw)

added 5 commits

December 22, 2023 02:52

Where the behavior is intended.

In the case of an invalid hash (or IOError, which in Python 2 was a subclass of OSError but now is just another name for it), the behavior of just yielding no items may be unintuitive, since on most other errors an exception is raised.

However, examining the code reveals this behavior is clearly intentional. Furthrmore, it may be reasonable for applications to rely on it, and it may be convenient in some situations. For backward compatibility, it probably can't be changed significantly.

This adds tests that show both an error that does raise an error-representing exception -- a well-formed hash not present in the repository raising ValueError with a suitable message -- and an error that silently causes the iterator to yield zero items.

Returning an explicit value from a generator function causes that value to be bound to the value attribute of the StopIteration exception. This is available as the result of "yield from" when it is used as an expression; or by explicitly catching StopIteration, binding the StopIteration exception to a variable, and accessing the attribute. This feature of generators is rarely used.

The return iter([]) statement in Submodule.iter_items uses this feature, causing the resulting StopIteration exception object to have a value attribute that refers to a separate second iterator that also yields no values (gitpython-developers#1779).

From context, this behavior is clearly not the goal; a bare return statement should be used here (which has the same effect except for the value attribute of the StopIteration exception). The code had used a bare return prior to 82b131c (gitpython-developers#1282), when return was changed to return iter([]). That was part of a change that added numerous type annotations. It looks like it was either a mistake, or possibly an attempt to work around an old bug in a static type checker.

This commit extends the test_iter_items_from_invalid_hash test to assert that the value attribute of the StopIteration is its usual default value of None. This commit only extends the test; it does not fix the bug.

This fixes the minor bug where a separate empty iterator was bound to the StopIteration exception raised as a result of returning from the generator function (gitpython-developers#1779).

This change does not cause what exceptions are raised from GitPython code in any situations, nor how many items any iterators yield.

• Fill in the missing part of the explanation of why to favor iter_items over list_items in IterableObj and Iterable (gitpython-developers#1775).

• Move the explanation of how subclasses must treat arguments from the list_items methods to the iter_items methods, because the iter_items methdos are the ones that are abstract and must be overridden by a well-behaved subclass, and also because, since the iter_items methods are preferred for use, they should be the place where less duplicated shared documentation resides.

• Subtantially reword the existing documentation for clarity, especially regarding the signifance of extra args and kwargs.

• Put the iter_items method before (i.e. above) the list_items method (in each of the IterableObj and Iterable classes), because that method is the one that should be used more often, and because it is also now the one with the more detailed docstring.

• Remove and old comment on a return type that said exactly the exact same thing as the annotation.

• In Iterable, note deprecation more consistently (and thus in more places).

• Rewrite the IterableClassWatcher class docstring to explain exactly what that metaclass achieves.

EliahKagan changed the titleBetter document IterableObj.iter_items an slightly improve some subclasses Better document IterableObj.iter_items and improve some subclasses

Dec 22, 2023

This was referenced

Dec 22, 2023

EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request

Dec 23, 2023

This shortens the git.util.Iterable docstrings, removing most of the text duplicated from git.util.IterableObj docstrings and instead referring the reader to the specific corresponding methods in IterableObj. A comment about return types that appeared in both places but really only documented IterableObj is likewise removed from Iterable.

This also reorders the classes, placing IterableObj before Iterable and its associated metaclass. This makes it easier to find, and may help remind users that IterableObj is the class they should use.

These changes build on those in dfee31f (gitpython-developers#1780).

EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request

Dec 24, 2023

Three wrong references in docstrings to list_items methods had accidentally remained: one that was meant to be removed altogether, and two that were meant to be references to iter_items.

This completes those changes, correcting the errors from dfee31f (gitpython-developers#1780) and 2b768c7 (gitpython-developers#1785) so the docstrings make sense and references to further information are to places with that information.

lettuce-bot bot referenced this pull request in lettuce-financial/github-bot-signed-commit

Jan 10, 2024

renovate bot referenced this pull request in allenporter/flux-local

Jan 11, 2024

otc-zuul bot pushed a commit to opentelekomcloud-infra/eyes_on_docs that referenced this pull request

Mar 6, 2024

JoeWang1127 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
==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

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:

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:

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:

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

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.

• If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

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

[ Show hidden characters]({{ revealButtonHref }})