On Thu, 18 May 2023 18:13:57 +0200, Omar Polo wrote:
> existing code just used 0 and 1 (e.g. creategid.) lone zeros as
> arguments are not really readable, but still I can't make myself
> liking stdbool. not that my tastes matters, but it seems to be used
> very sparsingly in usr.sbin
This was leftover from before I added bit flags to runas().
It is not actually needed now so I have removed it.
> > #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
>
> another nit: the other flags are defined within an enums.
Sure, no reason to be inconsistent.
> > [...]
> > -/* 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)
> > {
> > [...]
> > + 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);
>
> more than "unable to run" I'd say "failed to run" or "command
> failed" / "exited with nonzero status".
How about "failed with status N"? It can be useful to have the
exit status since that may indicate the source of the error (though
perhaps not so much in this case).
Updated diff follows.
- 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 17:24:01 -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,6 @@
#include <login_cap.h>
#include <paths.h>
#include <pwd.h>
-#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -103,6 +103,12 @@ enum {
F_ACCTUNLOCK = 0x10000
};
+/* flags for runas() */
+enum {
+ RUNAS_DUP_DEVNULL = 0x01,
+ RUNAS_IGNORE_EXITVAL = 0x02
+};
+
#define CONFFILE "/etc/usermgmt.conf"
#define _PATH_NONEXISTENT "/nonexistent"
@@ -179,8 +185,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 +218,79 @@ 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] `%s' failed with status %d",
+ buf, ret);
+ }
+ }
+ 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, 0);
}
/* 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 +316,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 +339,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 +363,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 +1263,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 +1279,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 +1726,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();
