Re: [PATCH v2] sandbox/seunshare: Replace system() with execv() to prevent shell injection

Jason Zaman
2026-01-25T22:34:20Z

On Fri, Jan 23, 2026 at 10:03:10AM -0500, James Carter wrote:
> On Wed, Jan 7, 2026 at 12:33 PM Petr Lautrbach <lautrbach@redhat.com> wrote:
> >
> > Refactor spawn_command() to use execv() instead of system() to eliminate
> > shell injection vulnerabilities.
> >
> > Reported-By: Rahul Sandhu <nvraxn@gmail.com>
> > Signed-off-by: Petr Lautrbach <lautrbach@redhat.com>
>
> Acked-by: James Carter <jwcart2@gmail.com>

Signed-off-by: Jason Zaman <jason@perfinion.com>

Thanks, applied

>
> > ---
> > v2 - drop index initialization
> >
> > sandbox/seunshare.c | 132 ++++++++++++++++++++++++++------------------
> > 1 file changed, 79 insertions(+), 53 deletions(-)
> >
> > diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c
> > index 65da46463a8f..2512a18c7b52 100644
> > --- a/sandbox/seunshare.c
> > +++ b/sandbox/seunshare.c
> > @@ -56,6 +56,12 @@
> > #define USAGE_STRING _("USAGE: seunshare [ -v ] [ -C ] [ -k ] [ -t tmpdir ] [ -h homedir ] \
> > [ -r runuserdir ] [ -P pipewiresocket ] [ -W waylandsocket ] [ -Z CONTEXT ] -- executable [args] ")
> >
> > +#define strdup_or_err(args, index, src) do { \
> > + args[index] = strdup(src); \
> > + if (! args[index]) \
> > + goto err; \
> > + } while(0)
> > +
> > static int verbose = 0;
> > static int child = 0;
> >
> > @@ -136,15 +142,14 @@ static int set_signal_handles(void)
> > } while(0)
> >
> > /**
> > - * Spawn external command using system() with dropped privileges.
> > - * TODO: avoid system() and use exec*() instead
> > + * Spawn external command with dropped privileges.
> > */
> > -static int spawn_command(const char *cmd, uid_t uid){
> > +static int spawn_command(char **cmd, uid_t uid){
> > int childpid;
> > int status = -1;
> >
> > if (verbose > 1)
> > - printf("spawn_command: %s\n", cmd);
> > + printf("spawn_command: %s\n", cmd[0]);
> >
> > childpid = fork();
> > if (childpid == -1) {
> > @@ -155,8 +160,7 @@ static int spawn_command(const char *cmd, uid_t uid){
> > if (childpid == 0) {
> > if (drop_privs(uid) != 0) exit(-1);
> >
> > - status = system(cmd);
> > - status_to_retval(status, status);
> > + status = execv(cmd[0], cmd);
> > exit(status);
> > }
> >
> > @@ -342,15 +346,24 @@ static int bad_path(const char *path) {
> > return 0;
> > }
> >
> > -static int rsynccmd(const char * src, const char *dst, char **cmdbuf)
> > -{
> > +static void free_args(char **args) {
> > + char **args_p = args;
> > + if (! args)
> > + return;
> > + while (*args_p != NULL) {
> > + free(*args_p);
> > + args_p++;
> > + }
> > + free(args);
> > +}
> > +
> > +static int rsynccmd(const char * src, const char *dst, char ***cmd) {
> > + char **args;
> > char *buf = NULL;
> > - char *newbuf = NULL;
> > glob_t fglob;
> > fglob.gl_offs = 0;
> > int flags = GLOB_PERIOD;
> > - unsigned int i = 0;
> > - int rc = -1;
> > + unsigned int i = 0, index;
> >
> > /* match glob for all files in src dir */
> > if (asprintf(&buf, "%s/*", src) == -1) {
> > @@ -365,43 +378,35 @@ static int rsynccmd(const char * src, const char *dst, char **cmdbuf)
> >
> > free(buf); buf = NULL;
> >
> > - for ( i=0; i < fglob.gl_pathc; i++) {
> > - const char *path = fglob.gl_pathv[i];
> > -
> > - if (bad_path(path)) continue;
> > -
> > - if (!buf) {
> > - if (asprintf(&newbuf, "\'%s\'", path) == -1) {
> > - fprintf(stderr, _("Out of memory\n"));
> > - goto err;
> > - }
> > - } else {
> > - if (asprintf(&newbuf, "%s \'%s\'", buf, path) == -1) {
> > - fprintf(stderr, _("Out of memory\n"));
> > - goto err;
> > - }
> > - }
> > -
> > - free(buf); buf = newbuf;
> > - newbuf = NULL;
> > + /* rsync -trlHDq + <glob list> + dst + NULL */
> > + *cmd = calloc(2 + fglob.gl_pathc + 2, sizeof(char *));
> > + if (! *cmd) {
> > + fprintf(stderr, _("Out of memory\n"));
> > + return -1;
> > }
> >
> > - if (buf) {
> > - if (asprintf(&newbuf, "/usr/bin/rsync -trlHDq %s '%s'", buf, dst) == -1) {
> > - fprintf(stderr, _("Out of memory\n"));
> > - goto err;
> > - }
> > - *cmdbuf=newbuf;
> > - }
> > - else {
> > - *cmdbuf=NULL;
> > - }
> > - rc = 0;
> > + args = *cmd;
> > + strdup_or_err(args, 0, "/usr/bin/rsync");
> > + strdup_or_err(args, 1, "-trlHDq");
> >
> > + for ( i=0, index = 2; i < fglob.gl_pathc; i++) {
> > + const char *path = fglob.gl_pathv[i];
> > + if (bad_path(path)) continue;
> > + strdup_or_err(args, index, path);
> > + index++;
> > + }
> > + strdup_or_err(args, index, dst);
> > + index++;
> > + args[index] = NULL;
> > + globfree(&fglob);
> > + return 0;
> > err:
> > - free(buf); buf = NULL;
> > globfree(&fglob);
> > - return rc;
> > + if (args) {
> > + free_args(args);
> > + *cmd = NULL;
> > + }
> > + return -1;
> > }
> >
> > /*
> > @@ -472,21 +477,38 @@ static bool rm_rf(int targetfd, const char *path) {
> > static int cleanup_tmpdir(const char *tmpdir, const char *src,
> > struct passwd *pwd, int copy_content)
> > {
> > - char *cmdbuf = NULL;
> > + char **args;
> > int rc = 0;
> >
> > /* rsync files back */
> > if (copy_content) {
> > - if (asprintf(&cmdbuf, "/usr/bin/rsync --exclude=.X11-unix -utrlHDq --delete '%s/' '%s/'", tmpdir, src) == -1) {
> > + args = calloc(7, sizeof(char *));
> > + if (! args) {
> > fprintf(stderr, _("Out of memory\n"));
> > - cmdbuf = NULL;
> > - rc++;
> > + return 1;
> > + }
> > +
> > + strdup_or_err(args, 0, "/usr/bin/rsync");
> > + strdup_or_err(args, 1, "--exclude=.X11-unix");
> > + strdup_or_err(args, 2, "-utrlHDq");
> > + strdup_or_err(args, 3, "--delete");
> > + if (asprintf(&args[4], "%s/", tmpdir) == -1) {
> > + fprintf(stderr, _("Out of memory\n"));
> > + free_args(args);
> > + return 1;
> > }
> > - if (cmdbuf && spawn_command(cmdbuf, pwd->pw_uid) != 0) {
> > + if (asprintf(&args[5], "%s/", src) == -1) {
> > + fprintf(stderr, _("Out of memory\n"));
> > + free_args(args);
> > + return 1;
> > + }
> > + args[6] = NULL;
> > +
> > + if (spawn_command(args, pwd->pw_uid) != 0) {
> > fprintf(stderr, _("Failed to copy files from the runtime temporary directory\n"));
> > rc++;
> > }
> > - free(cmdbuf); cmdbuf = NULL;
> > + free_args(args);
> > }
> >
> > if ((uid_t)setfsuid(0) != 0) {
> > @@ -506,6 +528,10 @@ static int cleanup_tmpdir(const char *tmpdir, const char *src,
> > }
> >
> > return rc;
> > +err:
> > + if (args)
> > + free_args(args);
> > + return 1;
> > }
> >
> > /**
> > @@ -518,7 +544,7 @@ static char *create_tmpdir(const char *src, struct stat *src_st,
> > struct stat *out_st, struct passwd *pwd, const char *execcon)
> > {
> > char *tmpdir = NULL;
> > - char *cmdbuf = NULL;
> > + char **cmd = NULL;
> > int fd_t = -1, fd_s = -1;
> > struct stat tmp_st;
> > char *con = NULL;
> > @@ -605,7 +631,7 @@ static char *create_tmpdir(const char *src, struct stat *src_st,
> > if ((uid_t)setfsuid(pwd->pw_uid) != 0)
> > goto err;
> >
> > - if (rsynccmd(src, tmpdir, &cmdbuf) < 0) {
> > + if (rsynccmd(src, tmpdir, &cmd) < 0) {
> > goto err;
> > }
> >
> > @@ -613,7 +639,7 @@ static char *create_tmpdir(const char *src, struct stat *src_st,
> > if ((uid_t)setfsuid(0) != pwd->pw_uid)
> > goto err;
> >
> > - if (cmdbuf && spawn_command(cmdbuf, pwd->pw_uid) != 0) {
> > + if (spawn_command(cmd, pwd->pw_uid) != 0) {
> > fprintf(stderr, _("Failed to populate runtime temporary directory\n"));
> > cleanup_tmpdir(tmpdir, src, pwd, 0);
> > goto err;
> > @@ -623,7 +649,7 @@ static char *create_tmpdir(const char *src, struct stat *src_st,
> > err:
> > free(tmpdir); tmpdir = NULL;
> > good:
> > - free(cmdbuf); cmdbuf = NULL;
> > + free_args(cmd);
> > freecon(con); con = NULL;
> > if (fd_t >= 0) close(fd_t);
> > if (fd_s >= 0) close(fd_s);
> > --
> > 2.52.0
> >
> >
>