Refactor input validation

Mutualize code to validate inputs of both 'user' and 'group' command
Test that the input name fits into MAXLOGNAME

5 files changed, 117 insertions, 103 deletions

diff --git a/usr.sbin/pw/pw.c b/usr.sbin/pw/pw.c
index c81b5c40e4ce..7274c32f3fca 100644
--- a/usr.sbin/pw/pw.c
+++ b/usr.sbin/pw/pw.c
@@ -99,9 +99,11 @@ main(int argc, char *argv[])
 	int             ch;
 	int             mode = -1;
 	int             which = -1;
+	long		id = -1;
 	char		*config = NULL;
 	struct stat	st;
-	char		arg;
+	const char	*errstr;
+	char		arg, *name;
 	bool		relocated, nis;
 
 	static const char *opts[W_NUM][M_NUM] =
@@ -124,12 +126,14 @@ main(int argc, char *argv[])
 		 }
 	};
 
-	static int      (*funcs[W_NUM]) (int _mode, struct cargs * _args) =
+	static int      (*funcs[W_NUM]) (int _mode, char *_name, long _id,
+	    struct cargs * _args) =
 	{			/* Request handlers */
 		pw_user,
 		pw_group
 	};
 
+	name = NULL;
 	relocated = nis = false;
 	memset(&conf, 0, sizeof(conf));
 	strlcpy(conf.etcpath, _PATH_PWD, sizeof(conf.etcpath));
@@ -190,9 +194,15 @@ main(int argc, char *argv[])
 			mode = tmp % M_NUM;
 		} else if (strcmp(argv[1], "help") == 0 && argv[2] == NULL)
 			cmdhelp(mode, which);
-		else if (which != -1 && mode != -1)
-			addarg(&arglist, 'n', argv[1]);
-		else
+		else if (which != -1 && mode != -1) {
+			if (strspn(argv[1], "0123456789") == strlen(argv[1])) {
+				id = strtonum(argv[1], 0, LONG_MAX, &errstr);
+				if (errstr != NULL)
+					errx(EX_USAGE, "Bad id '%s': %s",
+					    argv[1], errstr);
+			} else
+				name = argv[1];
+		} else
 			errx(EX_USAGE, "unknown keyword `%s'", argv[1]);
 		++argv;
 		--argc;
@@ -230,6 +240,30 @@ main(int argc, char *argv[])
 		case 'Y':
 			nis = true;
 			break;
+		case 'g':
+			if (which == 0) { /* for user* */
+				addarg(&arglist, 'g', optarg);
+				break;
+			}
+			/* FALLTHROUGH */
+		case 'u':
+			if (strspn(optarg, "0123456789") != strlen(optarg))
+				errx(EX_USAGE, "%s expects a number",
+				    which == 1 ? "-g" : "-u" );
+			id = strtonum(optarg, 0, LONG_MAX, &errstr);
+			if (errstr != NULL)
+				errx(EX_USAGE, "Bad id '%s': %s", optarg,
+				    errstr);
+			break;
+		case 'n':
+			if (strspn(optarg, "0123456789") != strlen(optarg)) {
+				name = optarg;
+				break;
+			}
+			id = strtonum(optarg, 0, LONG_MAX, &errstr);
+			if (errstr != NULL)
+				errx(EX_USAGE, "Bad id '%s': %s", optarg,
+				    errstr);
 		default:
 			addarg(&arglist, ch, optarg);
 			break;
@@ -237,6 +271,9 @@ main(int argc, char *argv[])
 		optarg = NULL;
 	}
 
+	if (name != NULL && strlen(name) >= MAXLOGNAME)
+		errx(EX_USAGE, "name too long: %s", name);
+
 	/*
 	 * Must be root to attempt an update
 	 */
@@ -265,7 +302,7 @@ main(int argc, char *argv[])
 	 */
 	conf.userconf = read_userconfig(config);
 
