-
-
Notifications
You must be signed in to change notification settings - Fork 941
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 our terms of service and privacy 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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,6 +26,7 @@ | |||||||||||||||
CommandError, | ||||||||||||||||
GitCommandError, | ||||||||||||||||
GitCommandNotFound, | ||||||||||||||||
UnsafeExecutionError, | ||||||||||||||||
UnsafeOptionError, | ||||||||||||||||
UnsafeProtocolError, | ||||||||||||||||
) | ||||||||||||||||
|
@@ -398,6 +399,7 @@ class Git(metaclass=_GitMeta): | |||||||||||||||
|
||||||||||||||||
__slots__ = ( | ||||||||||||||||
"_working_dir", | ||||||||||||||||
"_safe", | ||||||||||||||||
"cat_file_all", | ||||||||||||||||
"cat_file_header", | ||||||||||||||||
"_version_info", | ||||||||||||||||
|
@@ -944,17 +946,56 @@ def __del__(self) -> None: | |||||||||||||||
self._stream.read(bytes_left + 1) | ||||||||||||||||
# END handle incomplete read | ||||||||||||||||
|
||||||||||||||||
def __init__(self, working_dir: Union[None, PathLike] = None) -> None: | ||||||||||||||||
def __init__(self, working_dir: Union[None, PathLike] = None, safe: bool = False) -> None: | ||||||||||||||||
"""Initialize this instance with: | ||||||||||||||||
:param working_dir: | ||||||||||||||||
Git directory we should work in. If ``None``, we always work in the current | ||||||||||||||||
directory as returned by :func:`os.getcwd`. | ||||||||||||||||
This is meant to be the working tree directory if available, or the | ||||||||||||||||
``.git`` directory in case of bare repositories. | ||||||||||||||||
:param safe: | ||||||||||||||||
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 | ||||||||||||||||
external programs 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 have unknown exploit vectors. It is a best | ||||||||||||||||
effort defense rather than an exhaustive protection measure. | ||||||||||||||||
In order to make this more likely to work with submodules, | ||||||||||||||||
some attempts are made to rewrite remote URLs to ``https://`` | ||||||||||||||||
using `insteadOf` in the config. This might not work on all | ||||||||||||||||
projects, so submodules should always use ``https://`` URLs. | ||||||||||||||||
:envvar:`GIT_TERMINAL_PROMPT` is set to `false` and these | ||||||||||||||||
environment variables are forced to `/bin/true`: | ||||||||||||||||
:envvar:`GIT_ASKPASS`, :envvar:`GIT_EDITOR`, | ||||||||||||||||
:envvar:`GIT_PAGER`, :envvar:`GIT_SSH`, | ||||||||||||||||
:envvar:`GIT_SSH_COMMAND`, and :envvar:`SSH_ASKPASS`. | ||||||||||||||||
Git config options are supplied via the command line to set | ||||||||||||||||
up key parts of safe mode. | ||||||||||||||||
- Direct options for executing external commands are set to ``/bin/true``: | ||||||||||||||||
``core.askpass``, ``core.sshCommand`` and ``credential.helper``. | ||||||||||||||||
- External password prompts are disabled by skipping authentication using | ||||||||||||||||
``http.emptyAuth=true``. | ||||||||||||||||
- Any use of an fsmonitor daemon is disabled using ``core.fsmonitor=false``. | ||||||||||||||||
- Hook scripts are disabled using ``core.hooksPath=/dev/null``. | ||||||||||||||||
It was not possible to cover all config items that might execute an external | ||||||||||||||||
command, for example, ``receive.procReceiveRefs``, | ||||||||||||||||
``uploadpack.packObjectsHook`` and ``remote.<name>.vcs``. | ||||||||||||||||
""" | ||||||||||||||||
super().__init__() | ||||||||||||||||
self._working_dir = expand_path(working_dir) | ||||||||||||||||
self._safe = safe | ||||||||||||||||
self._git_options: Union[List[str], Tuple[str, ...]] = () | ||||||||||||||||
self._persistent_git_options: List[str] = [] | ||||||||||||||||
|
||||||||||||||||
|
@@ -1201,6 +1242,8 @@ def execute( | |||||||||||||||
:raise git.exc.GitCommandError: | ||||||||||||||||
:raise git.exc.UnsafeExecutionError: | ||||||||||||||||
:note: | ||||||||||||||||
If you add additional keyword arguments to the signature of this method, you | ||||||||||||||||
must update the ``execute_kwargs`` variable housed in this module. | ||||||||||||||||
|
@@ -1210,6 +1253,51 @@ def execute( | |||||||||||||||
if self.GIT_PYTHON_TRACE and (self.GIT_PYTHON_TRACE != "full" or as_process): | ||||||||||||||||
_logger.info(" ".join(redacted_command)) | ||||||||||||||||
|
||||||||||||||||
if self._safe: | ||||||||||||||||
if isinstance(command, str) or command[0] != self.GIT_PYTHON_GIT_EXECUTABLE: | ||||||||||||||||
raise UnsafeExecutionError( | ||||||||||||||||
redacted_command, | ||||||||||||||||
f"Only {self.GIT_PYTHON_GIT_EXECUTABLE} can be executed when in safe mode.", | ||||||||||||||||
) | ||||||||||||||||
if shell: | ||||||||||||||||
raise UnsafeExecutionError( | ||||||||||||||||
redacted_command, | ||||||||||||||||
"Command cannot be executed in a shell when in safe mode.", | ||||||||||||||||
) | ||||||||||||||||
Comment on lines
+1262
to
+1266
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This blocks using a shell when the reason the shell would be used is that a We can observe our own direct subprocess invocations (provided that they are done from Python and in the usual manner). Setting it up: import sys
import git
def popen_hook(event, args):
if event == "subprocess.Popen":
print(event, args)
sys.addaudithook(popen_hook)
g = git.Git(".", safe=True) Due to the g.execute(["git", "version"], shell=True) # Blocked as of 07a3889. But this executes a shell, even though it should also decline for the same reason, in the same way, and preferably through the same logic automatically: git.Git.USE_SHELL = True
g.execute(["git", "version"]) # NOT blocked as of 07a3889. (This example is written in such a way that it demonstrates the problem on any platform, not just Windows, though exactly what unintended The cause of the problem is that, at this point in the code, a Lines 1343 to 1349 in 07a3889
Although one option would be to move the new |
||||||||||||||||
config_args = [ | ||||||||||||||||
eighthave marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
"-c", | ||||||||||||||||
"core.askpass=/bin/true", | ||||||||||||||||
"-c", | ||||||||||||||||
"core.fsmonitor=false", | ||||||||||||||||
"-c", | ||||||||||||||||
"core.hooksPath=/dev/null", | ||||||||||||||||
"-c", | ||||||||||||||||
"core.sshCommand=/bin/true", | ||||||||||||||||
"-c", | ||||||||||||||||
"credential.helper=/bin/true", | ||||||||||||||||
"-c", | ||||||||||||||||
"http.emptyAuth=true", | ||||||||||||||||
"-c", | ||||||||||||||||
"protocol.allow=never", | ||||||||||||||||
"-c", | ||||||||||||||||
"protocol.https.allow=always", | ||||||||||||||||
"-c", | ||||||||||||||||
"url.https://bitbucket.org/[email protected]:", | ||||||||||||||||
"-c", | ||||||||||||||||
"url.https://codeberg.org/[email protected]:", | ||||||||||||||||
"-c", | ||||||||||||||||
"url.https://github.com/[email protected]:", | ||||||||||||||||
"-c", | ||||||||||||||||
"url.https://gitlab.com/[email protected]:", | ||||||||||||||||
"-c", | ||||||||||||||||
"url.https://.insteadOf=git://", | ||||||||||||||||
"-c", | ||||||||||||||||
"url.https://.insteadOf=http://", | ||||||||||||||||
"-c", | ||||||||||||||||
"url.https://.insteadOf=ssh://", | ||||||||||||||||
] | ||||||||||||||||
command = [command.pop(0)] + config_args + command | ||||||||||||||||
|
||||||||||||||||
# Allow the user to have the command executed in their working dir. | ||||||||||||||||
try: | ||||||||||||||||
cwd = self._working_dir or os.getcwd() # type: Union[None, str] | ||||||||||||||||
|
@@ -1227,6 +1315,15 @@ def execute( | |||||||||||||||
# just to be sure. | ||||||||||||||||
env["LANGUAGE"] = "C" | ||||||||||||||||
env["LC_ALL"] = "C" | ||||||||||||||||
# Globally disable things that can execute commands, including password prompts. | ||||||||||||||||
if self._safe: | ||||||||||||||||
env["GIT_ASKPASS"] = "/bin/true" | ||||||||||||||||
env["GIT_EDITOR"] = "/bin/true" | ||||||||||||||||
env["GIT_PAGER"] = "/bin/true" | ||||||||||||||||
env["GIT_SSH"] = "/bin/true" | ||||||||||||||||
env["GIT_SSH_COMMAND"] = "/bin/true" | ||||||||||||||||
env["GIT_TERMINAL_PROMPT"] = "false" | ||||||||||||||||
env["SSH_ASKPASS"] = "/bin/true" | ||||||||||||||||
env.update(self._environment) | ||||||||||||||||
if inline_env is not None: | ||||||||||||||||
env.update(inline_env) | ||||||||||||||||
|
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.
To give clearer error messages, I recommend moving this check of the
command
type and value below theif shell
check, and also splitting thiscommand
check into two checks with two separate messages.The reason I recommend doing this after the
if shell
check is that, if a shell would (be attempted to) be used, then the condition here could come outTrue
even forgit
commands. Technically the message is correct in that case, in that the shell is separate from thegit
executable, but I don't think users would understand what was going on.The reasons I recommend splitting this into separate
if isinstance(command, str)
andif command[0] != self.GIT_PYTHON_GIT_EXECUTABLE
checks, with two separate exception messages, are:"git version"
is unambiguously an attempt to rungit
with the argumentversion
. So in that case the message would be incorrect (and very confusing, if someone is using GitPython in a Windows-only application and writingcommand
arguments as strings)."git"
by itself (or whatever other valueGit.GIT_PYTHON_GIT_EXECUTABLE
has been set to), either by accident or intentionally in order to see the summary of subcommands it outputs.