{"thread":{"id":"45403","subject":"Shared repositories no longer securable against privilege escalation","startedAt":"2017-03-17T02:08:04Z","lastAt":"2017-03-18T21:18:21Z","messageCount":9,"participants":["Joe Rayhawk","Michael Haggerty","Junio C Hamano","Jakub Narębski","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"314297","messageId":"148971018136.2144.12683278043600094739@richardiv.omgwallhack.org","threadId":"45403","inReplyTo":null,"subject":"Shared repositories no longer securable against privilege escalation","fromName":"Joe Rayhawk","fromEmail":"jrayhawk@freedesktop.org","sentAt":"2017-03-17T00:23:01Z","receivedAt":"2017-03-17T02:08:04Z","isPatch":false,"sender":{"key":"jrayhawk@freedesktop.org","avatar":null},"body":"Git has started requiring write access to the root of bare repositories\nin order to create /HEAD.lock. This is a major security problem in\nshared environments as it also entails control over the /config link\ni.e. core.hooksPath. Permission to write objects and update refs should\nbe entirely separate from permission to edit hook execution logic.\n\nGiven that /HEAD is not dynamically modified in the normal lifetimes of\nbare repositories, /HEAD.lock creation failure should probably be, at\nworst, an ignorable soft failure. Alternatively, some form of stale\nlockfile handling (currently there is none) could be made to work with\na writable HEAD.lock in a read-only bare repository.\n\nObigatory HEAD.lock creation was introduced in the following commit:\n\ncommit 92b1551b1d407065f961ffd1d972481063a0edcc\nAuthor: Michael Haggerty <mhagger@alum.mit.edu>\nDate:   Mon Apr 25 15:56:07 2016 +0200\n\n    refs: resolve symbolic refs first\n\nTest case:\n\nroot@richardiv:~# GIT_DIR=/tmp/test.git git init --bare --shared=group\nInitialized empty shared Git repository in /tmp/test.git/\nroot@richardiv:~# cd /tmp/test.git\nroot@richardiv:/tmp/test.git# touch git-daemon-export-ok packed-refs\nroot@richardiv:/tmp/test.git# mkdir -p info logs branches\nroot@richardiv:/tmp/test.git# find refs info branches objects logs          -type d -print0 | xargs -0 chmod 2775\nroot@richardiv:/tmp/test.git# find refs info branches logs HEAD packed-refs -type f -print0 | xargs -0 chmod 0664\nroot@richardiv:/tmp/test.git# find objects                                  -type f -print0 | xargs -0 --no-run-if-empty chmod 0644\nroot@richardiv:/tmp/test.git# find refs info branches objects logs HEAD packed-refs -print0 | xargs -0 chgrp git-test\nroot@richardiv:/tmp/test.git# chown root.root . config description git-daemon-export-ok hooks\nroot@richardiv:/tmp/test.git# chmod 0644 config description git-daemon-export-ok\nroot@richardiv:/tmp/test.git# chmod 00755 . hooks\nroot@richardiv:/tmp/test.git# sudo -i -u user1\nuser1@richardiv:~$ git clone /tmp/test.git\nCloning into 'test'...\nwarning: You appear to have cloned an empty repository.\ndone.\nuser1@richardiv:~$ cd test\nuser1@richardiv:~/test$ touch test\nuser1@richardiv:~/test$ git add test\nuser1@richardiv:~/test$ git commit -m test test\n[master (root-commit) ff21d72] test\n 1 file changed, 0 insertions(+), 0 deletions(-)\ncreate mode 100644 test\nuser1@richardiv:~/test$ git push\nCounting objects: 3, done.\nWriting objects: 100% (3/3), 206 bytes | 0 bytes/s, done.\nTotal 3 (delta 0), reused 0 (delta 0)\nremote: error: cannot lock ref 'HEAD': Unable to create '/tmp/test.git/HEAD.lock': Permission denied\nTo /tmp/test.git\n ! [remote rejected] master -> master (failed to update ref)\nerror: failed to push some refs to '/tmp/test.git'\nuser1@richardiv:~/test$ logout\nroot@richardiv:/tmp/test.git# chgrp git-test .\nroot@richardiv:/tmp/test.git# chmod 2775 .\nroot@richardiv:/tmp/test.git# sudo -s -u user1\nuser1@richardiv:/tmp/test.git$ mv config config-old\nuser1@richardiv:/tmp/test.git$ touch config # POWER ALMIGHTY\nuser1@richardiv:/tmp/test.git$\n\nPlease CC me on this thread; I am not on the mailing list.\n\n"},{"id":"314325","messageId":"195b30d7-9ea2-7a9b-79ca-41b7bb890a30@alum.mit.edu","threadId":"45403","inReplyTo":"148971018136.2144.12683278043600094739@richardiv.omgwallhack.org","subject":"Re: Shared repositories no longer securable against privilege escalation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-03-17T12:07:36Z","receivedAt":"2017-03-17T12:10:31Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/17/2017 01:23 AM, Joe Rayhawk wrote:\n> Git has started requiring write access to the root of bare repositories\n> in order to create /HEAD.lock. This is a major security problem in\n> shared environments as it also entails control over the /config link\n> i.e. core.hooksPath. Permission to write objects and update refs should\n> be entirely separate from permission to edit hook execution logic.\n> \n> Given that /HEAD is not dynamically modified in the normal lifetimes of\n> bare repositories, /HEAD.lock creation failure should probably be, at\n> worst, an ignorable soft failure. Alternatively, some form of stale\n> lockfile handling (currently there is none) could be made to work with\n> a writable HEAD.lock in a read-only bare repository.\n> \n> Obigatory HEAD.lock creation was introduced in the following commit:\n> \n> commit 92b1551b1d407065f961ffd1d972481063a0edcc\n> Author: Michael Haggerty <mhagger@alum.mit.edu>\n> Date:   Mon Apr 25 15:56:07 2016 +0200\n> \n>     refs: resolve symbolic refs first\n\nThanks for the report. This is indeed a problem for people who want to\nset restrictive privileges on $GIT_DIR. I'd never thought of that use\ncase, but it makes sense. Is this practice recommended somewhere or\nrequired by any Git hosting tools? (I'm curious how prevalent it is.)\n\nThe locking was added intentionally, to ensure that the reflog for\n`HEAD` is written safely. But the code didn't do that prior to the\ncommit that you referenced, so (as a special case) ignoring failures to\nlock `HEAD` due to insufficient permission is probably a reasonable\ncompromise.\n\nI think the special case could be restricted even further to when `HEAD`\nhas the `REF_LOG_ONLY` flag in the reference transaction. I don't think\nthat `HEAD` would ever show up in a transaction solely to verify its old\nvalue in a typical server scenario, but if so, that situation could be\nspecial cased too.\n\n(I can't resist pointing out that the *real* bug is storing special\nreferences like `HEAD` in the top level of $GIT_DIR, but that can't be\nchanged now.)\n\nMichael\n\n"},{"id":"314342","messageId":"xmqqo9wz9b68.fsf@gitster.mtv.corp.google.com","threadId":"45403","inReplyTo":"195b30d7-9ea2-7a9b-79ca-41b7bb890a30@alum.mit.edu","subject":"Re: Shared repositories no longer securable against privilege escalation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-17T15:26:39Z","receivedAt":"2017-03-17T15:27:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> (I can't resist pointing out that the *real* bug is storing special\n> references like `HEAD` in the top level of $GIT_DIR, but that can't be\n> changed now.)\n\nIf you call that \"pointing out\", I can't resist pointing out that\nyou are utterly *wrong* ;-)\n\nFor one thing, HEAD.lock being the only reported case does not mean\n\"special refs\" is the only thing, and more importantly, it will stay\nto be the only thing, that would want to write directly underneath\n$GIT_DIR directory.  We may want to add a feature to store push\ncertificates whenever a signed push is made, and we are free to\ndecide that directly underneath $GIT_DIR is the place to do so.\n\nAlso, with your same logic, you could also say that the real bug is\nnot in the refs subsystem but is in the lockfile subsystem.  If it\ndid not use $GIT_DIR/$thing.lock when locking $GIT_DIR/$thing, and\ninstead it used $GIT_DIR/lock/$thing to do so, you wouldn't have\nneeded to be able to create $GIT_DIR/HEAD.lock.\n\nI _think_ the real bug is that somehow a user got a wrong impression\nthat directly underneath $GIT_DIR/ is somehow different from its\nsubdirectory and it is OK to make the directory unwritable.  I do\nnot think we never intended to give such a promise, but there may be\na documentation bug that gives the wrong impression, which we may\nhave to fix.\n\nWe do try to make sure that in a read-only repository $GIT_DIR/ and\neverything underneath can be read-only (and if that is not the case,\nyou found a bug), but even in that case, we do not special case\n$GIT_DIR/ itself and its subdirectories.\n"},{"id":"314347","messageId":"148976932628.2144.11216577266857568258@richardiv.omgwallhack.org","threadId":"45403","inReplyTo":"xmqqo9wz9b68.fsf@gitster.mtv.corp.google.com","subject":"Re: Shared repositories no longer securable against privilege escalation","fromName":"Joe Rayhawk","fromEmail":"jrayhawk@freedesktop.org","sentAt":"2017-03-17T16:48:46Z","receivedAt":"2017-03-17T16:49:22Z","isPatch":false,"sender":{"key":"jrayhawk@freedesktop.org","avatar":null},"body":"Quoting Junio C Hamano (2017-03-17 08:26:39)\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> I _think_ the real bug is that somehow a user got a wrong impression\n> that directly underneath $GIT_DIR/ is somehow different from its\n> subdirectory and it is OK to make the directory unwritable.  I do\n> not think we never intended to give such a promise, but there may be\n> a documentation bug that gives the wrong impression, which we may\n> have to fix.\n\nActually, yeah, that's a useful outcome I can steelman out of this\nemail: given that git init --shared has always introduced trivially\nexploitable security escalations, it should probably either be changed\nto use sane permissions or have its documentation changed to mention\nthat, at least on base POSIX, using --shared to share a repository\nbetween multiple UIDs literally eliminates the purpose of having\nmultiple UIDs.\n"},{"id":"314367","messageId":"148977072642.2144.7040206366650087348@richardiv.omgwallhack.org","threadId":"45403","inReplyTo":"195b30d7-9ea2-7a9b-79ca-41b7bb890a30@alum.mit.edu","subject":"Re: Shared repositories no longer securable against privilege escalation","fromName":"Joe Rayhawk","fromEmail":"jrayhawk@freedesktop.org","sentAt":"2017-03-17T17:12:06Z","receivedAt":"2017-03-17T18:08:26Z","isPatch":false,"sender":{"key":"jrayhawk@freedesktop.org","avatar":null},"body":"Quoting Michael Haggerty (2017-03-17 05:07:36)\n> On 03/17/2017 01:23 AM, Joe Rayhawk wrote:\n> > Git has started requiring write access to the root of bare repositories\n> > in order to create /HEAD.lock. This is a major security problem in\n> > shared environments as it also entails control over the /config link\n> > i.e. core.hooksPath. Permission to write objects and update refs should\n> > be entirely separate from permission to edit hook execution logic.\n> \n> Thanks for the report. This is indeed a problem for people who want to\n> set restrictive privileges on $GIT_DIR. I'd never thought of that use\n> case, but it makes sense. Is this practice recommended somewhere or\n> required by any Git hosting tools? (I'm curious how prevalent it is.)\n\nI had to work out the practice for my own management engine; I have\nsince deployed it to around eight different mixed-use multi-user\noperations, the most significant of which is Freedesktop.org.\n\nWithout this practice, core.sharedRepository is an enormous liability\nof a feature. I can't speak to whether anyone but me ever noticed, what\nwith mixed-use multi-user POSIX environments becoming increasingly rare.\n\n> The locking was added intentionally, to ensure that the reflog for\n> `HEAD` is written safely. But the code didn't do that prior to the\n> commit that you referenced, so (as a special case) ignoring failures to\n> lock `HEAD` due to insufficient permission is probably a reasonable\n> compromise.\n\nHooray!\n\n> (I can't resist pointing out that the *real* bug is storing special\n> references like `HEAD` in the top level of $GIT_DIR, but that can't be\n> changed now.)\n\nYeah, putting refs outside of refs/ is conceptually pretty nutty.\n"},{"id":"314371","messageId":"xmqqtw6r7p16.fsf@gitster.mtv.corp.google.com","threadId":"45403","inReplyTo":"148976932628.2144.11216577266857568258@richardiv.omgwallhack.org","subject":"Re: Shared repositories no longer securable against privilege escalation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-17T18:10:13Z","receivedAt":"2017-03-17T18:18:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joe Rayhawk <jrayhawk@freedesktop.org> writes:\n\n> that, at least on base POSIX, using --shared to share a repository\n> between multiple UIDs literally eliminates the purpose of having\n> multiple UIDs.\n\nI do not think the world is _that_ blank-and-white.  If you cannot\ntrust those who push to the repository, you can give them git-only\naccess without a shell account and keep separating them with UIDs.\nIf you can, then the --shared setting is suitable for you.\n"},{"id":"314373","messageId":"xmqqinn77odv.fsf@gitster.mtv.corp.google.com","threadId":"45403","inReplyTo":"195b30d7-9ea2-7a9b-79ca-41b7bb890a30@alum.mit.edu","subject":"Re: Shared repositories no longer securable against privilege escalation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-17T18:24:12Z","receivedAt":"2017-03-17T18:38:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> The locking was added intentionally, to ensure that the reflog for\n> `HEAD` is written safely. But the code didn't do that prior to the\n> commit that you referenced, so (as a special case) ignoring failures to\n> lock `HEAD` due to insufficient permission is probably a reasonable\n> compromise.\n>\n> I think the special case could be restricted even further to when `HEAD`\n> has the `REF_LOG_ONLY` flag in the reference transaction. I don't think\n> that `HEAD` would ever show up in a transaction solely to verify its old\n> value in a typical server scenario, but if so, that situation could be\n> special cased too.\n\nI find both of these acceptably good changes.\n\n"},{"id":"314536","messageId":"726ef4cf-db9e-991b-5df5-168c7e9a41f6@gmail.com","threadId":"45403","inReplyTo":"148977072642.2144.7040206366650087348@richardiv.omgwallhack.org","subject":"Re: Shared repositories no longer securable against privilege escalation","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2017-03-18T19:32:41Z","receivedAt":"2017-03-18T19:32:56Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 17.03.2017 o 18:12, Joe Rayhawk pisze:\n> Quoting Michael Haggerty (2017-03-17 05:07:36)\n\n>>\n>> Thanks for the report. This is indeed a problem for people who want to\n>> set restrictive privileges on $GIT_DIR. I'd never thought of that use\n>> case, but it makes sense. Is this practice recommended somewhere or\n>> required by any Git hosting tools? (I'm curious how prevalent it is.)\n> \n> I had to work out the practice for my own management engine; I have\n> since deployed it to around eight different mixed-use multi-user\n> operations, the most significant of which is Freedesktop.org.\n> \n> Without this practice, core.sharedRepository is an enormous liability\n> of a feature. I can't speak to whether anyone but me ever noticed, what\n> with mixed-use multi-user POSIX environments becoming increasingly rare.\n\nIs there a reason why you rely on file permissions and user groups\nto enforce access control, instead of using public-key based solution\nsuch as Gitolite?\n\n-- \nJakub Narębski\n\n"},{"id":"314545","messageId":"CACBZZX4VApZnO1u708nxqq711K7PD66U=mTT2tjnbt2XjxmQew@mail.gmail.com","threadId":"45403","inReplyTo":"148971018136.2144.12683278043600094739@richardiv.omgwallhack.org","subject":"Re: Shared repositories no longer securable against privilege escalation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T21:17:50Z","receivedAt":"2017-03-18T21:18:21Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Mar 17, 2017 at 1:23 AM, Joe Rayhawk <jrayhawk@freedesktop.org> wrote:\n> Git has started requiring write access to the root of bare repositories\n> in order to create /HEAD.lock. This is a major security problem in\n> shared environments as it also entails control over the /config link\n> i.e. core.hooksPath. Permission to write objects and update refs should\n> be entirely separate from permission to edit hook execution logic.\n\n[Full disclosure: I implemented core.hooksPath]\n\nThe core.hooksPath facility doesn't introduce any sort of new security\nproblems that didn't exist already, and if you're just focusing on the\nsort of problems changing core.hooksPath might bring up you're still\nvulnerable to those.\n\nIf you give me general shell access to some repo where I can write\nrefs and objects you can't use hooks to sanity check anything I push.\nE.g. let's say you have an \"update\" hook which makes sure I can't push\nbinaries (malware) to your \"master\" branch. I can just push that to\nsome other branch, then log in and run:\n\n    echo <new_sha1> > /path/to/bare/repo.git/refs/heads/master\n\nAh ha! You might say, you'll just make that update hook run for any\nbranch or reference! That doesn't matter either, if you give me write\naccess to objects/ and refs/ I can just manually echo the objects I\nwant there, and then manually update the ref.\n\nIf you want to run a shared repository via ssh login where you want to\nreliably execute hooks you need to either use something like Gitolite,\nas Jakub points out, or e.g. set the user's shell to git-shell or some\nsimilar facility which whitelists the commands the user can run.\nAnything else is just security through obscurity.\n"}]}