Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Draft: safe mode to disable executing any external programs except git#2029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
eighthave wants to merge1 commit intogitpython-developers:main
base:main
Choose a base branch
Loading
fromeighthave:safe-mode

Conversation

eighthave
Copy link

As described in#2020, here is the core implementation of "safe mode". The core idea is to set up operations so that external programs are not executed bygit. This has been a major source of vulnerabilities.

This means that network connections are limited to HTTPS. As much as possible, this will rewrite remote URLs to HTTPS. This is necessary so that submodules work even when they do not use HTTPS URLs, as long as they are public, HTTPS-accessible repos.

This is a draft to confirm the approach. Then I will follow up and polish everything for merging.

Copy link
Member

@ByronByron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks a lot for the preview - I see now how this can work and like that it seems to be minimally invasive overall.

I assume that the testing will primarily be done by hand for ease of use but hope that some sort of 'practical' test-case could be contributed as well.

git/cmd.py Outdated
'-c', 'http.emptyAuth=true',
'-c', 'protocol.allow=never',
'-c', 'protocol.https.allow=always',
'-c', 'url.https://.insteadOf=ssh://',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There is also configuration related to which filesystem monitor to launch, triggered bygit status.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice catch, I think that should be covered bycore.fsmonitor=false. I also looked atreceive.procReceiveRefs,uploadpack.packObjectsHook, andremote.<name>.vcs but couldn't see how to disable them here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I see, sections that are dependent on actual values are problematic.