-	ch = funcs[which] (mode, &arglist);
+	ch = funcs[which] (mode, name, id, &arglist);
 
 	/*
 	 * If everything went ok, and we've been asked to update
diff --git a/usr.sbin/pw/pw.h b/usr.sbin/pw/pw.h
index 9c0aa329a8d8..6239004b61f7 100644
--- a/usr.sbin/pw/pw.h
+++ b/usr.sbin/pw/pw.h
@@ -82,8 +82,8 @@ int write_userconfig(char const * file);
 struct carg *addarg(struct cargs * _args, int ch, char *argstr);
 struct carg *getarg(struct cargs * _args, int ch);
 
-int pw_user(int mode, struct cargs * _args);
-int pw_group(int mode, struct cargs * _args);
+int pw_user(int mode, char *name, long id, struct cargs * _args);
+int pw_group(int mode, char *name, long id,  struct cargs * _args);
 char *pw_checkname(char *name, int gecos);
 
 int addnispwent(const char *path, struct passwd *pwd);
diff --git a/usr.sbin/pw/pw_group.c b/usr.sbin/pw/pw_group.c
index d5d7d5236903..b06b59ecaa1e 100644
--- a/usr.sbin/pw/pw_group.c
+++ b/usr.sbin/pw/pw_group.c
@@ -48,12 +48,10 @@ static int	print_group(struct group * grp);
 static gid_t    gr_gidpolicy(struct userconf * cnf, struct cargs * args);
 
 int
-pw_group(int mode, struct cargs * args)
+pw_group(int mode, char *name, long id, struct cargs * args)
 {
 	int		rc;
 	struct carg    *a_newname = getarg(args, 'l');
-	struct carg    *a_name = getarg(args, 'n');
-	struct carg    *a_gid = getarg(args, 'g');
 	struct carg    *arg;
 	struct group   *grp = NULL;
 	int	        grmembers = 0;
@@ -68,11 +66,6 @@ pw_group(int mode, struct cargs * args)
 		NULL
 	};
 
-	if (a_gid != NULL) {
-		if (strspn(a_gid->val, "0123456789") != strlen(a_gid->val))
-			errx(EX_USAGE, "-g expects a number");
-	}
-
 	if (mode == M_LOCK || mode == M_UNLOCK)
 		errx(EX_USAGE, "'lock' command is not available for groups");
 
@@ -95,67 +88,63 @@ pw_group(int mode, struct cargs * args)
 		ENDGRENT();
 		return EXIT_SUCCESS;
 	}
-	if (a_gid == NULL) {
-		if (a_name == NULL)
-			errx(EX_DATAERR, "group name or id required");
+	if (id < 0 && name == NULL)
+		errx(EX_DATAERR, "group name or id required");
 
-		if (mode != M_ADD && grp == NULL && isdigit((unsigned char)*a_name->val)) {
-			(a_gid = a_name)->ch = 'g';
-			a_name = NULL;
-		}
-	}
-	grp = (a_name != NULL) ? GETGRNAM(a_name->val) : GETGRGID((gid_t) atoi(a_gid->val));
+	grp = (name != NULL) ? GETGRNAM(name) : GETGRGID(id);
 
 	if (mode == M_UPDATE || mode == M_DELETE || mode == M_PRINT) {
-		if (a_name == NULL && grp == NULL)	/* Try harder */
-			grp = GETGRGID(atoi(a_gid->val));
+		if (name == NULL && grp == NULL)	/* Try harder */
+			grp = GETGRGID(id);
 
 		if (grp == NULL) {
 			if (mode == M_PRINT && getarg(args, 'F')) {
 				char	*fmems[1];
 				fmems[0] = NULL;
-				fakegroup.gr_name = a_name ? a_name->val : "nogroup";
-				fakegroup.gr_gid = a_gid ? (gid_t) atol(a_gid->val) : (gid_t)-1;
+				fakegroup.gr_name = name ? name : "nogroup";
+				fakegroup.gr_gid = (gid_t) id;
 				fakegroup.gr_mem = fmems;
 				return print_group(&fakegroup);
 			}
-			errx(EX_DATAERR, "unknown group `%s'", a_name ? a_name->val : a_gid->val);
+			if (name == NULL)
+				errx(EX_DATAERR, "unknown group `%s'", name);
+			else
+				errx(EX_DATAERR, "unknown group `%ld'", id);
 		}
