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

reassign 407231 adduser
retitle 407231 adduser: with addgroup, users may gain system group access on
package installation by coincidence
thanks

Quoting Leonard Norrgård (vinsci@refactor.fi):
> Package: passwd
> Version: 1:4.0.18.1-6
> Severity: critical
> Tags: security
> Justification: root security hole
> 
> 
> An ordinary user may end up with group ownership of system files
> in the following scenario [2]:
> 
>  1. A user is added, and receives the user and group ids, <name>.
>  2. Later, a package is installed that asks for an identically named
>     system group to be created, using ''addgroup --system
<name>''.
>  3. Addgroup returns with a success exit status, showing the message
>     ''The group `<name>'' already exists as a system
group. Exiting.",
>     even though the pre-existing <name> group, as a group added for
>     a user has a non-system id (ie. outside the range 100-999 [1].
>  4. The user <name> now has access to all system files that are
>     installed for the <name> group.
> 
> The problem occurs because in /usr/sbin/addgroup, the code on/after
> line 247 to existing_group_ok fails to check for and handle
> the situation where the existing GID is outside of the system GID
> boundaries.
> 
> [1] http://www.debian.org/doc/debian-policy/ch-opersys.html#s9.2.2)
> [2] I discovered this while working on the packaging for kvm, which
>     will create a ''kvm'' group, likely to collide with
existing user
>     id:s on some systems.

Thanks for your detailed explanations and bug report. I won''t go into
the details, essentially because this bug report is misdirected. At
first glance, you seem to be right and the bug seems easy to handle.

You identified the bug as a bug in the "addgroup" utility. However
"dpkg -S /usr/sbin/addgroup" will show you that this utility belongs
to the "adduser" package, not passwd.

I''m therefore reassigning this bug to adduser.

Again, thanks a lot for your care investigating this issue.




_______________________________________________
Adduser-devel mailing list
Adduser-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/mailman/listinfo/adduser-devel

Processing commands for control@bugs.debian.org:
> reassign 407231 adduser
Bug#407231: passwd: users may gain system group access on package installation
by coincidence
Bug reassigned from package `passwd'' to `adduser''.
> retitle 407231 adduser: with addgroup, users may gain system group access
on package installation by coincidence
Bug#407231: passwd: users may gain system group access on package installation
by coincidence
Changed Bug title.
> thanks
Stopping processing here.

Please contact me if you need assistance.

Debian bug tracking system administrator
(administrator, Debian Bugs database)

Processing commands for control@bugs.debian.org:
> tags #407231 confirmed
Bug#407231: adduser: with addgroup, users may gain system group access on
package installation by coincidence
Tags were: security
Tags added: confirmed
> thanks
Stopping processing here.

Please contact me if you need assistance.

Debian bug tracking system administrator
(administrator, Debian Bugs database)

tags #407231 confirmed
thanks

I can reproduce this bug on sid adduser and have written (and
committed) a test suite case to catch this.

I _think_ that this patch fixes the issue:
Index: adduser
==================================================================--- adduser   
(revision 689)
+++ adduser     (working copy)
@@ -244,11 +244,11 @@
 #################
 if ($action eq "addsysgroup") {
     # 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;
     }
@@ -695,21 +695,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;
     }

I would like the people who are more knowledgeable with that part of
the code to comment before I commit this.

Greetings
Marc

-- 
-----------------------------------------------------------------------------
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

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.
-- 
 -----------------------------------------------------------------
|   ,''''`.                                            Stephen
Gran |
|  : :'' :                                        sgran@debian.org |
|  `. `''                        Debian user, admin, and developer |
|    `-                                     http://www.debian.org |
 -----------------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url :
http://lists.alioth.debian.org/pipermail/adduser-devel/attachments/20070117/99062f08/attachment-0003.pgp

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

On Wed, Jan 17, 2007 at 02:43:49PM +0100, Marc Haber
wrote:
> The new test still fails with your patch applied, and my patch (diff
> attached) fixes it.
Stupid me. With your new code, the test is successful.

I have tweaked the test suite somewhat.

Can somebody please go over the diff beteween 3.101 and current svn
HEAD before I upload?

Greetings
Marc

-- 
-----------------------------------------------------------------------------
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

[Taking this from the BTS to the developer mailing list]

This is adduser from current svn HEAD (as is, Stephen''s fix for
#407231, not mine).

$ grep addusertest117 /etc/group
addusertest117:x:157:
$ sudo addgroup --system addusertest117
The group `addusertest117'' already exists, but has a different GID.
Exiting.
$ echo $?
1
$

I think this is wrong. addusertest117 is already existing a system
group, and the user has requested it to be created as a system group
with no preference regarding UID.

Thus, I''d expect addgroup to say "addusertest117 already exists as
a
system group" and exit with a zero exit code since the user''s
request
could be catered for (in fact, it was already fulfilled before).

It we upload as is, we''ll break every package with a postinst that
just does "addgroup --system foo".

Or am I missing something here?

Greetings
Marc

-- 
-----------------------------------------------------------------------------
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