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

["Rahul Sandhu" <nvraxn@gmail.com>]

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);