-		if (a_name == NULL)	/* Needed later */
-			a_name = addarg(args, 'n', grp->gr_name);
+		if (name == NULL)	/* Needed later */
+			name = grp->gr_name;
 
 		/*
 		 * Handle deletions now
 		 */
 		if (mode == M_DELETE) {
-			gid_t           gid = grp->gr_gid;
-
 			rc = delgrent(grp);
 			if (rc == -1)
-				err(EX_IOERR, "group '%s' not available (NIS?)", grp->gr_name);
+				err(EX_IOERR, "group '%s' not available (NIS?)",
+				    name);
 			else if (rc != 0) {
 				err(EX_IOERR, "group update");
 			}
-			pw_log(cnf, mode, W_GROUP, "%s(%u) removed", a_name->val, gid);
+			pw_log(cnf, mode, W_GROUP, "%s(%ld) removed", name, id);
 			return EXIT_SUCCESS;
 		} else if (mode == M_PRINT)
 			return print_group(grp);
 
-		if (a_gid)
-			grp->gr_gid = (gid_t) atoi(a_gid->val);
+		if (id > 0)
+			grp->gr_gid = (gid_t) id;
 
 		if (a_newname != NULL)
 			grp->gr_name = pw_checkname(a_newname->val, 0);
 	} else {
-		if (a_name == NULL)	/* Required */
+		if (name == NULL)	/* Required */
 			errx(EX_DATAERR, "group name required");
 		else if (grp != NULL)	/* Exists */
-			errx(EX_DATAERR, "group name `%s' already exists", a_name->val);
+			errx(EX_DATAERR, "group name `%s' already exists", name);
 
 		extendarray(&members, &grmembers, 200);
 		members[0] = NULL;
 		grp = &fakegroup;
-		grp->gr_name = pw_checkname(a_name->val, 0);
+		grp->gr_name = pw_checkname(name, 0);
 		grp->gr_passwd = "*";
 		grp->gr_gid = gr_gidpolicy(cnf, args);
 		grp->gr_mem = members;
@@ -265,7 +254,7 @@ pw_group(int mode, struct cargs * args)
 			    grp->gr_name);
 		else
 			err(EX_IOERR, "group update");
-	} else if (mode == M_UPDATE && (rc = chggrent(a_name->val, grp)) != 0) {
+	} else if (mode == M_UPDATE && (rc = chggrent(name, grp)) != 0) {
 		if (rc == -1)
 			errx(EX_IOERR, "group '%s' not available (NIS?)",
 			    grp->gr_name);
@@ -273,9 +262,10 @@ pw_group(int mode, struct cargs * args)
 			err(EX_IOERR, "group update");
 	}
 
-	arg = a_newname != NULL ? a_newname : a_name;
+	if (a_newname != NULL)
+		name = a_newname->val;
 	/* grp may have been invalidated */
-	if ((grp = GETGRNAM(arg->val)) == NULL)
+	if ((grp = GETGRNAM(name)) == NULL)
 		errx(EX_SOFTWARE, "group disappeared during update");
 
 	pw_log(cnf, mode, W_GROUP, "%s(%u)", grp->gr_name, grp->gr_gid);
diff --git a/usr.sbin/pw/pw_user.c b/usr.sbin/pw/pw_user.c
index 0b15c7706db2..c9093bb9e73a 100644
--- a/usr.sbin/pw/pw_user.c
+++ b/usr.sbin/pw/pw_user.c
@@ -52,7 +52,7 @@ static const char rcsid[] =
 static		char locked_str[] = "*LOCKED*";
 
 static int	delete_user(struct userconf *cnf, struct passwd *pwd,
-		    struct carg *a_name, int delete, int mode);
+		    char *name, int delete, int mode);
 static int	print_user(struct passwd * pwd);
 static uid_t    pw_uidpolicy(struct userconf * cnf, struct cargs * args);
 static uid_t    pw_gidpolicy(struct cargs * args, char *nam, gid_t prefer);
@@ -119,13 +119,11 @@ create_and_populate_homedir(int mode, struct passwd *pwd)
  */
 
 int
