Subject: bin/23362: usermod doesn't check for overflow of uid/gid
To: None <gnats-bugs@gnats.netbsd.org>
From: Christian Biere <christianbiere@gmx.de>
List: netbsd-bugs
Date: 11/03/2003 20:04:17
>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 UTC 2003
>Closed-Date:
>Last-Modified:
>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:
>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