From 34193604fed04cad2b7b6b0f1a3a0428afd9ed5b Mon Sep 17 00:00:00 2001
From: Reiner Herrmann <reiner@reiner-h.de>
Date: Wed, 29 Jul 2020 20:22:52 +0200
Subject: [PATCH] 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>
---
 src/firejail/output.c | 80 +++++++++++++++++++++++++++++--------------
 1 file 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);