NetBSD Problem Report #23362

Received: (qmail 28626 invoked by uid 605); 3 Nov 2003 19:04:24 -0000
Message-Id: <20031103200417.5d9ca5ce.christianbiere@gmx.de>
Date: Mon, 3 Nov 2003 20:04:17 +0100
From: Christian Biere <christianbiere@gmx.de>
Sender: gnats-bugs-owner@NetBSD.org
To: gnats-bugs@gnats.netbsd.org
Subject: usermod doesn't check for overflow of uid/gid

>Number:         23362
>Category:       bin
>Synopsis:       usermod doesn't check for overflow of uid/gid
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Nov 03 19:05:00 +0000 2003
>Closed-Date:    
>Last-Modified:  Mon Nov 03 20:24:00 +0000 2003
>Originator:     Christian Biere
>Release:        NetBSD 1.6ZD
>Organization:
>Environment:

>Description:

usermod uses atoi() to parse its arguments. atoi() shouldn't be used in
*any* half-serious program because it has no defined indicator for any
errors.

>How-To-Repeat:

# usermod -u 10000000000000 juser
$ id juser
uid=2147483647(juser) gid=1(users) groups=1(users)

>Fix:

I've replaced atoi() at the concerned places with another function which
uses strtoul() and returns errors if applicable. I've also replaced int
with gid_t and uid_t because these are the proper types for UIDs and
GIDs.




--Multipart=_Mon__3_Nov_2003_20_04_17_+0100_BDNB68O_tTzp=7bi
Content-Type: text/plain;
 name="user.c.udif"
Content-Disposition: attachment;
 filename="user.c.udif"
Content-Transfer-Encoding: 7bit

--- user.c	2003/10/23 03:16:06	1.1
+++ user.c	2003/11/03 18:29:31
@@ -45,6 +45,7 @@
 #include <ctype.h>
 #include <dirent.h>
 #include <err.h>
+#include <errno.h>
 #include <fcntl.h>
 #include <grp.h>
 #include <paths.h>
@@ -65,14 +66,14 @@

 /* this struct describes a uid range */
 typedef struct range_t {
-	int	r_from;		/* low uid */
-	int	r_to;		/* high uid */
+	uid_t	r_from;		/* low uid */
+	uid_t	r_to;		/* high uid */
 } range_t;

 /* this struct encapsulates the user information */
 typedef struct user_t {
 	int		u_flags;		/* see below */
-	int		u_uid;			/* uid of user */
+	uid_t	        u_uid;			/* uid of user */
 	char	       *u_password;		/* encrypted password */
 	char	       *u_comment;		/* comment field */
 	char	       *u_home;			/* home directory */
@@ -226,7 +227,7 @@

 /* remove a users home directory, returning 1 for success (ie, no problems encountered) */
 static int
-removehomedir(const char *user, int uid, const char *dir)
+removehomedir(const char *user, uid_t uid, const char *dir)
 {
 	struct stat st;

@@ -361,6 +362,40 @@
 	return 1;
 }

+/* string to unsigned long (enhanced)
+ *
+ * Returns the nul-terminated string `str' converted to an unsigned long.
+ * If `errorcode' is not zero an error occured.
+ * If `endptr' is not NULL it will point to the first invalid character.
+ * See strtoul(3) for more details about valid and invalid inputs. 
+ */
+static unsigned long
+strtoul_eh(const char *str, char **endptr, int *errorcode)
+{
+	char *ep;
+	unsigned long ret;
+	int old_errno = errno;
+
+	if (NULL == endptr)
+		endptr = &ep;
+
+	errno = 0;
+	ret = strtoul(str, endptr, 10);
+	if (str == *endptr) {
+		*errorcode = EINVAL;
+		ret = 0;
+	} else {
+		if (0 != errno) {
+			*errorcode = ERANGE;
+			ret = 0;
+		} else
+			*errorcode = 0;
+	}
+
+	errno = old_errno;
+	return ret;
+}
+
 /*
  * check that the effective uid is 0 - called from funcs which will
  * modify data and config files.
@@ -375,7 +410,7 @@

 /* copy any dot files into the user's home directory */
 static int
-copydotfiles(char *skeldir, int uid, int gid, char *dir)
+copydotfiles(char *skeldir, uid_t uid, gid_t gid, char *dir)
 {
 	struct dirent	*dp;
 	DIR		*dirp;
@@ -399,14 +434,14 @@
 		(void) asystem("cd %s && %s -rw -pe %s . %s", 
 				skeldir, PAX, (verbose) ? "-v" : "", dir);
 	}
-	(void) asystem("%s -R -h %d:%d %s", CHOWN, uid, gid, dir);
+	(void) asystem("%s -R -h %d:%d %s", CHOWN, (int)uid, (int)gid, dir);
 	(void) asystem("%s -R u+w %s", CHMOD, dir);
 	return n;
 }

 /* create a group entry with gid `gid' */
 static int
