Shared repositories no longer securable against privilege escalation

Shared repositories no longer securable against privilege escalation

[Joe Rayhawk]

[-- Attachment #1: Type: text/plain, Size: 3188 bytes --]

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.


[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE0W3/Ls5On90y4dmX35w74P7tvu8FAljLLDsACgkQ35w74P7t
vu/AFw/+JIKahCTesYSyHeALs4CYwM2iHuwkKoml+dw3rcbCkLe87Y2LGcD6vpMd
IrZQBCvNfpNpN2+PrCswhAaxMRwRmh9iLps2oTFhqR0AmPA5deP6f0zzvKjK5mbz
z33LveA11Dh0ydcqsMSpFF3gipwd2tWK6Jor06E2JYX7oBUn+jArjCf2uNRz5kpJ
WCbfJyqbJ4dILA3P+YgCFpT2841A0plqA8m/jBheTVnmG04Rs9jb4YjBHvQyq9E3
AEQHa3yJCfKCLjH7DxYiGeIJAsm6XhfRmCpvWXYMiq3EyLhwRL63zrrjgUOXD8rh
XHmaUjjDxQdD41/Q3ZwKKh9cvNpDIJ/8Hdh970n397sn1cuVNVlikLmnm3rLuybr
UOlfwgKlmT+CjNp0QndCbfQZqv6Q+0tsNCnHd9N6Yb5Ky1xDyoEuIN7A6+JdAEx/
UwxhI9q+n4ooV+1uyuNVsUAuk7tvCZ98OJiV8Qy9Tf5iYH31yKpLNPRAWjKu2b8n
yEVfekSFKZmELaMFRzStaDSdJ1S3vhqCJkaFSa/NJmczkVftB/WQnatiqb2pS6PF
tOV6KDv4G6bg5L6loY1aBP6vX3fSdc8zlyyayWznXkGoKPfbw+F6m1T2zc8HfA/p
/9ssS6ZBahq44+enaGqg/mR7PLkuBGOAk7H/mDzQ2n5kMdlQCrE=
=w1ni
-----END PGP SIGNATURE-----

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 <mhagger@alum.mit.edu>
> 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

Re: Shared repositories no longer securable against privilege escalation

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> 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

[Joe Rayhawk]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

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

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE0W3/Ls5On90y4dmX35w74P7tvu8FAljME2oACgkQ35w74P7t
vu/zIw//ZDtdLDtu9YLIGxQJKUGboOpuZeBfwowwLPzouYMip2jVvO4EUrE54Yvj
DBMGGf5stoCsB4pexZZbsi9z49jpgIgvAdOaLA2budTb/eiCuNCeJWqcqr6xHoHI
HwOIPFDKijrXs5MJaX3AC8v7nCjkB1Wz5sJnGUsxzmwgEitg1GPI40HSCSbxjNzW
RbwpGMEUc14Dd63tSniQJml+vAYA78UKfrmB650vrms1Lf6D47t15xPTv/nRalLa
3IGwmW+6738XXZ/rw8Awj4TjoL8jQ4k/ZCOMWHW4SPakuoGx233/YCHsNlOyrFWX
r3g7DtXo6uX2OKp3Q3pnvgTFdXreYCEDBtQXmBKLAyVYqlGO37hqkVliMQZPc3Qz
z9rOgStEQdM2xm79jouOfgcAayoZMrHGuUD7MfF++G/Nt+VAyj+M9XT5N1qk4MNc
jPt5IU65TecbLghISkZz5e0TIdaGYmJmqyyRrEixPC8sMpzf9B+9kgYe+hgnNwU2
l/Wef0fGxXkbhItw30X9RcU5+7LG/gZEhEmzOo7fS1M4yp1MBSnDNLv/Yvi5eFPD
u9t3GWDL+OVvoZgGIGcdeHQxftYxqOAMtGVF1hoE4PZ5id0QfQJJaDLSlD66Vsm6
3fbggDYsT26C1u/5lfgxQYvjCME5nLlY1Uenw8omnJZCRzTJqHM=
=+P0d
-----END PGP SIGNATURE-----

Re: Shared repositories no longer securable against privilege escalation

[Joe Rayhawk]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

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.

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE0W3/Ls5On90y4dmX35w74P7tvu8FAljMGOMACgkQ35w74P7t
vu/Lgw/9FFdT80VNlTn9iXKu9/7hEWYg9tDbWZkwXOX2Uf0akbb2SS1qsaMOmL+Y
gff2v2xy8E022yPuCwXk/l21z9Y0wmHm5QBmTr6x71yj15Mjs92bm/XS8ZKy//J4
dh3P3FpQ9AvDkaggET4qzl6mEWqWNfrcdfSp6URvK3JHuimn+cfWDqfgVVwSzPP7
8OMoU4TdytuH5t8oIMuAASKgxZ+u4A5Pe3rrZY9UHa51z4PV8Osd9xJUSZpyWnJJ
iK6tGJ4Gp4wXpsJ91b0YMFoq+vS5eteYS50Ka2mSmF+omUcb0iswQd84wkeUZJux
sAjQq7wF5AFTGI5swSJ633A0sGiZ3o64agzhskYQ3o8J57yBHaAAXxezauFjITrQ
yYc4glYJsCpFWqOmerra2dP/FOVxhJmsIJhwI9MSZDiAXgUi7y+uzzCx4FCmz0yh
BwPRJIS1SAS+td/TW59l1DXFSqwV575c19u8tweSD0dEjkXNQRaxssTBuwMxEXzb
ePNzbq03jXNUqov9mEsoqMwDw5ZOJYE+uCUH9V8zM+u53TiESge4wLMLuiQhjlGR
2VtzYS39djkjNI6FxiDm2W1CQIBpHHuFXpmO/bzMvdsjDVCbDGHiFJTsIbqTWsf8
QR7ArJol6qgFlJMFswR0mAbzBi+m2fReKBDron0f476SEuG2lfA=
=inQD
-----END PGP SIGNATURE-----

Re: Shared repositories no longer securable against privilege escalation

[Junio C Hamano]

Joe Rayhawk <jrayhawk@freedesktop.org> 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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> 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

[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

[Ævar Arnfjörð Bjarmason]

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