Shared repositories no longer securable against privilege escalation

Git has started requiring write access to the root of bare repositories in order to create /HEAD.lock. This is a major security problem in shared environments as it also entails control over the /config link i.e. core.hooksPath. Permission to write objects and update refs should be entirely separate from permission to edit hook execution logic.

Given that /HEAD is not dynamically modified in the normal lifetimes of bare repositories, /HEAD.lock creation failure should probably be, at worst, an ignorable soft failure. Alternatively, some form of stale lockfile handling (currently there is none) could be made to work with a writable HEAD.lock in a read-only bare repository.

Obigatory HEAD.lock creation was introduced in the following commit:

commit 92b1551b1d407065f961ffd1d972481063a0edcc
Author: Michael Haggerty <mhagger@alum.mit.edu>
Date:   Mon Apr 25 15:56:07 2016 +0200

    refs: resolve symbolic refs first

Test case:

root@richardiv:~# GIT_DIR=/tmp/test.git git init --bare --shared=group
Initialized empty shared Git repository in /tmp/test.git/
root@richardiv:~# cd /tmp/test.git
root@richardiv:/tmp/test.git# touch git-daemon-export-ok packed-refs
root@richardiv:/tmp/test.git# mkdir -p info logs branches
root@richardiv:/tmp/test.git# find refs info branches objects logs          -type d -print0 | xargs -0 chmod 2775
root@richardiv:/tmp/test.git# find refs info branches logs HEAD packed-refs -type f -print0 | xargs -0 chmod 0664
root@richardiv:/tmp/test.git# find objects                                  -type f -print0 | xargs -0 --no-run-if-empty chmod 0644
root@richardiv:/tmp/test.git# find refs info branches objects logs HEAD packed-refs -print0 | xargs -0 chgrp git-test
root@richardiv:/tmp/test.git# chown root.root . config description git-daemon-export-ok hooks
root@richardiv:/tmp/test.git# chmod 0644 config description git-daemon-export-ok
root@richardiv:/tmp/test.git# chmod 00755 . hooks
root@richardiv:/tmp/test.git# sudo -i -u user1
user1@richardiv:~$ git clone /tmp/test.git
Cloning into 'test'...
warning: You appear to have cloned an empty repository.
done.
user1@richardiv:~$ cd test
user1@richardiv:~/test$ touch test
user1@richardiv:~/test$ git add test
user1@richardiv:~/test$ git commit -m test test
[master (root-commit) ff21d72] test
 1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 test
user1@richardiv:~/test$ git push
Counting objects: 3, done.
Writing objects: 100% (3/3), 206 bytes | 0 bytes/s, done.
Total 3 (delta 0), reused 0 (delta 0)
remote: error: cannot lock ref 'HEAD': Unable to create '/tmp/test.git/HEAD.lock': Permission denied
To /tmp/test.git
 ! [remote rejected] master -> master (failed to update ref)
error: failed to push some refs to '/tmp/test.git'
user1@richardiv:~/test$ logout
root@richardiv:/tmp/test.git# chgrp git-test .
root@richardiv:/tmp/test.git# chmod 2775 .
root@richardiv:/tmp/test.git# sudo -s -u user1
user1@richardiv:/tmp/test.git$ mv config config-old
user1@richardiv:/tmp/test.git$ touch config # POWER ALMIGHTY
user1@richardiv:/tmp/test.git$

Please CC me on this thread; I am not on the mailing list.

On 03/17/2017 01:23 AM, Joe Rayhawk wrote:

Thanks for the report. This is indeed a problem for people who want to set restrictive privileges on $GIT_DIR. I'd never thought of that use case, but it makes sense. Is this practice recommended somewhere or required by any Git hosting tools? (I'm curious how prevalent it is.)

The locking was added intentionally, to ensure that the reflog for `HEAD` is written safely. But the code didn't do that prior to the commit that you referenced, so (as a special case) ignoring failures to lock `HEAD` due to insufficient permission is probably a reasonable compromise.