-creategid(char *group, int gid, const char *name)
+creategid(char *group, gid_t gid, const char *name)
 {
 	struct stat	st;
 	FILE		*from;
@@ -450,7 +485,7 @@
 			return 0;
 		}
 	}
-	(void) fprintf(to, "%s:*:%d:%s\n", group, gid, name);
+	(void) fprintf(to, "%s:*:%d:%s\n", group, (int)gid, name);
 	(void) fclose(from);
 	(void) fclose(to);
 	if (rename(f, _PATH_GROUP) < 0) {
@@ -459,7 +494,7 @@
 		return 0;
 	}
 	(void) chmod(_PATH_GROUP, st.st_mode & 07777);
-	syslog(LOG_INFO, "new group added: name=%s, gid=%d", group, gid);
+	syslog(LOG_INFO, "new group added: name=%s, gid=%d", group, (int)gid);
 	return 1;
 }

@@ -665,10 +700,10 @@

 /* find the next gid in the range lo .. hi */
 static int
-getnextgid(int *gidp, int lo, int hi)
+getnextgid(gid_t *gidp, gid_t lo, gid_t hi)
 {
 	for (*gidp = lo ; *gidp < hi ; *gidp += 1) {
-		if (getgrgid((gid_t)*gidp) == NULL) {
+		if (getgrgid(*gidp) == NULL) {
 			return 1;
 		}
 	}
@@ -680,9 +715,9 @@
 static int
 save_range(user_t *up, char *cp)
 {
-	int	from;
-	int	to;
-	int	i;
+	uid_t	from;
+	uid_t	to;
+	uid_t	i;

 	if (up->u_rsize == 0) {
 		up->u_rsize = 32;
@@ -865,10 +900,10 @@

 /* return the next valid unused uid */
 static int
-getnextuid(int sync_uid_gid, int *uid, int low_uid, int high_uid)
+getnextuid(int sync_uid_gid, uid_t *uid, uid_t low_uid, uid_t high_uid)
 {
 	for (*uid = low_uid ; *uid <= high_uid ; (*uid)++) {
-		if (getpwuid((uid_t)(*uid)) == NULL && *uid != NOBODY_UID) {
+		if (getpwuid(*uid) == NULL && *uid != NOBODY_UID) {
 			if (sync_uid_gid) {
 				if (getgrgid((gid_t)(*uid)) == NULL) {
 					return 1;
@@ -957,7 +992,7 @@
 	int		sync_uid_gid;
 	int		masterfd;
 	int		ptmpfd;
-	int		gid;
+	gid_t		gid;
 	int		cc;
 	int		i;

@@ -985,7 +1020,7 @@
 	}
 	/* if no uid was specified, get next one in [low_uid..high_uid] range */
 	sync_uid_gid = (strcmp(up->u_primgrp, "=uid") == 0);
-	if (up->u_uid == -1) {
+	if (up->u_uid == (uid_t)-1) {
 		int	got_id = 0;

 		/*
@@ -1008,14 +1043,14 @@
 		if (!got_id) {
 			(void) close(ptmpfd);
 			pw_abort();
-			errx(EXIT_FAILURE, "can't get next uid for %d", up->u_uid);
+			errx(EXIT_FAILURE, "can't get next uid for %d", (int) up->u_uid);
 		}
 	}
 	/* check uid isn't already allocated */
-	if (!(up->u_flags & F_DUPUID) && getpwuid((uid_t)(up->u_uid)) != NULL) {
+	if (!(up->u_flags & F_DUPUID) && getpwuid(up->u_uid) != NULL) {
 		(void) close(ptmpfd);
 		pw_abort();
-		errx(EXIT_FAILURE, "uid %d is already in use", up->u_uid);
+		errx(EXIT_FAILURE, "uid %d is already in use", (int) up->u_uid);
 	}
 	/* if -g=uid was specified, check gid is unused */
 	if (sync_uid_gid) {
@@ -1024,7 +1059,7 @@
 			pw_abort();
 			errx(EXIT_FAILURE, "gid %d is already in use", up->u_uid);
 		}
-		gid = up->u_uid;
+		gid = (gid_t)up->u_uid;
 	} else if ((grp = getgrnam(up->u_primgrp)) != NULL) {
 		gid = grp->gr_gid;
 	} else if (is_number(up->u_primgrp) &&
@@ -1073,8 +1108,8 @@
 	cc = snprintf(buf, sizeof(buf), "%s:%s:%d:%d:%s:%ld:%ld:%s:%s:%s\n",
 			login_name,
 			password,
-			up->u_uid,
-			gid,
+			(int) up->u_uid,
+			(int) gid,
 #ifdef EXTENSIONS
 			up->u_class,
 #else
@@ -1109,7 +1144,7 @@
 	    !creategid(login_name, gid, login_name)) {
 		(void) close(ptmpfd);
 		pw_abort();
-		errx(EXIT_FAILURE, "can't create gid %d for login name %s", gid, login_name);
+		errx(EXIT_FAILURE, "can't create gid %d for login name %s", (int)gid, login_name);
 	}
 	if (up->u_groupc > 0 && !append_group(login_name, up->u_groupc, up->u_groupv)) {
 		(void) close(ptmpfd);
@@ -1129,7 +1164,7 @@
 	}
 #endif
 	syslog(LOG_INFO, "new user added: name=%s, uid=%d, gid=%d, home=%s, shell=%s", 
-		login_name, up->u_uid, gid, home, up->u_shell);
+		login_name, (int)up->u_uid, (int)gid, home, up->u_shell);
 	return 1;
 }

@@ -1321,10 +1356,10 @@
 		}
 		if (up->u_flags & F_UID) {
 			/* check uid isn't already allocated */
-			if (!(up->u_flags & F_DUPUID) && getpwuid((uid_t)(up->u_uid)) != NULL) {
+			if (!(up->u_flags & F_DUPUID) && getpwuid(up->u_uid) != NULL) {
 				(void) close(ptmpfd);
 				pw_abort();
-				errx(EXIT_FAILURE, "uid %d is already in use", up->u_uid);
+				errx(EXIT_FAILURE, "uid %d is already in use", (int)up->u_uid);
 			}
 			pwp->pw_uid = up->u_uid;
 		}
@@ -1336,7 +1371,7 @@
 					pw_abort();
 					errx(EXIT_FAILURE, "gid %d is already in use", up->u_uid);
 				}
-				pwp->pw_gid = up->u_uid;
+				pwp->pw_gid = (gid_t)up->u_uid;
 			} else if ((grp = getgrnam(up->u_primgrp)) != NULL) {
 				pwp->pw_gid = grp->gr_gid;
 			} else if (is_number(up->u_primgrp) &&
@@ -1387,8 +1422,8 @@
 									      ":%ld:%ld:%s:%s:%s\n",
 					newlogin,
 					pwp->pw_passwd,
-					pwp->pw_uid,
-					pwp->pw_gid,
+					(int) pwp->pw_uid,
+					(int) pwp->pw_gid,
 #ifdef EXTENSIONS
 					pwp->pw_class,
 #endif
@@ -1447,10 +1482,10 @@
 		syslog(LOG_INFO, "user removed: name=%s", login_name);
 	} else if (strcmp(login_name, newlogin) == 0) {
 		syslog(LOG_INFO, "user information modified: name=%s, uid=%d, gid=%d, home=%s, shell=%s", 
-			login_name, pwp->pw_uid, pwp->pw_gid, pwp->pw_dir, pwp->pw_shell);
+			login_name, (int)pwp->pw_uid, (int)pwp->pw_gid, pwp->pw_dir, pwp->pw_shell);
 	} else {
 		syslog(LOG_INFO, "user information modified: name=%s, new name=%s, uid=%d, gid=%d, home=%s, shell=%s", 
-			login_name, newlogin, pwp->pw_uid, pwp->pw_gid, pwp->pw_dir, pwp->pw_shell);
+			login_name, newlogin, (int)pwp->pw_uid, (int)pwp->pw_gid, pwp->pw_dir, pwp->pw_shell);
 	}
 	return 1;
 }
@@ -1549,13 +1584,15 @@
 	int	defaultfield;
 	int	bigD;
 	int	c;
+        unsigned long val;
+        int     ec;
 #ifdef EXTENSIONS
 	int	i;
 #endif

 	(void) memset(&u, 0, sizeof(u));
 	read_defaults(&u);
-	u.u_uid = -1;
+	u.u_uid = (uid_t)-1;
 	defaultfield = bigD = 0;
 	while ((c = getopt(argc, argv, "DG:b:c:d:e:f:g:k:mou:s:" ADD_OPT_EXTENSIONS)) != -1) {
 		switch(c) {
@@ -1631,7 +1668,14 @@
 			if (!is_number(optarg)) {
 				errx(EXIT_FAILURE, "When using [-u uid], the uid must be numeric");
 			}
-			u.u_uid = atoi(optarg);
+			val = strtoul_eh(optarg, NULL, &ec);
+			if (ec) {
+				errx(EXIT_FAILURE, "Invalid uid: %s", strerror(ec));
+			}
+			if (val > UID_MAX) {
+				errx(EXIT_FAILURE, "The given uid is too big");
+			}
+			u.u_uid = (uid_t)val;
 			break;
 #ifdef EXTENSIONS
 		case 'v':
@@ -1659,7 +1703,7 @@
 		(void) printf("expire\t\t%s\n", (u.u_expire == NULL) ? UNSET_EXPIRY : u.u_expire);
 #ifdef EXTENSIONS
 		for (i = 0 ; i < u.u_rc ; i++) {
-			(void) printf("range\t\t%d..%d\n", u.u_rv[i].r_from, u.u_rv[i].r_to);
+			(void) printf("range\t\t%d..%d\n", (int)u.u_rv[i].r_from, (int)u.u_rv[i].r_to);
 		}
 #endif
 		return EXIT_SUCCESS;
@@ -1685,7 +1729,8 @@
 {
 	user_t	u;
 	char	newuser[MaxUserNameLen + 1];
-	int	c, have_new_user;
+	int	c, have_new_user, ec;
+	unsigned long val;

 	(void) memset(&u, 0, sizeof(u));
 	(void) memset(newuser, 0, sizeof(newuser));
@@ -1756,7 +1801,14 @@
 			if (!is_number(optarg)) {
 				errx(EXIT_FAILURE, "When using [-u uid], the uid must be numeric");
 			}
-			u.u_uid = atoi(optarg);
+			val = strtoul_eh(optarg, NULL, &ec);
+			if (ec) {
+				errx(EXIT_FAILURE, "Invalid uid: %s", strerror(ec));
+			}
+			if (val > UID_MAX) {
+				errx(EXIT_FAILURE, "The given uid is too big");
+			}
+			u.u_uid = (uid_t)val;
 			u.u_flags |= F_UID;
 			break;
 #ifdef EXTENSIONS
@@ -1882,10 +1934,12 @@
 groupadd(int argc, char **argv)
 {
 	int	dupgid;
-	int	gid;
+	gid_t	gid;
 	int	c;
+	int	ec;
+	unsigned long val;

-	gid = -1;
+	gid = (gid_t)-1;
 	dupgid = 0;
 	while ((c = getopt(argc, argv, "g:o" GROUP_ADD_OPT_EXTENSIONS)) != -1) {
 		switch(c) {
@@ -1893,7 +1947,14 @@
 			if (!is_number(optarg)) {
 				errx(EXIT_FAILURE, "When using [-g gid], the gid must be numeric");
 			}
-			gid = atoi(optarg);
+			val = strtoul_eh(optarg, NULL, &ec);
+			if (ec) {
+				errx(EXIT_FAILURE, "Invalid gid: %s", strerror(ec));
+			}
+			if (val > UID_MAX) {
+				errx(EXIT_FAILURE, "The given gid is too big");
+			}
+			gid = (gid_t)val;
 			break;
 		case 'o':
 			dupgid = 1;
@@ -1917,8 +1978,8 @@
 	if (gid < 0 && !getnextgid(&gid, LowGid, HighGid)) {
 		err(EXIT_FAILURE, "can't add group: can't get next gid");
 	}
-	if (!dupgid && getgrgid((gid_t) gid) != NULL) {
-		errx(EXIT_FAILURE, "can't add group: gid %d is a duplicate", gid);
+	if (!dupgid && getgrgid(gid) != NULL) {
+		errx(EXIT_FAILURE, "can't add group: gid %d is a duplicate", (int)gid);
 	}
 	if (!valid_group(*argv)) {
 		warnx("warning - invalid group name `%s'", *argv);
@@ -1982,11 +2043,13 @@
 	char		*newname;
 	char		**cpp;
 	int		dupgid;
-	int		gid;
+	gid_t		gid;
 	int		cc;
 	int		c;
+        int             ec;
+        unsigned long   val;

-	gid = -1;
+	gid = (gid_t)-1;
 	dupgid = 0;
 	newname = NULL;
 	while ((c = getopt(argc, argv, "g:on:" GROUP_MOD_OPT_EXTENSIONS)) != -1) {
@@ -1995,7 +2058,14 @@
 			if (!is_number(optarg)) {
 				errx(EXIT_FAILURE, "When using [-g gid], the gid must be numeric");
 			}
-			gid = atoi(optarg);
+			val = strtoul_eh(optarg, NULL, &ec);
+			if (ec) {
+				errx(EXIT_FAILURE, "Invalid gid: %s", strerror(ec));
+			}
+			if (val > GID_MAX) {
+				errx(EXIT_FAILURE, "The given gid is too big");
+			}
+			gid = (gid_t)val;
 			break;
 		case 'o':
 			dupgid = 1;
@@ -2037,7 +2107,7 @@
 	cc = snprintf(buf, sizeof(buf), "%s:%s:%d:",
 			(newname) ? newname : grp->gr_name,
 			grp->gr_passwd,
-			(gid < 0) ? grp->gr_gid : gid);
+			(int)(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) ? "" : ",");
@@ -2100,7 +2170,7 @@
 		}
 	}
 	if ((grp = getgrgid(pwp->pw_gid)) == NULL) {
-		(void) printf("groups\t%d %s\n", pwp->pw_gid, buf);
+		(void) printf("groups\t%d %s\n", (int)pwp->pw_gid, buf);
 	} else {
 		(void) printf("groups\t%s %s\n", grp->gr_name, buf);
 	}
@@ -2154,7 +2224,7 @@
 	}
 	(void) printf("name\t%s\n", grp->gr_name);
 	(void) printf("passwd\t%s\n", grp->gr_passwd);
-	(void) printf("gid\t%d\n", grp->gr_gid);
+	(void) printf("gid\t%d\n", (int)grp->gr_gid);
 	(void) printf("members\t");
 	for (cpp = grp->gr_mem ; *cpp ; cpp++) {
 		(void) printf("%s ", *cpp);

--Multipart=_Mon__3_Nov_2003_20_04_17_+0100_BDNB68O_tTzp=7bi--
>Release-Note:
>Audit-Trail:

From: David Laight <david@l8s.co.uk>
To: netbsd-bugs@netbsd.org
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/23362: usermod doesn't check for overflow of uid/gid
Date: Mon, 3 Nov 2003 20:03:42 +0000

 > >Synopsis:       usermod doesn't check for overflow of uid/gid
 ...
 > usermod uses atoi() to parse its arguments. atoi() shouldn't be used in
 > *any* half-serious program because it has no defined indicator for any
 > errors.
 > 
 > >How-To-Repeat:
 > 
 > # usermod -u 10000000000000 juser
 > $ id juser
 > uid=2147483647(juser) gid=1(users) groups=1(users)

 See: http://www.opengroup.org/onlinepubs/007904975/xrat/xcu_chap01.html#tag_02_01_07_03

 In particular the last part of the paragraph which says:

     The ISO C standard requires that a C compiler must issue a diagnostic
     for constants that are too large to represent.  Most standard utilities
     are not required to issue these diagnostics; for example, the command:

 	diff -C 2147483648 file1 file2

     has undefined behavior, and the diff utility is not required to issue a
     diagnostic even if the number 2147483648 cannot be represented.

 So it isn't necessary to make every utility check for numeric input
 overflow.

 	David

 -- 
 David Laight: david@l8s.co.uk

From: Christian Biere <christianbiere@gmx.de>
To: David Laight <david@l8s.co.uk>
Cc: netbsd-bugs@netbsd.org, gnats-bugs@gnats.netbsd.org
Subject: Re: bin/23362: usermod doesn't check for overflow of uid/gid
Date: Mon, 3 Nov 2003 21:22:52 +0100

 David Laight <david@l8s.co.uk> wrote:

 >     The ISO C standard requires that a C compiler must issue a
 >     diagnostic for constants that are too large to represent.

 I'm not sure whether the code of atoi() resp. strtol() ever creates
 a too large value. AFAIK, this actually the error indication of
 strtol i.e., LONG_MAX. So there doesn't happen an overflow on the
 C layer.

 > Most standard utilities are not required to issue these diagnostics;
 > for example, the command:
 > 
 > 	diff -C 2147483648 file1 file2
 > 
 >     has undefined behavior, and the diff utility is not required to
 >     issue a diagnostic even if the number 2147483648 cannot be
 >     represented.

 Actually, neither diff nor usermod are C compilers, so such argument
 cannot be applied, IMHO. As a user I don't have to know whether a
 program was coded in C, Java, Python, Perl or whatever. But I can
 expect that the program behaves as documented. If there are any
 arbitrary limits for input values, they should be documented. I really
 don't understand the diff example. How am I supposed to know what
 kind of variables are used inside the code? Does that depended on the
 platform? Of course, I know that every computer has limited memory
 resources and humans have limited time resources.
 OK, usermod isn't specified by any standard, so it could as well
 format all my partitions and eat my pets. Is that what you're trying
 to tell me?

 -- 
 Christian
>Unformatted:
 This is a multi-part message in MIME format.

 --Multipart=_Mon__3_Nov_2003_20_04_17_+0100_BDNB68O_tTzp=7bi
 Content-Type: text/plain; charset=US-ASCII
 Content-Transfer-Encoding: 7bit

Home	PR Database Search