-pw_user(int mode, struct cargs * args)
+pw_user(int mode, char *name, long id, struct cargs * args)
 {
 	int	        rc, edited = 0;
 	char           *p = NULL;
 	char					 *passtmp;
-	struct carg    *a_name;
-	struct carg    *a_uid;
 	struct carg    *arg;
 	struct passwd  *pwd = NULL;
 	struct group   *grp;
@@ -166,7 +164,7 @@ pw_user(int mode, struct cargs * args)
 		if (getarg(args, 'q'))
 			return next;
 		printf("%u:", next);
-		pw_group(mode, args);
+		pw_group(mode, name, -1, args);
 		return EXIT_SUCCESS;
 	}
 
@@ -323,29 +321,11 @@ pw_user(int mode, struct cargs * args)
 		return EXIT_SUCCESS;
 	}
 
-	if ((a_name = getarg(args, 'n')) != NULL)
-		pwd = GETPWNAM(pw_checkname(a_name->val, 0));
-	a_uid = getarg(args, 'u');
+	if (name != NULL)
+		pwd = GETPWNAM(pw_checkname(name, 0));
 
-	if (a_uid == NULL) {
-		if (a_name == NULL)
-			errx(EX_DATAERR, "user name or id required");
-
-		/*
-		 * Determine whether 'n' switch is name or uid - we don't
-		 * really don't really care which we have, but we need to
-		 * know.
-		 */
-		if (mode != M_ADD && pwd == NULL
-		    && strspn(a_name->val, "0123456789") == strlen(a_name->val)
-		    && *a_name->val) {
-			(a_uid = a_name)->ch = 'u';
-			a_name = NULL;
-		}
-	} else {
-		if (strspn(a_uid->val, "0123456789") != strlen(a_uid->val))
-			errx(EX_USAGE, "-u expects a number");
-	}
+	if (id < 0 && name == NULL)
+		errx(EX_DATAERR, "user name or id required");
 
 	/*
 	 * Update, delete & print require that the user exists
@@ -353,22 +333,22 @@ pw_user(int mode, struct cargs * args)
 	if (mode == M_UPDATE || mode == M_DELETE ||
 	    mode == M_PRINT  || mode == M_LOCK   || mode == M_UNLOCK) {
 
-		if (a_name == NULL && pwd == NULL)	/* Try harder */
-			pwd = GETPWUID(atoi(a_uid->val));
+		if (name == NULL && pwd == NULL)	/* Try harder */
+			pwd = GETPWUID(id);
 
 		if (pwd == NULL) {
 			if (mode == M_PRINT && getarg(args, 'F')) {
-				fakeuser.pw_name = a_name ? a_name->val : "nouser";
-				fakeuser.pw_uid = a_uid ? (uid_t) atol(a_uid->val) : (uid_t) -1;
+				fakeuser.pw_name = name ? name : "nouser";
+				fakeuser.pw_uid = (uid_t) id;
 				return print_user(&fakeuser);
 			}
-			if (a_name == NULL)
-				errx(EX_NOUSER, "no such uid `%s'", a_uid->val);
-			errx(EX_NOUSER, "no such user `%s'", a_name->val);
+			if (name == NULL)
+				errx(EX_NOUSER, "no such uid `%ld'", id);
+			errx(EX_NOUSER, "no such user `%s'", name);
 		}
 
-		if (a_name == NULL)	/* May be needed later */
-			a_name = addarg(args, 'n', newstr(pwd->pw_name));
+		if (name == NULL)
+			name = pwd->pw_name;
 
 		/*
 		 * The M_LOCK and M_UNLOCK functions simply add or remove
@@ -394,7 +374,7 @@ pw_user(int mode, struct cargs * args)
 			pwd->pw_passwd += sizeof(locked_str)-1;
 			edited = 1;
 		} else if (mode == M_DELETE)
-			return (delete_user(cnf, pwd, a_name,
+			return (delete_user(cnf, pwd, name,
 				    getarg(args, 'r') != NULL, mode));
 		else if (mode == M_PRINT)
 			return print_user(pwd);
@@ -511,16 +491,16 @@ pw_user(int mode, struct cargs * args)
 		 * Add code
 		 */
 
-		if (a_name == NULL)	/* Required */
+		if (name == NULL)	/* Required */
 			errx(EX_DATAERR, "login name required");