I think the special case could be restricted even further to when `HEAD` has the `REF_LOG_ONLY` flag in the reference transaction. I don't think that `HEAD` would ever show up in a transaction solely to verify its old value in a typical server scenario, but if so, that situation could be special cased too.

(I can't resist pointing out that the *real* bug is storing special references like `HEAD` in the top level of $GIT_DIR, but that can't be changed now.)

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

If you call that "pointing out", I can't resist pointing out that you are utterly *wrong* ;-)

For one thing, HEAD.lock being the only reported case does not mean "special refs" is the only thing, and more importantly, it will stay to be the only thing, that would want to write directly underneath $GIT_DIR directory. We may want to add a feature to store push certificates whenever a signed push is made, and we are free to decide that directly underneath $GIT_DIR is the place to do so.

Also, with your same logic, you could also say that the real bug is not in the refs subsystem but is in the lockfile subsystem. If it did not use $GIT_DIR/$thing.lock when locking $GIT_DIR/$thing, and instead it used $GIT_DIR/lock/$thing to do so, you wouldn't have needed to be able to create $GIT_DIR/HEAD.lock.

I _think_ the real bug is that somehow a user got a wrong impression that directly underneath $GIT_DIR/ is somehow different from its subdirectory and it is OK to make the directory unwritable. I do not think we never intended to give such a promise, but there may be a documentation bug that gives the wrong impression, which we may have to fix.

We do try to make sure that in a read-only repository $GIT_DIR/ and everything underneath can be read-only (and if that is not the case, you found a bug), but even in that case, we do not special case $GIT_DIR/ itself and its subdirectories.

Quoting Junio C Hamano (2017-03-17 08:26:39)

Actually, yeah, that's a useful outcome I can steelman out of this email: given that git init --shared has always introduced trivially exploitable security escalations, it should probably either be changed to use sane permissions or have its documentation changed to mention that, at least on base POSIX, using --shared to share a repository between multiple UIDs literally eliminates the purpose of having multiple UIDs.

Joe Rayhawk <jrayhawk@freedesktop.org> writes:

I do not think the world is _that_ blank-and-white. If you cannot trust those who push to the repository, you can give them git-only access without a shell account and keep separating them with UIDs. If you can, then the --shared setting is suitable for you.

Quoting Michael Haggerty (2017-03-17 05:07:36)

I had to work out the practice for my own management engine; I have since deployed it to around eight different mixed-use multi-user operations, the most significant of which is Freedesktop.org.

Without this practice, core.sharedRepository is an enormous liability of a feature. I can't speak to whether anyone but me ever noticed, what with mixed-use multi-user POSIX environments becoming increasingly rare.

Hooray!

Yeah, putting refs outside of refs/ is conceptually pretty nutty.

W dniu 17.03.2017 o 18:12, Joe Rayhawk pisze:

Is there a reason why you rely on file permissions and user groups to enforce access control, instead of using public-key based solution such as Gitolite?

Michael Haggerty <mhagger@alum.mit.edu> writes:

I find both of these acceptably good changes.

On Fri, Mar 17, 2017 at 1:23 AM, Joe Rayhawk <jrayhawk@freedesktop.org> wrote:

[Full disclosure: I implemented core.hooksPath]

The core.hooksPath facility doesn't introduce any sort of new security problems that didn't exist already, and if you're just focusing on the sort of problems changing core.hooksPath might bring up you're still vulnerable to those.

If you give me general shell access to some repo where I can write refs and objects you can't use hooks to sanity check anything I push. E.g. let's say you have an "update" hook which makes sure I can't push binaries (malware) to your "master" branch. I can just push that to some other branch, then log in and run:

    echo <new_sha1> > /path/to/bare/repo.git/refs/heads/master

Ah ha! You might say, you'll just make that update hook run for any branch or reference! That doesn't matter either, if you give me write access to objects/ and refs/ I can just manually echo the objects I want there, and then manually update the ref.

If you want to run a shared repository via ssh login where you want to reliably execute hooks you need to either use something like Gitolite, as Jakub points out, or e.g. set the user's shell to git-shell or some similar facility which whitelists the commands the user can run. Anything else is just security through obscurity.