NetBSD Problem Report #13974

Received: (qmail 22851 invoked from network); 16 Sep 2001 17:56:11 -0000
Message-Id: <200109161756.f8GHuCM16017@miyu.feyrer.net>
Date: Sun, 16 Sep 2001 19:56:12 +0200 (MEST)
From: Hubert Feyrer <hubert@feyrer.de>
Reply-To: hubert.feyrer@rz.uni-regensburg.de
To: gnats-bugs@gnats.netbsd.org
Subject: usermod/add etc. group handling problems
X-Send-Pr-Version: 3.95

>Number:         13974
>Category:       bin
>Synopsis:       usermod/add etc. group handling problems
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Sep 16 17:57:01 +0000 2001
>Closed-Date:    
>Last-Modified:  Thu Jan 24 16:13:41 +0000 2008
>Originator:     Hubert Feyrer
>Release:        NetBSD 4.0, originally submitted for NetBSD 1.5.1
>Organization:
bla!
>Environment:

System: 
 * NetBSD 4.0/i386
 * NetBSD miyu 1.5.2 NetBSD 1.5.2 (MIYU) #13: Tue Sep 11 22:00:05 MEST 2001 feyrer@miyu:/usr/cvs/src-1.5/sys/arch/i386/compile/MIYU i386


>Description:
	Looking at handling setups with many users as e.g. found on
	cvs.netbsd.org, I found several problems with useradd/usermod
	WRT group handling. 

	1. When adding users, the max. 1000 line limit in our
	   routines is not taken care of. Bad thing is if a line in
	   /etc/group grows beyond 1000 bytes, NONE of the users 
	   will belong to that group any more.
	2. When adding a user to a group manually (usermod -G), and
	   /etc/group has the same group more than once (to work around
	   the afore-mentioned 1000 bytes limit), the user is added
	   to to each line
	3. When removing a user (userdel), he is left in /etc/group.

>How-To-Repeat:
	0. Add "test2" group: groupadd test2

	1. * Create a bunch of accounts:
	     perl -e 'for($i=0;$i<200;$i++){printf "useradd test%03d; \
		usermod -d /tmp -G test2 test%03d\n", $i, $i;}' | sh -v
	   * Change to last user: su - test200
	   * Run "id" to see the user is not in the "test2" group
	   * exit back to r00t
	   * Change to first user: su - test000
	   * Run "id" to see the user is not in the "test2" group either
	   * grep ^test2:  /etc/group | wc -c, and see the line is longer
	     than 1000 bytes
	   * Split it into two lines ("test2:*:111:test000,...,test100",
	     "test2:*:111:test101,...,test200") and see the above "su"
	     commands show the user being member of "test2"

	2. * Add a new user: useradd -d /tmp test201
	   * Add the user to our (now-split) test2-group: usermod -G test2 test201
	   * grep ^test2: /etc/group and note that test201 is now on ALL
	     lines of test2 groups!
	   * Play with "useradd -G", and notice the same problem

	3. * Remove all users:
	     perl -e 'for($i=0;$i<200;$i++){printf "userdel test%03d\n", \
		      $i;}' | sh -v
           * Notice /etc/group still lists all users as part of the
	     test2 group

>Fix:
	Unknown.

	Need to read all users of each group into some list, and when
	writing that out pay attention to the line limit.

	As an alternative, fix our group handling (but that's probably
	not gonna happen), so fixing user* is the easier thing here.
>Release-Note:
>Audit-Trail:

