Re: Loophole in cvs_acls script allows restricted files to be committed

[Geoff Beier]

My apologies for such a long message... it's a bit of a complex issue.
Delete now if you're not interested :-)

On Thu, Dec 18, 2003 at 10:09:40AM -0800, Peter Connolly wrote:
> There appears to be a loophole in the cvs_acls script that allows 
> someone to bypass an 'unavail' on a specific file and commit changes to 
> that file.
> 
> It seems that all one needs to do is update another file in that same
> directory. Then a commit of that unrestricted file will include the
> restricted file, which commits successfully.
> 
> The avail file would look something like this:
> 
> unavail||CVSROOT/avail
> avail|cvsadmin|CVSROOT/avail
> 
> So that only 'cvsadmin' should be able to update the 'avail' file.
> 
You've really got a loophole in your config file. cvs_acls is behaving
as documented, although I would not consider this desirable behavior.
The cvs_acls script states that:

# PROGRAM LOGIC:
#
#       CVS passes to @ARGV an absolute directory pathname (the
#       repository
#       appended to your $CVSROOT variable), followed by a list of
#       filenames
#       within that directory.
#
#       We walk through the avail file looking for a line that matches
#       the
#       username, repository and branch.
#

Note that, although this is not emphasized here, for an "unavail" to
apply, we must have a perfect match. Further in the comments, the author
explains just what constitutes a repository match:

#       A repository match is either:
#               - One element of the third column matches $ARGV[0], or
#               some
#                 parent directory of $ARGV[0].
#               - Otherwise *all* file arguments ($ARGV[1..$#ARGV]) must
#               be
#                 in the file list in one avail line.
#           - In other words, using directory names in the third column
#           of
#             the avail file allows committing of any file (or group of
#             files in a single commit) in the tree below that
#             directory.
#           - If individual file names are used in the third column of
#             the avail file, then files must be committed individually
#             or
#             all files specified in a single commit must all appear in
#             third column of a single avail line.

This is slightly ambiguous. If you read the script, though, "avail line"
means "a line in the avail file beginning with 'avail' or 'unavail'.
Note, though, that this means lines which specify individual files (as
opposed to directories) will require an exact match of all parameters in
order to apply to multiple file commits.

The configuration file you detail:
unavail||CVSROOT/avail
avail|cvsadmin|CVSROOT/avail
<<EOF
specifies a "default allow" policy. This means that unless one of the
lines in the file matches, a commit will be allowed.

Now, looking at your two commits:

>    address@hidden ssh]$ vi CVSROOT/avail
>    address@hidden ssh]$ cvs ci -m"" CVSROOT/avail
>    cvs commit: Examining CVSROOT
>    address@hidden's password:
>    **** Access denied: Insufficient permission for this dir/file
> (wimp|CVSROOT|)
>    cvs commit: Pre-commit check failed
>    cvs [commit aborted]: correct above errors first!
Your commit was CVSROOT/avail, which exactly matched line 1 and not line
2, therefore the commit was disallowed.

>    address@hidden ssh]$ vi CVSROOT/loginfo
>    address@hidden ssh]$ cvs ci -m"" CVSROOT/loginfo
>    cvs commit: Examining CVSROOT
>    address@hidden's password:
>    Checking in CVSROOT/avail;
>    /usr/cvsroot/CVSROOT/avail,v  <--  avail
>    new revision: 1.7; previous revision: 1.6
>    done
>    Checking in CVSROOT/loginfo;
>    /usr/cvsroot/CVSROOT/loginfo,v  <--  loginfo
>    new revision: 1.139; previous revision: 1.138
>    done
>    cvs commit: Rebuilding administrative file database
>
Now, your commit uncovers what I'd consider a bug in cvs; the commit
*should* only include CVSROOT/loginfo, but there appears to be an error
in the option handling such that if you supply the argument -m"" to
commit, it behaves as though you'd typed
cvs ci -m"" .

At any rate, you'd have uncovered the loophole in your configuration if
you had typed cvs ci -m"" ., so looking at that, you can see that your
commit is:
CVSROOT/avail
CVSROOT/loginfo
Line 1 of your config does not match, and line 2 does not either. Since
your default policy is to allow the commit, cvs_acls returns 0 and your
commit is allowed. 

There are two possible solutions to your problem:
1. Change the behavior of cvs_acls to treat "avail" lines and "unavail"
lines differently; you'd want "unavail" lines to treat *any* file as a
repository match but "avail" lines to retain their current behavior.
Note that this does not address any of the other limitations of the
script, but is otherwise a sensible change. I *think* this would do it,
but don't have many rules to test against myself... use this completely
at your own risk; I have barely tested it at all, and there may well be
a logic bomb in it as big as the one it fixes:
--- cvs_acls    Thu Dec 18 14:53:44 2003
+++ cvs_acls_patched Thu Dec 18 17:26:35 2003
@@ -162,7 +162,11 @@
        if (!$in_repo) {
            $in_repo = 1;
            for $j (@ARGV) {
-               last if !($in_repo = grep ($_ eq $j, @tmp));
+               if( 1 == $flag ) {
+                       last if ($in_repo = grep ($_ eq $j, @tmp));
+               } else {
+                       last if !($in_repo = grep ($_ eq $j, @tmp));
+               }
            }
        }
     }


2. Change the way you write your acls. Your behavior will be more in
line with what you expect if you write your unavail directives based on
directories rather than files. So to achieve what you want:
unavail CVSROOT
avail|cvsadmin|CVSROOT/avail
avail|wimpy|CVSROOT/loginfo
<<<EOF

I'd recommend option 2 here, or better yet, just using filesystem
permissions if you can.

Hope this helps,

Geoff