Bug#407231: [Adduser-devel] Processed: Re: [Pkg-shadow-devel] Bug#407231: passwd: users may gain system group access on package installation by coincidence

[Marc Haber]

On Wed, Jan 17, 2007 at 12:12:52PM +0000, Stephen Gran wrote:
> This one time, at band camp, Marc Haber said:
> > I can reproduce this bug on sid adduser and have written (and
> > committed) a test suite case to catch this.
> > 
> > I would like the people who are more knowledgeable with that part of
> > the code to comment before I commit this.
> 
> The real problem (I think - reflected by the patch I just checked in) is
> that existing_group_ok is called with an uninitialized $new_gid.  This
> makes existing_group_ok return 1 (that behavior is probably wrong in
> existing_group_ok, but can be fixed later - for now, we just need to
> make sure we initialize $new_group before calling it).  I have checked
> in a fix that takes care to initialize $new_group before calling the
> function, and it looks like it works, although I haven't yet had time to
> run the test suite on it.

The new test still fails with your patch applied, and my patch (diff
attached) fixes it. I have not yet committed it since it is a pretty
severe change to a function that is used in different places.

I also think that existing_group_ok was kind of misdocumented.

Greetings
Marc

--- adduser     (revision 695)
+++ adduser     (working copy)
@@ -255,11 +255,11 @@
     }

     # Check if requested group already exists and we can exit safely
-    if (existing_group_ok($new_name, $new_gid) == 1) {
+    if (existing_group_ok($new_name, $new_gid) == 2) {
        printf (gtx("The group `%s' already exists as a system group. Exiting.\n"), $new_name) if $verbose;
        exit 0;
     }
-    if (existing_group_ok($new_name, $new_gid) == 2) {
+    if (existing_group_ok($new_name, $new_gid) == 1) {
        printf (gtx("The group `%s' already exists, but has a different GID. Exiting.\n"), $new_name) if $verbose;
        exit 1;
     }
@@ -697,21 +697,21 @@

 # returns 0 if the group doesn't exist or
 # returns 1 if the group already exists with the specified gid (or $new_gid wasn't specified)
-# returns 2 if the group already exists, but $new_gid doesn't match its gid
+# returns 2 if the group already exists as a system group
 sub existing_group_ok {
     my($new_name,$new_gid) = @_;
     my ($dummy1,$dummy2,$gid);
     if (($dummy1,$dummy2,$gid) = getgrnam($new_name)) {
+       if( $gid >= $config{"first_system_gid"} &&
+           $gid <= $config{"last_system_gid" } ) {
+               return 2;
+       }
        if( defined($new_gid) && $gid == $new_gid ) {
            return 1;
        }
        if (! defined($new_gid)) {
                return 1;
        }
-       if( $gid >= $config{"first_system_gid"} &&
-           $gid <= $config{"last_system_gid" } ) {
-               return 2;
-       }
     } else {
        return 0;
     }


-- 
-----------------------------------------------------------------------------
Marc Haber         | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany  |  lose things."    Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature |  How to make an American Quilt | Fax: *49 621 72739835