Security of .git/config and .git/hooks

Security of .git/config and .git/hooks

[Jonathan Nieder]

Hi,

This topic has been mentioned on this mailing list before but I had
trouble finding a relevant reference.  Links welcome.

Suppose that I add the following to .git/config in a repository on a
shared computer:

	[pager]
		log = rm -fr /
		fsck = rm -fr /

("rm -fr /" is of course a placeholder here.)

I then tell a sysadmin that git commands are producing strange output
and I am having trouble understanding what is going on.  They may run
"git fsck" or "git log"; in either case, the output is passed to the
pager I configured, allowing me to run an arbitrary command using the
sysadmin's credentials.

You might say that this is the sysadmin's fault, that they should have
read through .git/config before running any Git commands.  But I don't
find it so easy to blame them.

A few related cases that might not seem so dated:

 1. I put my repository in a zip file and ask a Git expert to help me
    recover data from it, or

 2. My repository is in a shared directory on NFS.  Unless the
    administrator setting that up is very careful, it is likely that
    the least privileged user with write access to .git/config or
    .git/hooks/ may be someone that I don't want to be able to run
    arbitrary commands on behalf of the most privileged user working
    in that repository.

A similar case to compare to is Linux's "perf" tool, which used to
respect a .perfconfig file from the current working directory.
Fortunately, nowadays "perf" only respects ~/.perfconfig and
/etc/perfconfig.

Proposed fix: because of case (1), I would like a way to tell Git to
stop trusting any files in .git.  That is:

 1. Introduce a (configurable) list of "safe" configuration items that
    can be set in .git/config and don't respect any others.

 2. But what if I want to set a different pager per-repository?
    I think we could do this using configuration "profiles".
    My ~/.config/git/profiles/ directory would contain git-style
    config files for repositories to include.  Repositories could
    then contain

	[include]
		path = ~/.config/git/profiles/fancy-log-pager

    to make use of those settings.  The facility (1) would
    special-case this directory to allow it to set "unsafe" settings
    since files there are assumed not to be under the control of an
    attacker.

 3. Likewise for hooks: my ~/.config/git/hooks/ directory would
    contain hooks for repositories to make use of.  Repositories could
    symlink to hook files from there to make use of them.

    That would allow the configured hooks in ~/.config/git/hooks/ to
    be easy to find and to upgrade in one place.

    To help users migrate, when a hook is present and executable in
    .git/hooks/, Git would print instructions for moving it to
    ~/.config/git/hooks/ and replacing it with a symlink after
    inspecting it.

For backward compatibility, this facility would be controlled by a
global configuration setting.  If that setting is not enabled, then
the current, less safe behavior would remain.

One downside of (3) is its reliance on symlinks.  Some alternatives:

 3b. Use core.hooksPath configuration instead.  Rely on (2).
 3c. Introduce new hook.* configuration to be used instead of hook
     scripts.  Rely on (2).

Thoughts?
Jonathan

Re: Security of .git/config and .git/hooks

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Proposed fix: because of case (1), I would like a way to tell Git to
> stop trusting any files in .git.  That is:
>
>  1. Introduce a (configurable) list of "safe" configuration items that
>     can be set in .git/config and don't respect any others.

The list of "safe" things are configurable by having something in
~/.gitconfig, perhaps?

How would this work, from the end-user's point of view, with "git
config --global" and "git config --local"?

>  2. But what if I want to set a different pager per-repository?
>     I think we could do this using configuration "profiles".
>     My ~/.config/git/profiles/ directory would contain git-style
>     config files for repositories to include.  Repositories could
>     then contain
>
> 	[include]
> 		path = ~/.config/git/profiles/fancy-log-pager
>
>     to make use of those settings.  The facility (1) would
>     special-case this directory to allow it to set "unsafe" settings
>     since files there are assumed not to be under the control of an
>     attacker.

Meaning, "include" is not in "safe" category, but a value that
begins with "~/.config/git/" are excempt???

>  3. Likewise for hooks: my ~/.config/git/hooks/ directory would
>     contain hooks for repositories to make use of.  Repositories could
>     symlink to hook files from there to make use of them.

I am not sure what this means.  .git/hooks/pre-commit being a
symbolic link to "~/.config/git/hooks/pre-commit-fancy"
(i.e. readlink gives the path with tilde unexpanded), so that the
attacked sysadmin will not run it from ~attacker/.config/git/hooks?  

And the code that finds a hook to run sees .git/hooks/pre-commit,
resolves the symlink manually and makes sure it leads to somewhere
inside ~/.config/...  (otherwise it rejects) and then uses the
pointed-at copy?

At that point, we are not taking any advantage of symbolic-link-ness
of the entity, so .git/hooks/pre-commit being a text file that has a
single like, e.g.

	# safe-hook: pre-commit-fancy