@@ -204,6 +208,11 @@ def __init__(
Please note that this was the default behaviour in older versions of
GitPython, which is considered a bug though.
:param safe:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think without exhaustive tests and actually challenging the code we wouldn't want to make the flag seem like a perfect defence.
Somehow I feel thatas safe as possible should rather be a disclaimer about this being a best-effort basis and that the defence is probably not complete, while stating what it focusses on.

eighthave reacted with thumbs up emoji
Copy link
Author

@eighthaveeighthaveMay 27, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Agreed. This is what I have so far:

Lock down the configuration to make it as safe as possible when working with publicly accessible, untrusted repositories. This disables all known options that can run an external program and limits networking to the HTTP protocol via https:// URLs. This might not cover Git config options that were added since this was implemented, or options that might have unknown exploit vectors. It is a best effort defense rather than an exhaustive protection measure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That sounds good! Maybe it makes sense to explicitly mention configuration keys with subsections, as these are particularly hard to pickup.

On the bright side, thinking about it, this would only be a problem if someone else controls the environment or the configuration. Maybe the latter is common for repositories on shared drives?

@eighthave
Copy link
Author

I would like to write tests, I looked around through the test suite, but it still escapes me how to structure these tests. Since these options affect howgit is executed, it seems like it would have to connect to the network? Or is there a test that already replaces the protocol helper, e.g.git-remote-https, to avoid network access?

@Byron
Copy link
Member

It really isn't easy to test it at all, and probably impossible to test it exhaustively. So I am fine admitting defeat on this one, particularly because the tests I could imagine would be so specificand spotty that they barely have any value.

@gcmarx
Copy link
Contributor

if we can only rungit, that could mean we pick#2027 over#2026. I don't think it makes sense to have an allowlist of other executables GitPython could run.@EliahKagan, your thoughts?

@EliahKagan
Copy link
Member

EliahKagan commentedMay 28, 2025
edited
Loading

if we can only rungit, that could mean we pick#2027 over#2026. I don't think it makes sense to have an allowlist of other executables GitPython could run.@EliahKagan, your thoughts?

I haven't reviewed this draft PR in detail, and I also don't know what it will be like and how it will document the "safe mode" feature once it's done. But my first impression is that the changes being proposed here should not directly impose any requirements on whetheris_cygwin_git runs an external subprocess besidesgit:

  • Although the PR title makes it seem like this prevents all external subprocesses exceptgit from being executed, it is otherwise described more specifically to preventgit subprocesses from executing other external subprocesses.
  • If this covers all subprocesses GitPython creates, rather than only those throughgit, then there are already more subprocesses that need to be suppressed:at leastps.
  • If these changes are to be "minimally invasive" as characterized in#2029 (review), then I think they shouldn't impose requirements on code that doesn't operate on any repository or use any repository or URL specific paths or state.

However, that may not be the whole story. Although I don't think "safe mode" should prohibit the use of subprocesses inis_cygwin_git, it may be that they should be avoided in general when not needed for the same reasons. As currently written, it takes some effort to verify thatis_cygwin_git does not introduce an untrusted search path vulnerability.

Furthermore, regarding the use ofuname inis_cygwin_git, while I think it's probably not a vulnerability, it's not really ideal to assume adjacent executables are similarly trusted. It might be considered a valuable security enhancement to avoid assuming that just because an executable has a standard name and resides in the same directory asgit that it is safe or reasonable to execute it.

Copy link
Member

@EliahKaganEliahKagan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I noticed a couple of things related to unusual but plausiblecommand arguments.

if self._safe:
if isinstance(command, str):
command = [command]
config_args = [
Copy link
Member

@EliahKaganEliahKaganMay 28, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Theseconfig_args passing command-scope Git configuration variables via-c are only meaningful if the executable being run isgit or otherwise recognizesgit options. But thisGit.execute method is not limited to runninggit. It is intentionally usable to run any command in an environment that theGit object would rungit in, in a manner similar to how it would rungit.

Even if I am right to think, as expressed in#2029 (comment), that "safe mode" need not inherently preclude other subprocess invocations from occurring so long as they are not happening throughgit, it seems to me that users may intuitively assume that the protections apply to all use ofexecute when "safe mode" is enabled and not overridden in any way. It also seems like that is part of the intent here.

Furthermore, sinceexecute uses state associated with the repository--itscwd in particular--I think the risks "safe mode" is meant to mitigate can only be mitigated if its protections apply to allexecute calls for which "safe mode" is considered to be in effect. And ifexecute is used to run something that itself runsgit, such as a script ofgit commands, then it could be a problem for the necessary command-scope configuration not to be in effect.

It seems to me that a simple and effective solution to the problem thatgit options could be passed to programs other thangit is therefore for "safe mode" to enforce, not merely thatcommand is the right kind of object (see above), but also that its first element is exactly what it would be whenexecute is called through_call_process--that is, that the first element is equal toGit.GIT_PYTHON_GIT_EXECUTABLE.


If for some reason it is necessary in "safe mode" forexecute to run other programs besides thegit executable that_call_process would use, then it might be possible to pass the command-scope configuration entirely using environment variables, either always or in the case that the command is notGit.GIT_PYTHON_GIT_EXECUTABLE.

However, I think this would be challenging, both in general because of the increased complexity it would entail, and more specifically because, of the two ways to pass command-scope Git configuration variables using environment variables:

  • GIT_CONFIG_{COUNT,KEY,VALUE} is not recognized by all versions ofgit in current use (some distributions maintain old versions with downstream security patches but not new features).
  • GIT_CONFIG_PARAMETERS is considered an implementation detail ofgit with two different syntaxes so fardepending on thegit version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It seems to me that a simple and effective solution to the problem that git options could be passed to programs other than git is therefore for "safe mode" to enforce, not merely that command is the right kind of object (see above), but also that its first element is exactly what it would be when execute is called through _call_process--that is, that the first element is equal to Git.GIT_PYTHON_GIT_EXECUTABLE.

Thanks for this detailed analysis! I agree with this approach. I'll try implementing it. I was not aware that GitPython is executing other things besidesgit. Makes sense to me that safe mode should also restrict those as well.

@eighthave
Copy link
Author

FYI this is my very simple test suite:

#!/usr/bin/python3importosimportgitimporttempfileforurlin ['git@gitlab.com:fdroid/ci-test-app.git','ssh://gitlab.com/fdroid/ci-test-app.git','git://gitlab.com/fdroid/ci-test-app.git',]:print('====================',url)d=tempfile.mkdtemp(prefix='foo_py_')repo=git.Repo.clone_from(url,d,safe=True)# clone from fake URL should prompt for passwordd=tempfile.mkdtemp(prefix='foo_py_')forurlin ['https://github.com/asdfasdfasdf/adsfasdfasdf.git','https://gitlab.com/asdfasdfasdf/adsfasdfasdf.git','https://codeberg.org/asdfasdfasdf/adsfasdfasdf.git',]:try:repo=git.Repo.clone_from(url,d,safe=True)exceptgit.exc.GitCommandErrorase:print('repo',e)url='https://gitlab.com/fdroid/ci-test-app.git'd=tempfile.mkdtemp(prefix='foo_py_')print(d)repo=git.Repo.init(d,safe=True)origin=repo.create_remote("origin",url)origin.fetch()d=tempfile.mkdtemp(prefix='foo_py_')print(d)repo=git.Repo.clone_from(url,d,safe=True)print(type(repo))print('SUCCESS')

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ByronByronByron left review comments

@EliahKaganEliahKaganEliahKagan requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@eighthave@Byron@gcmarx@EliahKagan

[8]ページ先頭

©2009-2025 Movatter.jp