Re: user: handle paths with whitespace / metacharacters
On 2023/05/18 11:28:58 -0600, Todd C. Miller wrote:
> On Thu, 18 May 2023 18:13:57 +0200, Omar Polo wrote:
> > > + 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).
I couldn't have come up with something better.
> Updated diff follows.
ok for me.
Thanks!
Re: user: handle paths with whitespace / metacharacters
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
> > #include
> > #include
> > @@ -103,6 +104,10 @@ enum {
> > F_ACCTUNLOCK= 0x1
> > };
> >
> > +/* 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.c16 May 2023 21:28:46 - 1.130
+++ usr.sbin/user/user.c18 May 2023 17:24:01 -
@@ -31,6 +31,7 @@
#include
#include
+#include
#include
#include
@@ -42,7 +43,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -103,6 +103,12 @@ enum {
F_ACCTUNLOCK= 0x1
};
+/* 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;
- charbuf[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)
+
Re: user: handle paths with whitespace / metacharacters
On 2023/05/18 09:11:59 -0600, Todd C. Miller wrote:
> 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.
when I saw that the previous change was to remove the only "cd xyz &&
cmd" I hoped that the follow up was to use exec instead of system :)
> OK?
verified that it works as intended. -v is actually nicer now, since
it logs the commands it executes (which is expected as per man page
but not as per previous behaviour.)
for what is worth ok op@ (with or without nits below)
> - 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 - 1.130
> +++ usr.sbin/user/user.c 18 May 2023 15:10:52 -
> @@ -31,6 +31,7 @@
>
> #include
> #include
> +#include
>
> #include
> #include
> @@ -42,7 +43,7 @@
> #include
> #include
> #include
> -#include
> +#include
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
> #include
> #include
> #include
> @@ -103,6 +104,10 @@ enum {
> F_ACCTUNLOCK= 0x1
> };
>
> +/* bit flags for runas() */
> +#define RUNAS_DUP_DEVNULL0x01
> +#define RUNAS_IGNORE_EXITVAL 0x02
another nit: the other flags are defined within an enums.
> [...]
> -/* 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".
"unable to run" makes me think that it failed to exec the
program, rather than it failed at runtime.
> + }
> + 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);
> }
user: handle paths with whitespace / metacharacters
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.c16 May 2023 21:28:46 - 1.130
+++ usr.sbin/user/user.c18 May 2023 15:10:52 -
@@ -31,6 +31,7 @@
#include
#include
+#include
#include
#include
@@ -42,7 +43,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
@@ -103,6 +104,10 @@ enum {
F_ACCTUNLOCK= 0x1
};
+/* 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;
- charbuf[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);