may be sufficient (and we do not have to worry about systems without
symbolic links)?  The machinery that used to manually resolved symlink
instead reads it, finds "pre-commit-fancy" in ~/.config/git/hooks/
and runs it, and you get the same behaviour, no?

> One downside of (3) is its reliance on symlinks.  Some alternatives:
>
>  3b. Use core.hooksPath configuration instead.  Rely on (2).
>  3c. Introduce new hook.* configuration to be used instead of hook
>      scripts.  Rely on (2).

I guess I invented 3d. without reading ahead X-<.  None of the 3x
variants other than 3 proper will not work for scripts and existing
code that sees that .git/hooks/pre-commit is an executable and runs
it, and 3 proper will not work without symbolic links, so this means
we'd need "git locate-hook pre-commit" (and underlying locate_hook()
helper API) that returns "/home/me/.git/config/hook/pre-commit-fancy"
or fails when we do this transition.  In an unconverted repository,
it may return $PWD/.git/hooks/pre-commit, or failure if we are
running under the paranoid mode.

Sounds workable.

Re: Security of .git/config and .git/hooks

[Christian Couder]

Hi,

On Tue, Oct 3, 2017 at 1:45 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

> Proposed fix: because of case (1), I would like a way to tell Git to
> stop trusting any files in .git.  That is:
>
>  1. Introduce a (configurable) list of "safe" configuration items that
>     can be set in .git/config and don't respect any others.

Maybe we can already add a --list-security or --check-security or
--unsafe to `git config` to list the unsafe options and their values
as well as the active hooks, so that admins/users can already easily
take a quick look at the config before they start playing with a
potentially unsafe repo.

Re: Security of .git/config and .git/hooks

[Jeff King]

On Mon, Oct 02, 2017 at 04:45:17PM -0700, Jonathan Nieder wrote:

> This topic has been mentioned on this mailing list before but I had
> trouble finding a relevant reference.  Links welcome.

There were discussions long ago related to the upload-pack hook. One of
the proposed fixes was checking the owner of the hook against the
running user, but in the end we just removed it.

Some of the discussion I have in my mail archive is off-list, but it was
brought up on-list later, too:

  https://public-inbox.org/git/6f8b45101001141001q40d8b746v8385bc6ae37a6af4@mail.gmail.com/

The owner-match thing is intriguing, but I think it only helps with
multi-user systems. For your zipfile case, there's literally no
information on the disk about whether the repo is trusted or not. You'd
have to put Git into an "untrusted" mode.

There's also some prior art in the uploadpack.packObjectsHook, which is
ignored totally in repo-level config.

> Suppose that I add the following to .git/config in a repository on a
> shared computer:
> 
> 	[pager]
> 		log = rm -fr /
> 		fsck = rm -fr /
> 
> ("rm -fr /" is of course a placeholder here.)
> 
> I then tell a sysadmin that git commands are producing strange output
> and I am having trouble understanding what is going on.  They may run
> "git fsck" or "git log"; in either case, the output is passed to the
> pager I configured, allowing me to run an arbitrary command using the
> sysadmin's credentials.

I know you probably didn't mean this to be an exhaustive list, but there
are really a ton of config options that can result in executing
arbitrary commands. External diff, textconv, ssh commands, and so on.

I don't think that changes your point any, but it's something to keep in
mind when evaluating solutions:

  - if individual options need to be annotated as unsafe, there's a high
    risk of missing an option (or introducing a new one incorrectly)

  - any schemes which reduce functionality (e.g., by disallowing certain
    options in repo config by default) are going to affect a lot of
    people

> Proposed fix: because of case (1), I would like a way to tell Git to
> stop trusting any files in .git.  That is:
> 
>  1. Introduce a (configurable) list of "safe" configuration items that
>     can be set in .git/config and don't respect any others.

A whitelist is obviously safer than a blacklist. Though I also feel like
some of the options may give an unexpectedly wide attack surface. I.e.,
I wouldn't be surprised if some innocent-looking option ends up being
used in a tricky way to gain more access. E.g., submodule config
pointing to paths outside of the repository.

Do you plan to run in safe-mode all the time? What if safe-mode was a
lot more extreme, and simply avoided reading repo-level config at all
(except for check_repository_format(), which should be pretty innocent).

I have a feeling there are some features (like submodules) that would
simply be broken in safe-mode.

>  2. But what if I want to set a different pager per-repository?
>     I think we could do this using configuration "profiles".
>     My ~/.config/git/profiles/ directory would contain git-style
>     config files for repositories to include.  Repositories could
>     then contain
> 
> 	[include]
> 		path = ~/.config/git/profiles/fancy-log-pager
> 
>     to make use of those settings.  The facility (1) would
>     special-case this directory to allow it to set "unsafe" settings
>     since files there are assumed not to be under the control of an
>     attacker.

You can do something quite similar already:

  git config --global \
    include.gitdir:/path/to/specific/repo.path
    .gitconfig-fancy-log-pager

