From 516c028b815fd54231bfac962b919ab5af600835 Mon Sep 17 00:00:00 2001
From: Sitaram Chamarty <sitaram@sita-lt.atc.tcs.com>
Date: Mon, 23 Nov 2009 22:45:00 +0530
Subject: [PATCH] compile: (oopsies...) plug security hole in delegation
 feature

I was trying to determine how close gitolite can come to the ACL model
of a proprietary product called codebeamer, and one of the items was how
to make a "role" (like QA_Lead) have different "members" in different
projects.

I then realised delegation already does that!  Which is great, but as I
thought about it more, I realised...  well, we'll let the in-code
comments speak for themselves :-)

Anyway, all it needed was a 1-line fix, luckily... <phew>  And it would
have only affected people who use delegation.
---
 src/gl-compile-conf | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/gl-compile-conf b/src/gl-compile-conf
index 247e5cc..77d340b 100755
--- a/src/gl-compile-conf
+++ b/src/gl-compile-conf
@@ -86,7 +86,7 @@ my $USERNAME_PATT=qr(^\@?[0-9a-zA-Z][0-9a-zA-Z._-]*$);      # very simple patter
 
 # $groups{group}{member} = "master" (or name of fragment file in which the
 # group is defined).
-my %groups = ();
+our %groups = ();
 
 # %repos has two functions.
 
@@ -282,6 +282,24 @@ parse_conf_file($GL_CONF, 'master');
 wrap_chdir($GL_ADMINDIR);
 for my $fragment_file (glob("conf/fragments/*.conf"))
 {
+    # we already check (elsewhere) that a fragment called "foo" will not try
+    # to specify access control for a repo whose name is not "foo" or is not
+    # part of a group called "foo" created by master
+
+    # meanwhile, I found a possible attack where the admin for group B creates
+    # a "convenience" group of (a subset of) his users, and then the admin for
+    # repo group A (alphabetically before B) adds himself to that same group
+    # in his own fragment.
+
+    # as a result, admin_A now has access to group B repos :(
+
+    # so now we lock the groups hash to the value it had after parsing
+    # "master", and localise any changes to it by this fragment so that they
+    # don't propagate to the next fragment.  Thus, each fragment now has only
+    # those groups that are defined in "master" and itself
+
+    local %groups = %groups;
+
     my $fragment = $fragment_file;
     $fragment =~ s/^conf\/fragments\/(.*).conf$/$1/;
     parse_conf_file($fragment_file, $fragment);