-		else if ((pwd = GETPWNAM(a_name->val)) != NULL)	/* Exists */
-			errx(EX_DATAERR, "login name `%s' already exists", a_name->val);
+		else if ((pwd = GETPWNAM(name)) != NULL)	/* Exists */
+			errx(EX_DATAERR, "login name `%s' already exists", name);
 
 		/*
 		 * Now, set up defaults for a new user
 		 */
 		pwd = &fakeuser;
-		pwd->pw_name = a_name->val;
+		pwd->pw_name = name;
 		pwd->pw_class = cnf->default_class ? cnf->default_class : "";
 		pwd->pw_uid = pw_uidpolicy(cnf, args);
 		pwd->pw_gid = pw_gidpolicy(args, pwd->pw_name, (gid_t) pwd->pw_uid);
@@ -635,13 +615,13 @@ pw_user(int mode, struct cargs * args)
 		}
 	} else if (mode == M_UPDATE || mode == M_LOCK || mode == M_UNLOCK) {
 		if (edited) {	/* Only updated this if required */
-			rc = chgpwent(a_name->val, pwd);
+			rc = chgpwent(name, pwd);
 			if (rc == -1)
 				errx(EX_IOERR, "user '%s' does not exist (NIS?)", pwd->pw_name);
 			else if (rc != 0)
 				err(EX_IOERR, "passwd file update");
 			if ( cnf->nispasswd && *cnf->nispasswd=='/') {
-				rc = chgnispwent(cnf->nispasswd, a_name->val, pwd);
+				rc = chgnispwent(cnf->nispasswd, name, pwd);
 				if (rc == -1)
 					warn("User '%s' not found in NIS passwd", pwd->pw_name);
 				else
@@ -693,16 +673,16 @@ pw_user(int mode, struct cargs * args)
 
 
 	/* go get a current version of pwd */
-	pwd = GETPWNAM(a_name->val);
+	pwd = GETPWNAM(name);
 	if (pwd == NULL) {
 		/* This will fail when we rename, so special case that */
 		if (mode == M_UPDATE && (arg = getarg(args, 'l')) != NULL) {
-			a_name->val = arg->val;		/* update new name */
-			pwd = GETPWNAM(a_name->val);	/* refetch renamed rec */
+			name = arg->val;		/* update new name */
+			pwd = GETPWNAM(name);	/* refetch renamed rec */
 		}
 	}
 	if (pwd == NULL)	/* can't go on without this */
-		errx(EX_NOUSER, "user '%s' disappeared during update", a_name->val);
+		errx(EX_NOUSER, "user '%s' disappeared during update", name);
 
 	grp = GETGRGID(pwd->pw_gid);
 	pw_log(cnf, mode, W_USER, "%s(%u):%s(%u):%s:%s:%s",
@@ -849,7 +829,6 @@ pw_gidpolicy(struct cargs * args, char *nam, gid_t prefer)
 		char            tmp[32];
 
 		LIST_INIT(&grpargs);
-		addarg(&grpargs, 'n', nam);
 
 		/*
 		 * We need to auto-create a group with the user's name. We
@@ -866,11 +845,11 @@ pw_gidpolicy(struct cargs * args, char *nam, gid_t prefer)
 		}
 		if (conf.dryrun) {
 			addarg(&grpargs, 'q', NULL);
-			gid = pw_group(M_NEXT, &grpargs);
+			gid = pw_group(M_NEXT, nam, -1, &grpargs);
 		}
 		else
 		{
-			pw_group(M_ADD, &grpargs);
+			pw_group(M_ADD, nam, -1, &grpargs);
 			if ((grp = GETGRNAM(nam)) != NULL)
 				gid = grp->gr_gid;
 		}
@@ -1048,7 +1027,7 @@ pw_password(struct userconf * cnf, struct cargs * args, char const * user)
 }
 
 static int
-delete_user(struct userconf *cnf, struct passwd *pwd, struct carg *a_name,
+delete_user(struct userconf *cnf, struct passwd *pwd, char *name,
     int delete, int mode)
 {
 	char		 file[MAXPATHLEN];
@@ -1097,7 +1076,7 @@ delete_user(struct userconf *cnf, struct passwd *pwd, struct carg *a_name,
 		err(EX_IOERR, "passwd update");
 
 	if (cnf->nispasswd && *cnf->nispasswd=='/') {
-		rc = delnispwent(cnf->nispasswd, a_name->val);
+		rc = delnispwent(cnf->nispasswd, name);
 		if (rc == -1)
 			warnx("WARNING: user '%s' does not exist in NIS passwd", pwd->pw_name);
 		else if (rc != 0)
@@ -1105,11 +1084,11 @@ delete_user(struct userconf *cnf, struct passwd *pwd, struct carg *a_name,
 		/* non-fatal */
 	}
 
-	grp = GETGRNAM(a_name->val);
+	grp = GETGRNAM(name);
 	if (grp != NULL &&
 	    (grp->gr_mem == NULL || *grp->gr_mem == NULL) &&
-	    strcmp(a_name->val, grname) == 0)
-		delgrent(GETGRNAM(a_name->val));
+	    strcmp(name, grname) == 0)
+		delgrent(GETGRNAM(name));
 	SETGRENT();
 	while ((grp = GETGRENT()) != NULL) {
 		int i, j;
@@ -1118,7 +1097,7 @@ delete_user(struct userconf *cnf, struct passwd *pwd, struct carg *a_name,
 			continue;
 
 		for (i = 0; grp->gr_mem[i] != NULL; i++) {
-			if (strcmp(grp->gr_mem[i], a_name->val) != 0)
+			if (strcmp(grp->gr_mem[i], name) != 0)
 				continue;
 
 			for (j = i; grp->gr_mem[j] != NULL; j++)
@@ -1129,7 +1108,7 @@ delete_user(struct userconf *cnf, struct passwd *pwd, struct carg *a_name,
 	}
 	ENDGRENT();
 
-	pw_log(cnf, mode, W_USER, "%s(%u) account removed", a_name->val, uid);
+	pw_log(cnf, mode, W_USER, "%s(%u) account removed", name, uid);
 
 	if (!PWALTDIR()) {
 		/*
@@ -1150,7 +1129,7 @@ delete_user(struct userconf *cnf, struct passwd *pwd, struct carg *a_name,
 		    stat(home, &st) != -1) {
 			rm_r(home, uid);
 			pw_log(cnf, mode, W_USER, "%s(%u) home '%s' %sremoved",
-			       a_name->val, uid, home,
+			       name, uid, home,
 			       stat(home, &st) == -1 ? "" : "not completely ");
 		}
 	}
diff --git a/usr.sbin/pw/tests/pw_useradd.sh b/usr.sbin/pw/tests/pw_useradd.sh
index 48612ed59aec..27a8624b60ee 100755
--- a/usr.sbin/pw/tests/pw_useradd.sh
+++ b/usr.sbin/pw/tests/pw_useradd.sh
@@ -169,12 +169,19 @@ user_add_password_expiration_date_relative_body() {
 		atf_fail "Expiration time($TIME) was not within $EPOCH - $BUF seconds."
 }
 
+atf_test_case user_add_name_too_long
+user_add_name_too_long_body() {
+	populate_etc_skel
+	atf_check -e match:"too long" -s exit:64 \
+		${PW} useradd name_very_vert_very_very_very_long
+}
+
 atf_init_test_cases() {
 	atf_add_test_case user_add
 	atf_add_test_case user_add_noupdate
 	atf_add_test_case user_add_comments
 	atf_add_test_case user_add_comments_noupdate
-	atf_add_test_case user_add_comments_invalid 
+	atf_add_test_case user_add_comments_invalid
 	atf_add_test_case user_add_comments_invalid_noupdate
 	atf_add_test_case user_add_homedir
 	atf_add_test_case user_add_account_expiration_epoch
@@ -185,4 +192,5 @@ atf_init_test_cases() {
 	atf_add_test_case user_add_password_expiration_date_numeric
 	atf_add_test_case user_add_password_expiration_date_month
 	atf_add_test_case user_add_password_expiration_date_relative
+	atf_add_test_case user_add_name_too_long
 }