firejail: don't pass command line through shell when redirecting output

When redirecting output via --output or --output-stderr, firejail was
concatenating all command line arguments into a single string
that was passed to a shell. As the arguments were no longer escaped,
the shell was able to interpret them.
Someone who has control over the command line arguments of the
sandboxed application could use this to run arbitrary other commands.

Instead of passing it through a shell for piping the output to ftee,
the pipeline is now manually created and the processes are executed
directly.

Fixes: CVE-2020-17368

Reported-by: Tim Starling <tstarling@wikimedia.org>

1 files changed, 54 insertions, 26 deletions

diff --git a/src/firejail/output.c b/src/firejail/output.c
index 6e678afd3..0e961bb61 100644
--- a/src/firejail/output.c
+++ b/src/firejail/output.c
@@ -77,38 +77,66 @@ void check_output(int argc, char **argv) {
                 }
         }
 
-        // build the new command line
-        int len = 0;
-        for (i = 0; i < argc; i++) {
-                len += strlen(argv[i]) + 1; // + ' '
+        int pipefd[2];
+        if (pipe(pipefd) == -1) {
+                errExit("pipe");
         }
-        len += 100 + strlen(LIBDIR) + strlen(outfile); // tee command
 
-        char *cmd = malloc(len + 1); // + '\0'
-        if (!cmd)
-                errExit("malloc");
+        pid_t pid = fork();
+        if (pid == -1) {
+                errExit("fork");
+        } else if (pid == 0) {
+                /* child */
+                if (dup2(pipefd[0], STDIN_FILENO) == -1) {
+                        errExit("dup2");
+                }
+                close(pipefd[1]);
+                if (pipefd[0] != STDIN_FILENO) {
+                        close(pipefd[0]);
+                }
 
-        char *ptr = cmd;
-        for (i = 0; i < argc; i++) {
-                if (strncmp(argv[i], "--output=", 9) == 0)
-                        continue;
-                if (strncmp(argv[i], "--output-stderr=", 16) == 0)
-                        continue;
-                ptr += sprintf(ptr, "%s ", argv[i]);
+                char *args[3];
+                args[0] = LIBDIR "/firejail/ftee";
+                args[1] = outfile;
+                args[2] = NULL;
+                execv(args[0], args);
+                perror("execvp");
+                exit(1);
         }
 
-        if (enable_stderr)
-                sprintf(ptr, "2>&1 | %s/firejail/ftee %s", LIBDIR, outfile);
-        else
-                sprintf(ptr, " | %s/firejail/ftee %s", LIBDIR, outfile);
+        /* parent */
+        if (dup2(pipefd[1], STDOUT_FILENO) == -1) {
+                errExit("dup2");
+        }
+        if (enable_stderr && dup2(STDOUT_FILENO, STDERR_FILENO) == -1) {
+                errExit("dup2");
+        }
+        close(pipefd[0]);
+        if (pipefd[1] != STDOUT_FILENO) {
+                close(pipefd[1]);
+        }
 
-        // run command
-        char *a[4];
-        a[0] = "/bin/bash";
-        a[1] = "-c";
-        a[2] = cmd;
-        a[3] = NULL;
-        execvp(a[0], a);
+        char **args = calloc(argc + 1, sizeof(char *));
+        if (!args) {
+                errExit("calloc");
+        }
+        bool found_separator = false;
+        /* copy argv into args, but drop --output(-stderr) arguments */
+        for (int i = 0, j = 0; i < argc; i++) {
+                if (!found_separator && i > 0) {
+                        if (strncmp(argv[i], "--output=", 9) == 0) {
+                                continue;
+                        }
+                        if (strncmp(argv[i], "--output-stderr=", 16) == 0) {
+                                continue;
+                        }
+                        if (strncmp(argv[i], "--", 2) != 0 || strcmp(argv[i], "--") == 0) {
+                                found_separator = true;
+                        }
+                }
+                args[j++] = argv[i];
+        }
+        execvp(args[0], args);
 
         perror("execvp");
         exit(1);