Re: Shared repositories no longer securable against privilege escalation

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 17, 2017 at 1:23 AM, Joe Rayhawk  wrote:
> 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.

[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  > /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.

Re: Shared repositories no longer securable against privilege escalation

[Jakub Narębski]

W dniu 17.03.2017 o 18:12, Joe Rayhawk pisze:
> Quoting Michael Haggerty (2017-03-17 05:07:36)

>>
>> 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.)
> 
> 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.

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?

-- 
Jakub Narębski

Re: Shared repositories no longer securable against privilege escalation

[Junio C Hamano]

Michael Haggerty  writes:

> 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 find both of these acceptably good changes.

Re: Shared repositories no longer securable against privilege escalation

[Junio C Hamano]

Joe Rayhawk  writes:

> that, at least on base POSIX, using --shared to share a repository
> between multiple UIDs literally eliminates the purpose of having
> multiple UIDs.

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.

Re: Shared repositories no longer securable against privilege escalation

[Joe Rayhawk]

Quoting Michael Haggerty (2017-03-17 05:07:36)
> On 03/17/2017 01:23 AM, Joe Rayhawk wrote:
> > 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.
> 
> 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.)

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.

> 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.

Hooray!

> (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.)

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


signature.asc
Description: signature

Re: Shared repositories no longer securable against privilege escalation

[Joe Rayhawk]

Quoting Junio C Hamano (2017-03-17 08:26:39)
> Michael Haggerty  writes:
> 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.

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.


signature.asc
Description: signature

Re: Shared repositories no longer securable against privilege escalation

[Junio C Hamano]

Michael Haggerty  writes:

> (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.)

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.

Re: Shared repositories no longer securable against privilege escalation

[Michael Haggerty]

On 03/17/2017 01:23 AM, Joe Rayhawk wrote:
> 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 
> Date:   Mon Apr 25 15:56:07 2016 +0200
> 
> refs: resolve symbolic refs first

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

Shared repositories no longer securable against privilege escalation

[Joe Rayhawk]

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 
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.



signature.asc
Description: signature