diff options
author | 2015-10-16 16:22:05 +0530 | |
---|---|---|
committer | 2015-11-01 16:48:37 +0530 | |
commit | be5c2f5752a958a89d085c18b28d1df9c34affdf (patch) | |
tree | 880eb4d7990b0dd1f9584832e9b5497948b11083 | |
parent | minor fixups to 'perms' command: (diff) | |
download | gitolite-gentoo-be5c2f5752a958a89d085c18b28d1df9c34affdf.tar.gz gitolite-gentoo-be5c2f5752a958a89d085c18b28d1df9c34affdf.tar.bz2 gitolite-gentoo-be5c2f5752a958a89d085c18b28d1df9c34affdf.zip |
fix ref-create permission bug on wild repos
TLDR: users are able to *create* new refs that they are not supposed to.
Upgrading gitolite is not mandatory; there is a workaround within the conf
syntax itself (see below).
The bug shows up if the following four conditions are satisfied:
repo foo/..* # 1 (see below)
C = @all # 2
RW+CD = CREATOR # 3
RW = special_user # 4
1. must be a wild repo
2. the create-repo line must allow "special_user(s)"
3. there is at least one rule containing RW.*C in the repo
4. the intent is that special user(s) can ff-push but cannot *create* refs
since they have only RW; for a refresher on this, see
http://gitolite.com/gitolite/conf.html#write-types
In such cases, the restriction on creating a ref is ignored. (Only creation
is affected; the bug does not affect delete, rewind permissions).
In general, this is a relatively minor bug. No data is destroyed, and no data
is leaked. Some cleanup of useless refs may be required if someone has
(ab)used this. At worst this could be a potential DOS, but I have never
considered DOS to be a valid concern when it requires *authorised* users.
If you have such a conf, and cannot immediately upgrade, there is a
workaround:
repo foo/..*
RW+CD = CREATOR
RW = special_user
- = special_user # add a deny rule for the special user(s)
C = @all # move the repo-create perm to the end,
# *after* the deny rule above
(Thanks to hseg on #gitolite for catching this and being tenacious about it!
At first, I was arrogant enough to reject the idea that such a bug could
exist; it took me a bit to get the right conditions in place and see the
problem!)
-rw-r--r-- | src/lib/Gitolite/Conf/Load.pm | 16 | ||||
-rw-r--r-- | t/C-vs-C.t | 43 |
2 files changed, 57 insertions, 2 deletions
diff --git a/src/lib/Gitolite/Conf/Load.pm b/src/lib/Gitolite/Conf/Load.pm index 88d1612..7728a5a 100644 --- a/src/lib/Gitolite/Conf/Load.pm +++ b/src/lib/Gitolite/Conf/Load.pm @@ -123,9 +123,21 @@ sub access { trace( 2, "DENIED by $refex" ) if $perm eq '-'; return "$aa $safe_ref $repo $user DENIED by $refex" if $perm eq '-'; - # $perm can be RW\+?(C|D|CD|DC)?M?. $aa can be W, +, C or D, or - # any of these followed by "M". + # For repo creation, perm will be C and aa will be "^C". For branch + # access, $perm can be RW\+?(C|D|CD|DC)?M?, and $aa can be W, +, C or + # D, or any of these followed by "M". + + # We need to turn $aa into a regex that can match a suitable $perm. + # This is trivially true for "^C", "W" and "D", but the others (+, C, + # M) need some tweaking. + + # first, quote the '+': ( my $aaq = $aa ) =~ s/\+/\\+/; + # if aa is just "C", the user is trying to create a *branch* (not a + # *repo*), so let's make the pattern clearer to reflect that. + $aaq = "RW.*C" if $aaq eq "C"; + # if the aa is, say "WM", make this "W.*M" because the perm could be + # 'RW+M', 'RW+CDM' etc, and they are all valid: $aaq =~ s/M/.*M/; $rc{RULE_TRACE} .= "A"; diff --git a/t/C-vs-C.t b/t/C-vs-C.t new file mode 100644 index 0000000..fee5cc4 --- /dev/null +++ b/t/C-vs-C.t @@ -0,0 +1,43 @@ +#!/usr/bin/perl +use strict; +use warnings; + +# the commit message in which this test is introduced should have details, but +# briefly, this test makes sure that access() does not get confused by +# repo-create permissions being allowed, when looking for branch-create +# permissions. + +# this is hardcoded; change it if needed +use lib "src/lib"; +use Gitolite::Test; + +# branch permissions test +# ---------------------------------------------------------------------- + +try "plan 25"; + +confreset;confadd ' + repo foo/..* + C = @all + RW+CD = CREATOR + RW = u2 + +'; + +try "ADMIN_PUSH set1; !/FATAL/" or die text(); + +try " + cd ..; ok + glt clone u1 file:///foo/aa; ok + cd aa; ok + tc l-1; ok; /master/ + glt push u1 origin master:m1; ok; /To file:///foo/aa/ + /\\* \\[new branch\\] master -> m1/ + + tc l-2; ok; /master/ + glt push u2 origin master:m2; !ok; /FATAL: C/ + /DENIED by fallthru/ + glt push u2 origin master:m1; ok; /To file:///foo/aa/ + /8cd302a..29b8683/ + /master -> m1/ +"; |