misc/109416: pam_group doesn't check login_group membership in some situations

[siflus]

>Number:         109416
>Category:       misc
>Synopsis:       pam_group doesn't check login_group membership in some situations
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Feb 22 02:40:06 GMT 2007
>Closed-Date:
>Last-Modified:
>Originator:     siflus
>Release:        6.2-RELEASE
>Organization:
>Environment:
FreeBSD trashed.local 6.2-RELEASE FreeBSD 6.2-RELEASE #0: Fri Jan 12 23:30:59 UTC 2007     root at s-dallas.cse.buffalo.edu:/usr/obj/usr/src/sys/GENERIC  sparc64

>Description:
if a group is empty as per /etc/group, pam_group.so fails
before checking if the user's login_group matches.

>How-To-Repeat:
set a user's login group to the gid of some group.
make sure there aren't any usernames in /etc/group

add this to something like /etc/pam.d/su
auth            required        pam_group.so            no_warn group=YOUR_GROUP

that way you're required to be in the wheel group in order to su.

try to su. :)
>Fix:
it looks to me like the author intended to check the gid..however due to
the initial check if the group is empty...it fails before it gets to that point.

---------- 8< ---- snip ---- 8< -----------
    if ((group = openpam_get_option(pamh, "group")) == NULL)
        group = "wheel";
    if ((grp = getgrnam(group)) == NULL || grp->gr_mem == NULL)
        goto failed;

A   /* check if the group is empty */
A   if (*grp->gr_mem == NULL)
A       goto failed;

B   /* check membership */
B   if (pwd->pw_gid == grp->gr_gid)
B       goto found;
    for (list = grp->gr_mem; *list != NULL; ++list)
        if (strcmp(*list, pwd->pw_name) == 0)
            goto found;

---------- 8< ---- snip ---- 8< -----------

Currently the logic is ->

if group.members is Empty:
    goto failed
if user.group = group.gid:
    goto found
if user in group.members:
    goto found

I think A and B should be swapped, that way the logic looks like ->

if user.group = group.gid:
    goto found
if group.members is empty:
    goto failed
if user in group.members:
    goto found


Patch attached with submission follows:

diff -cru libpam/modules/pam_group/pam_group.c /home/siflus/pam_group/pam_group.c
--- libpam/modules/pam_group/pam_group.c	Thu Dec 11 08:55:15 2003
+++ /home/siflus/pam_group/pam_group.c	Wed Feb 21 21:02:13 2007
@@ -80,13 +80,14 @@
 	if ((grp = getgrnam(group)) == NULL || grp->gr_mem == NULL)
 		goto failed;
 
-	/* check if the group is empty */
-	if (*grp->gr_mem == NULL)
-		goto failed;
-
 	/* check membership */
 	if (pwd->pw_gid == grp->gr_gid)
 		goto found;
+
+	/* check if there are no members in the group */
+	if (*grp->gr_mem == NULL)
+		goto failed;
+
 	for (list = grp->gr_mem; *list != NULL; ++list)
 		if (strcmp(*list, pwd->pw_name) == 0)
 			goto found;

>Release-Note:
>Audit-Trail:
>Unformatted: