* [PATCH] sandbox/seunshare: Replace system() with execv() to prevent shell injection
@ 2026-01-07 11:48 Petr Lautrbach
2026-01-07 12:38 ` Rahul Sandhu
0 siblings, 1 reply; 5+ messages in thread
From: Petr Lautrbach @ 2026-01-07 11:48 UTC (permalink / raw)
To: selinux; +Cc: Petr Lautrbach, Rahul Sandhu
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>
---
sandbox/seunshare.c | 132 ++++++++++++++++++++++++++------------------
1 file changed, 79 insertions(+), 53 deletions(-)
diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c
index 65da46463a8f..74b87b7d13ec 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 = 0;
/* 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] sandbox/seunshare: Replace system() with execv() to prevent shell injection
2026-01-07 11:48 [PATCH] sandbox/seunshare: Replace system() with execv() to prevent shell injection Petr Lautrbach
@ 2026-01-07 12:38 ` Rahul Sandhu
2026-01-07 16:58 ` [PATCH v2] " Petr Lautrbach
0 siblings, 1 reply; 5+ messages in thread
From: Rahul Sandhu @ 2026-01-07 12:38 UTC (permalink / raw)
To: Petr Lautrbach, selinux; +Cc: Rahul Sandhu
On Wed Jan 7, 2026 at 11:48 AM GMT, Petr Lautrbach 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>
> ---
> sandbox/seunshare.c | 132 ++++++++++++++++++++++++++------------------
> 1 file changed, 79 insertions(+), 53 deletions(-)
>
> diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c
> index 65da46463a8f..74b87b7d13ec 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 = 0;
Why initalize this as 0?
>
> /* 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++) {
The next reference to it is here, where it's simply set as two, so it
would probably look nicer to get rid of the above decl and just change
it to:
for (int 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);
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2] sandbox/seunshare: Replace system() with execv() to prevent shell injection
2026-01-07 12:38 ` Rahul Sandhu
@ 2026-01-07 16:58 ` Petr Lautrbach
2026-01-23 15:03 ` James Carter
0 siblings, 1 reply; 5+ messages in thread
From: Petr Lautrbach @ 2026-01-07 16:58 UTC (permalink / raw)
To: selinux; +Cc: Petr Lautrbach, Rahul Sandhu
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>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] sandbox/seunshare: Replace system() with execv() to prevent shell injection
2026-01-07 16:58 ` [PATCH v2] " Petr Lautrbach
@ 2026-01-23 15:03 ` James Carter
2026-01-25 22:34 ` Jason Zaman
0 siblings, 1 reply; 5+ messages in thread
From: James Carter @ 2026-01-23 15:03 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: selinux, Rahul Sandhu
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>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] sandbox/seunshare: Replace system() with execv() to prevent shell injection
2026-01-23 15:03 ` James Carter
@ 2026-01-25 22:34 ` Jason Zaman
0 siblings, 0 replies; 5+ messages in thread
From: Jason Zaman @ 2026-01-25 22:34 UTC (permalink / raw)
To: James Carter; +Cc: Petr Lautrbach, selinux, Rahul Sandhu
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
> >
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-01-25 22:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-07 11:48 [PATCH] sandbox/seunshare: Replace system() with execv() to prevent shell injection Petr Lautrbach
2026-01-07 12:38 ` Rahul Sandhu
2026-01-07 16:58 ` [PATCH v2] " Petr Lautrbach
2026-01-23 15:03 ` James Carter
2026-01-25 22:34 ` Jason Zaman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox