The way user(8) runs commands via system(3) is fragile. It does
not correctly handle paths with whitespace or shell metacharacters.
Rather than try to quote everything (which is also fragile) I think
it is safest to just exec the commands directly.
OK?
- todd
Index: usr.sbin/user/user.c
===================================================================
RCS file: /cvs/src/usr.sbin/user/user.c,v
retrieving revision 1.130
diff -u -p -u -r1.130 user.c
--- usr.sbin/user/user.c 16 May 2023 21:28:46 -0000 1.130
+++ usr.sbin/user/user.c 18 May 2023 15:10:52 -0000
@@ -31,6 +31,7 @@
#include <sys/types.h>
#include <sys/stat.h>
+#include <sys/wait.h>
#include <ctype.h>
#include <dirent.h>
@@ -42,7 +43,7 @@
#include <login_cap.h>
#include <paths.h>
#include <pwd.h>
-#include <stdarg.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -103,6 +104,10 @@ enum {
F_ACCTUNLOCK = 0x10000
};
+/* bit flags for runas() */
+#define RUNAS_DUP_DEVNULL 0x01
+#define RUNAS_IGNORE_EXITVAL 0x02
+
#define CONFFILE "/etc/usermgmt.conf"
#define _PATH_NONEXISTENT "/nonexistent"
@@ -179,8 +184,6 @@ enum {
static int adduser(char *, user_t *);
static int append_group(char *, int, const char **);
-static int asystem(const char *, ...)
- __attribute__((__format__(__printf__, 1, 2)));
static int copydotfiles(char *, char *);
static int creategid(char *, gid_t, const char *);
static int getnextgid(uid_t *, uid_t, uid_t);
@@ -214,30 +217,77 @@ strsave(char **cpp, const char *s)
err(1, NULL);
}
-/* a replacement for system(3) */
+/* run the given command with optional arguments as the specified uid */
static int
-asystem(const char *fmt, ...)
+runas(const char *path, const char *const argv[], uid_t uid, int flags)
{
- va_list vp;
- char buf[MaxCommandLen];
- int ret;
+ int argc, status, ret = 0;
+ char buf[MaxCommandLen];
+ pid_t child;
- va_start(vp, fmt);
- (void) vsnprintf(buf, sizeof(buf), fmt, vp);
- va_end(vp);
- if (verbose) {
- printf("Command: %s\n", buf);
+ strlcpy(buf, path, sizeof(buf));
+ for (argc = 1; argv[argc] != NULL; argc++) {
+ strlcat(buf, " ", sizeof(buf));
+ strlcat(buf, argv[argc], sizeof(buf));
}
- if ((ret = system(buf)) != 0) {
- warnx("[Warning] can't system `%s'", buf);
+ if (verbose)
+ printf("Command: %s\n", buf);
+
+ child = fork();
+ switch (child) {
+ case -1:
+ err(EXIT_FAILURE, "fork");
+ case 0:
+ if (flags & RUNAS_DUP_DEVNULL) {
+ /* Redirect output to /dev/null if possible. */
+ int dev_null = open(_PATH_DEVNULL, O_RDWR);
+ if (dev_null != -1) {
+ dup2(dev_null, STDOUT_FILENO);
+ dup2(dev_null, STDERR_FILENO);
+ if (dev_null > STDERR_FILENO)
+ close(dev_null);
+ } else {
+ warn("%s", _PATH_DEVNULL);
+ }
+ }
+ if (uid != -1) {
+ if (setresuid(uid, uid, uid) == -1)
+ warn("setresuid(%u, %u, %u)", uid, uid, uid);
+ }
+ execv(path, (char **)argv);
+ warn("%s", buf);
+ _exit(EXIT_FAILURE);
+ default:
+ while (waitpid(child, &status, 0) == -1) {
+ if (errno != EINTR)
+ err(EXIT_FAILURE, "waitpid");
+ }
+ if (WIFSIGNALED(status)) {
+ ret = WTERMSIG(status);
+ warnx("[Warning] `%s' killed by signal %d", buf, ret);
+ ret |= 128;
+ } else {
+ if (!(flags & RUNAS_IGNORE_EXITVAL))
+ ret = WEXITSTATUS(status);
+ if (ret != 0)
+ warnx("[Warning] unable to run `%s'", buf);
+ }
+ return ret;
}
- return ret;
+}
+
+/* run the given command with optional arguments */
+static int
+run(const char *path, const char *const argv[])
+{
+ return runas(path, argv, -1, false);
}
/* remove a users home directory, returning 1 for success (ie, no problems
encountered) */
static int
removehomedir(const char *user, uid_t uid, const char *dir)
{
+ const char *rm_argv[] = { "rm", "-rf", dir, NULL };
struct stat st;
/* userid not root? */
@@ -263,11 +313,9 @@ removehomedir(const char *user, uid_t ui
return 0;
}
- (void) seteuid(uid);
- /* we add the "|| true" to keep asystem() quiet if there is a non-zero
exit status. */
- (void) asystem("%s -rf %s > /dev/null 2>&1 || true", RM, dir);
- (void) seteuid(0);
- if (rmdir(dir) == -1) {
+ /* run "rm -rf dir 2>&1/dev/null" as user, not root */
+ (void) runas(RM, rm_argv, uid, RUNAS_DUP_DEVNULL|RUNAS_IGNORE_EXITVAL);
+ if (rmdir(dir) == -1 && errno != ENOENT) {
warnx("Unable to remove all files in `%s'", dir);
return 0;
}
@@ -288,11 +336,12 @@ checkeuid(void)
/* copy any dot files into the user's home directory */
static int
-copydotfiles(char *skeldir, char *dir)
+copydotfiles(char *skeldir, char *dst)
{
+ char src[MaxFileNameLen];
struct dirent *dp;
DIR *dirp;
- int n;
+ int len, n;
if (*skeldir == '\0')
return 0;
@@ -311,8 +360,16 @@ copydotfiles(char *skeldir, char *dir)
if (n == 0) {
warnx("No \"dot\" initialisation files found");
} else {
- (void) asystem("%s -a %s %s/. %s",
- CP, (verbose) ? "-v" : "", skeldir, dir);
+ len = snprintf(src, sizeof(src), "%s/.", skeldir);
+ if (len < 0 || len >= sizeof(src)) {
+ warnx("skeleton directory `%s' too long", skeldir);
+ n = 0;
+ } else {
+ const char *cp_argv[] = { "cp", "-a", src, dst, NULL };
+ if (verbose)
+ cp_argv[1] = "-av";
+ run(CP, cp_argv);
+ }
}
return n;
}
@@ -1203,7 +1260,15 @@ adduser(char *login_name, user_t *up)
errx(EXIT_FAILURE, "home directory `%s' already exists",
home);
} else {
- if (asystem("%s -p %s", MKDIR, home) != 0) {
+ char idstr[64];
+ const char *mkdir_argv[] =
+ { "mkdir", "-p", home, NULL };
+ const char *chown_argv[] =
+ { "chown", "-RP", idstr, home, NULL };
+ const char *chmod_argv[] =
+ { "chmod", "-R", "u+w", home, NULL };
+
+ if (run(MKDIR, mkdir_argv) != 0) {
int saved_errno = errno;
close(ptmpfd);
pw_abort();
@@ -1211,9 +1276,10 @@ adduser(char *login_name, user_t *up)
"can't mkdir `%s'", home);
}
(void) copydotfiles(up->u_skeldir, home);
- (void) asystem("%s -R -P %u:%u %s", CHOWN, up->u_uid,
- gid, home);
- (void) asystem("%s -R u+w %s", CHMOD, home);
+ (void) snprintf(idstr, sizeof(idstr), "%u:%u",
+ up->u_uid, gid);
+ (void) run(CHOWN, chown_argv);
+ (void) run(CHMOD, chmod_argv);
}
}
if (strcmp(up->u_primgrp, "=uid") == 0 && !group_exists(login_name) &&
@@ -1657,8 +1723,9 @@ moduser(char *login_name, char *newlogin
}
}
if (up != NULL) {
+ const char *mv_argv[] = { "mv", homedir, pwp->pw_dir, NULL };
if ((up->u_flags & F_MKDIR) &&
- asystem("%s %s %s", MV, homedir, pwp->pw_dir) != 0) {
+ run(MV, mv_argv) != 0) {
int saved_errno = errno;
close(ptmpfd);
pw_abort();
