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.
git/cmd.py Outdated
'-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.
Uh oh!
There was an error while loading.Please reload this page.
@@ -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 |
EliahKagan commentedMay 28, 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.
However, that may not be the whole story. Although I don't think "safe mode" should prohibit the use of subprocesses in Furthermore, regarding the use of |
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 noticed a couple of things related to unusual but plausiblecommand
arguments.
Uh oh!
There was an error while loading.Please reload this page.
if self._safe: | ||
if isinstance(command, str): | ||
command = [command] | ||
config_args = [ |
EliahKaganMay 28, 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.
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.
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.
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.
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') |
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.