Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork939
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
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
| '-c', 'http.emptyAuth=true', | ||
| '-c', 'protocol.allow=never', | ||
| '-c', 'protocol.https.allow=always', | ||
| '-c', 'url.https://.insteadOf=ssh://', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| env['GIT_TERMINAL_PROMPT'] = 'false' | ||
| env['GIT_ASKPASS'] = '/bin/true' | ||
| env['SSH_ASKPASS'] = '/bin/true' | ||
| env['GIT_SSH'] = '/bin/true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think there is alsoGIT_SSH_PROGRAM, and there are probably more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, thanks,GIT_SSH_COMMAND. I just went throughhttps://git-scm.com/book/en/v2/Git-Internals-Environment-Variables and picked out all the relevant ones.
| @@ -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: | |||
There was a problem hiding this comment.
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.
eighthaveMay 27, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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 how |
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. |
if we can only run |
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 by
git. 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.