The main difference is that the profile inclusion is done by path rather
than riding along with the repository directory if it gets moved. In
practice I doubt that matters much, and I think the security model for
include.gitdir is a lot simpler.

>  3. Likewise for hooks: my ~/.config/git/hooks/ directory would
>     contain hooks for repositories to make use of.  Repositories could
>     symlink to hook files from there to make use of them.
> 
>     That would allow the configured hooks in ~/.config/git/hooks/ to
>     be easy to find and to upgrade in one place.
> 
>     To help users migrate, when a hook is present and executable in
>     .git/hooks/, Git would print instructions for moving it to
>     ~/.config/git/hooks/ and replacing it with a symlink after
>     inspecting it.

I kind of wonder if the path-limited includes from above plus
core.hooksPath would be simpler. The source of authority then is always
outside of the repository, rather than trying to vet some values inside
the repository.

Reading more, I think I just reinvented your 3b.

> For backward compatibility, this facility would be controlled by a
> global configuration setting.  If that setting is not enabled, then
> the current, less safe behavior would remain.

Are config and symlinks everything we need to care about? I can't think
of anything else, but Git really has quite a large attack surface when
accessing a local repo. Right now the safest thing you can do is "git
clone --no-local" an untrusted repo and then look only at the clone. Of
course nobody _actually_ does that, so any "safe" mode seems like it
would be an improvement. But would claiming to have a "safe" mode
encourage people to use it to look at risky repositories, exacerbating
any holes (e.g., exploiting a bug in the index format)? I don't know.

-Peff

Re: Security of .git/config and .git/hooks

[Stefan Beller]

So once upon a time we compared Gits security model with a
web browser. A web browser lets you execute 3rd party code
(e.g. javascript) and it is supposedly safe to look at malicious sites.

Currently Git only promises to have the clone/fetch operation safe,
not the "here is a zip of my whole repo" case, which sounds more
like the web browser experience ("here is a site with js, even zipped
in transfer"). Tightening the security model of Git towards this seems
like a good idea to me.

>>  1. Introduce a (configurable) list of "safe" configuration items that
>>     can be set in .git/config and don't respect any others.
>
> A whitelist is obviously safer than a blacklist. Though I also feel like
> some of the options may give an unexpectedly wide attack surface. I.e.,
> I wouldn't be surprised if some innocent-looking option ends up being
> used in a tricky way to gain more access. E.g., submodule config
> pointing to paths outside of the repository.
>
> Do you plan to run in safe-mode all the time? What if safe-mode was a
> lot more extreme, and simply avoided reading repo-level config at all
> (except for check_repository_format(), which should be pretty innocent).
>
> I have a feeling there are some features (like submodules) that would
> simply be broken in safe-mode.

I would think that the essential submodule things would be "safe"
to look at. But e.g. submodule.<name>.update = "!rm -rf /" would be
not ok, hence the .update configuration would be in the unsafe space.

Any unsafe config option would need to be set outside the actual
repository (~/.config/git/<repo-id>/config ?)

>
>>  2. But what if I want to set a different pager per-repository?
>>     I think we could do this using configuration "profiles".
>>     My ~/.config/git/profiles/ directory would contain git-style
>>     config files for repositories to include.  Repositories could
>>     then contain
>>
>>       [include]
>>               path = ~/.config/git/profiles/fancy-log-pager
>>
>>     to make use of those settings.  The facility (1) would
>>     special-case this directory to allow it to set "unsafe" settings
>>     since files there are assumed not to be under the control of an
>>     attacker.
>
> You can do something quite similar already:
>
>   git config --global \
>     include.gitdir:/path/to/specific/repo.path
>     .gitconfig-fancy-log-pager
>
> The main difference is that the profile inclusion is done by path rather
> than riding along with the repository directory if it gets moved. In
> practice I doubt that matters much, and I think the security model for
> include.gitdir is a lot simpler.

I am not sure if this works so well for the submodule.<name>.update
config (that we want to deprecate anyway, but still)

>> For backward compatibility, this facility would be controlled by a
>> global configuration setting.  If that setting is not enabled, then
>> the current, less safe behavior would remain.
>
> Are config and symlinks everything we need to care about? I can't think
> of anything else, but Git really has quite a large attack surface when
> accessing a local repo. Right now the safest thing you can do is "git
> clone --no-local" an untrusted repo and then look only at the clone. Of
> course nobody _actually_ does that, so any "safe" mode seems like it
> would be an improvement. But would claiming to have a "safe" mode
> encourage people to use it to look at risky repositories, exacerbating
> any holes (e.g., exploiting a bug in the index format)? I don't know.

Good point. Though we only care about the case of breaking out and
executing untrusted code; most of the index exploits would rather
trigger a segfault or infinite lop (in my imagination at least).

>
> -Peff