From: "Arne H. Juul" <arnej@pvv.ntnu.no>
To: gnats-bugs@gnats.netbsd.org
Cc: Hubert Feyrer <hubert@feyrer.de>, Alistair Crooks <agc@NetBSD.org>
Subject: Re: bin/13974
Date: Sun, 14 Mar 2004 13:25:28 +0100 (CET)

 Here is a patch that fixes several problems related
 to long group lines; this one auto-splits long groups
 when adding a user would overflow the line limit.

 It also detects if any old lines were too long and just exits
 instead of possibly making things worse, like it would before.

 Line limits are still hardcoded but now
 to the same values as getgrent and getpwent have
 hard-coded in them.

 I've also made it detect when adding some secondary
 groups for a user would overflow NGROUPS_MAX, and
 fixed a couple of bugs related to this.

 Index: usr.sbin/user/user.c
 ===================================================================
 RCS file: /usr/cvs/src/usr.sbin/user/user.c,v
 retrieving revision 1.75
 diff -u -r1.75 user.c
 --- usr.sbin/user/user.c	14 Jan 2004 09:35:33 -0000	1.75
 +++ usr.sbin/user/user.c	14 Mar 2004 13:09:21 -0000
 @@ -167,8 +167,8 @@
  	MaxUserNameLen = 32,
  	MaxFieldNameLen = 32,
  	MaxCommandLen = 2048,
 -	MaxEntryLen = 2048,
 -	PasswordLength = 2048,
 +	MaxEntryLen = 1024,
 +	PasswordLength = 1024,

  	DES_Len = 13,

 @@ -473,7 +473,6 @@
  	FILE		*to;
  	char		buf[MaxEntryLen];
  	char		f[MaxFileNameLen];
 -	char		*colon;
  	int		groupc;
  	int		entc;
  	int		fd;
 @@ -502,18 +501,42 @@
  	}
  	groupc = strlen(group);
  	while (fgets(buf, sizeof(buf), from) != NULL) {
 +		char	*firstcolon;
 +		char	*thirdcolon;
 +
  		cc = strlen(buf);
 -		if ((colon = strchr(buf, ':')) == NULL) {
 -			warn("badly formed entry `%s'", buf);
 -			continue;
 +		/*
 +		 * group entries are name : pwd : gid : members,
 +		 * so look for three colons
 +		 */
 +		thirdcolon = firstcolon = strchr(buf, ':');
 +		if (thirdcolon != NULL) thirdcolon = strchr(thirdcolon+1, ':');
 +		if (thirdcolon != NULL) thirdcolon = strchr(thirdcolon+1, ':');
 +
 +		if (thirdcolon == NULL || buf[cc-1] != '\n') {
 +			warnx("badly formed entry `%s'", buf);
 +			(void) fclose(from);
 +			(void) close(fd);
 +			(void) unlink(f);
 +			return 0;
  		}
 -		entc = (int)(colon - buf);
 +		entc = (int)(firstcolon - buf);
  		if (entc == groupc && strncmp(group, buf, (unsigned) entc) == 0) {
  			if (newent == NULL) {
  				continue;
  			} else {
 -				cc = strlen(newent);
 -				(void) strlcpy(buf, newent, sizeof(buf));
 +				char buf2[2*MaxEntryLen];
 +
 +				(void)strlcpy(buf2, newent, sizeof(buf2));
 +				(void)strlcat(buf2, thirdcolon+1, sizeof(buf2));
 +				cc = strlcpy(buf, buf2, sizeof(buf));
 +				if (cc >= sizeof(buf)) {
 +					(void) fclose(from);
 +					(void) close(fd);
 +					(void) unlink(f);
 +					warnx("too long group entry '%s'", buf2);
 +					return 0;
 +				}
  			}
  		}
  		if (fwrite(buf, sizeof(char), (unsigned) cc, to) != cc) {
 @@ -542,7 +565,7 @@

  /* modify the group entries for all `groups', by adding `user' */
  static int
 -append_group(char *user, int ngroups, const char **groups)
 +append_group(char *user, gid_t gid, int ngroups, const char **groups)
  {
  	struct group	*grp;
  	struct stat	st;
 @@ -558,19 +581,40 @@
  	int		cc;
  	int		i;
  	int		j;
 +	gid_t		oldgrouplist[NGROUPS_MAX];
 +	int		numoldg = NGROUPS_MAX;
 +
 +	nc = getgrouplist(user, gid, oldgrouplist, &numoldg);
 +	if (nc < 0) {
 +		if (numoldg > NGROUPS_MAX)
 +			warnx("user in %d groups already, max is %d",
 +				numoldg, NGROUPS_MAX);
 +		else
 +			warn("can't get grouplist for user `%s'", user);
 +		return 0;
 +	}

 +	/* stupid n*m algorithm is probably OK */
  	for (i = 0 ; i < ngroups ; i++) {
 -		if ((grp = getgrnam(groups[i])) == NULL) {
 -			warnx("can't append group `%s' for user `%s'", groups[i], user);
 -		} else {
 -			for (j = 0 ; grp->gr_mem[j] ; j++) {
 -				if (strcmp(user, grp->gr_mem[j]) == 0) {
 -					/* already in it */
 -					groups[i] = "";
 -				}
 +		for (j = 0; j < numoldg; j++) {
 +			grp = getgrgid(oldgrouplist[j]);
 +			if (grp == NULL) {
 +				warn("can't getgrgid `%d'", oldgrouplist[j]);
 +			}
 +			if (strcmp(groups[i], grp->gr_name) == 0) {
 +				/* already in it */
 +				oldgrouplist[j] = oldgrouplist[--numoldg];
 +				groups[i] = "";
 +				break;
  			}
  		}
  	}
 +	if (ngroups + numoldg > NGROUPS_MAX) {
 +		warnx("can't be member of %d groups (max is %d)",
 +			ngroups + numoldg, NGROUPS_MAX);
 +		return 0;
 +	}
 +
  	if ((from = fopen(_PATH_GROUP, "r")) == NULL) {
  		warn("can't append group for `%s': can't open `%s'", user, _PATH_GROUP);
  		return 0;
 @@ -594,9 +638,13 @@
  	}
  	while (fgets(buf, sizeof(buf), from) != NULL) {
  		cc = strlen(buf);
 -		if ((colon = strchr(buf, ':')) == NULL) {
 -			warn("badly formed entry `%s'", buf);
 -			continue;
 +		if ((colon = strchr(buf, ':')) == NULL || buf[cc-1] != '\n') {
 +			warnx("Error: badly formed group entry `%s'", buf);
 +			/* it's not safe to continue */
 +			(void) fclose(from);
 +			(void) close(fd);
 +			(void) unlink(f);
 +			return 0;
  		}
  		entc = (int)(colon - buf);
  		for (i = 0 ; i < ngroups ; i++) {
 @@ -604,14 +652,38 @@
  				continue;
  			}
  			if (entc == groupc && strncmp(groups[i], buf, (unsigned) entc) == 0) {
 -				if ((nc = snprintf(&buf[cc - 1],
 -						sizeof(buf) - cc + 1,
 -						"%s%s\n",
 -						(buf[cc - 2] == ':') ? "" : ",",
 -						user)) < 0) {
 -					warnx("Warning: group `%s' entry too long", groups[i]);
 +				/* need space for old group entry, comma, username, newline */
 +				if (cc + strlen(user) + 2 < sizeof(buf)) {
 +					nc = snprintf(&buf[cc - 1],
 +							sizeof(buf) - (cc - 1),
 +							"%s%s\n",
 +							(buf[cc - 2] == ':') ? "" : ",",
 +							user);
 +					if (nc < 0 || nc > sizeof(buf) - (cc-1)) {
 +						/* this should not happen */
 +						errx(EXIT_FAILURE, "Error: snprintf failed on group `%s'", groups[i]);
 +					}
 +					cc += nc - 1;
 +				} else {
 +					char buf2[MaxEntryLen];
 +					warnx("Warning: group `%s' entry too long, splitting", groups[i]);
 +					colon = strrchr(buf, ':');
 +					nc = snprintf(buf2, sizeof(buf2), "%.*s:%s\n", colon - buf, buf, user);
 +					if (nc < 0 || nc > sizeof(buf2)) {
 +						warnx("Error: cannot even make split group `%s' entry", buf2);
 +					} else {
 +						warnx("new entry: '%s'", buf2);
 +						if (fwrite(buf2, sizeof(char), (unsigned) nc, to) != nc) {
 +							(void) fclose(from);
 +							(void) close(fd);
 +							(void) unlink(f);
 +							warn("can't create gid: short write to `%s'", f);
 +							return 0;
 +						}
 +					}
  				}
 -				cc += nc - 1;
 +				/* done this once now, avoid doing it again */
 +				groups[i] = "";
  			}
  		}
  		if (fwrite(buf, sizeof(char), (unsigned) cc, to) != cc) {
 @@ -1118,7 +1190,7 @@
  		pw_abort();
  		errx(EXIT_FAILURE, "can't create gid %d for login name %s", gid, login_name);
  	}
 -	if (up->u_groupc > 0 && !append_group(login_name, up->u_groupc, up->u_groupv)) {
 +	if (up->u_groupc > 0 && !append_group(login_name, gid, up->u_groupc, up->u_groupv)) {
  		(void) close(ptmpfd);
  		pw_abort();
  		errx(EXIT_FAILURE, "can't append `%s' to new groups", login_name);
 @@ -1425,7 +1497,7 @@
  				homedir, pwp->pw_dir);
  		}
  		if (up->u_groupc > 0 &&
 -		    !append_group(newlogin, up->u_groupc, up->u_groupv)) {
 +		    !append_group(newlogin, pwp->pw_gid, up->u_groupc, up->u_groupv)) {
  			(void) close(ptmpfd);
  			pw_abort();
  			errx(EXIT_FAILURE, "can't append `%s' to new groups",
 @@ -1566,8 +1638,9 @@
  			bigD = 1;
  			break;
  		case 'G':
 -			while ((u.u_groupv[u.u_groupc] = strsep(&optarg, ",")) != NULL &&
 -			       u.u_groupc < NGROUPS_MAX) {
 +			while (u.u_groupc < NGROUPS_MAX &&
 +				(u.u_groupv[u.u_groupc] = strsep(&optarg, ",")) != NULL)
 +			{
  				if (u.u_groupv[u.u_groupc][0] != 0) {
  					u.u_groupc++;
  				}
 @@ -1702,8 +1775,9 @@
  	while ((c = getopt(argc, argv, "G:c:d:e:f:g:l:mos:u:" MOD_OPT_EXTENSIONS)) != -1) {
  		switch(c) {
  		case 'G':
 -			while ((u.u_groupv[u.u_groupc] = strsep(&optarg, ",")) != NULL &&
 -			       u.u_groupc < NGROUPS_MAX) {
 +			while (u.u_groupc < NGROUPS_MAX &&
 +				(u.u_groupv[u.u_groupc] = strsep(&optarg, ",")) != NULL)
 +			{
  				if (u.u_groupv[u.u_groupc][0] != 0) {
  					u.u_groupc++;
  				}
 @@ -1998,7 +2072,6 @@
  	struct group	*grp;
  	char		buf[MaxEntryLen];
  	char		*newname;
 -	char		**cpp;
  	int		dupgid;
  	int		gid;
  	int		cc;
 @@ -2056,11 +2129,10 @@
  			(newname) ? newname : grp->gr_name,
  			grp->gr_passwd,
  			(gid < 0) ? grp->gr_gid : gid);
 -	for (cpp = grp->gr_mem ; *cpp && cc < sizeof(buf) ; cpp++) {
 -		cc += snprintf(&buf[cc], sizeof(buf) - cc, "%s%s", *cpp,
 -			(cpp[1] == NULL) ? "" : ",");
 +	/* we need some space for members */
 +	if (2*cc > sizeof(buf)) {
 +		errx(EXIT_FAILURE, "group entry '%s' too long", buf);
  	}
 -	cc += snprintf(&buf[cc], sizeof(buf) - cc, "\n");
  	openlog("groupmod", LOG_PID, LOG_USER);
  	if (!modify_gid(*argv, buf)) {
  		err(EXIT_FAILURE, "can't change %s file", _PATH_GROUP);
>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.