Harden sbox_run by using fexecve instead of execvp

We require the command passed to sbox_run to be an absolute path,
and avoid resolving PATH.
Note that PATH-based attacks were already difficult to pull of,
because sbox_run clears the environment before executing the command.
This patch hopefully makes then impossible.

As an additional precaution, we check that the executable is owned by either
the root user or the root group, and is not world-writable.

The use of O_PATH, fstat and fexecve aims to prevent a race condition when
the invoked path (e.g., /usr/lib/firejail/fnet) is owned by root
or is a symlink to a binary owned by root, but the containing directory
(e.g., /usr/lib/firejail) is somehow owned by a user.
This is quite unlikely (but may be possible by abusing some other setuid
executable is a specific way), and would allow swapping the binary or symlink
to a malicious one after we checked ownership.
"Locking in" the file descriptor gets rid of the race condition.

We have to get rid of the `/proc/[pid]/comm` check in dhcp_read_pidfile,
because fexecve sets the comm value to the fd being exec'd (e.g., 3)
instead of the name of the file.
This is not a problem, unless by the time we pick up the pidfile of dhclient,
it has already crashed, and the pid number have wrapper around.
Needless to say, this is extremely unlikely
(and does not cause a security issue, anyways).

2 files changed, 38 insertions, 23 deletions

diff --git a/src/firejail/dhcp.c b/src/firejail/dhcp.c
index 4a7806a7f..bc5d68a5e 100644
--- a/src/firejail/dhcp.c
+++ b/src/firejail/dhcp.c
@@ -88,9 +88,6 @@ static void dhcp_run_dhclient(char *dhclient_path, const Dhclient *client) {
 static pid_t dhcp_read_pidfile(const Dhclient *client) {
         // We have to run dhclient as a forking daemon (not pass the -d option),
         // because we want to be notified of a successful DHCP lease by the parent process exit.
-        // However, try to be extra paranoid with race conditions,
-        // because dhclient only writes the daemon pid into the pidfile
-        // after its parent process has exited.
         int tries = 0;
         pid_t found = 0;
         while (found == 0 && tries < 10) {
@@ -99,11 +96,8 @@ static pid_t dhcp_read_pidfile(const Dhclient *client) {
                 FILE *pidfile = fopen(client->pid_file, "r");
                 if (pidfile) {
                         long pid;
-                        if (fscanf(pidfile, "%ld", &pid) == 1) {
-                                char *pidname = pid_proc_comm((pid_t) pid);
-                                if (pidname && strcmp(pidname, "dhclient") == 0)
-                                        found = (pid_t) pid;
-                        }
+                        if (fscanf(pidfile, "%ld", &pid) == 1)
+                                found = (pid_t) pid;
                         fclose(pidfile);
                 }
                 ++tries;
diff --git a/src/firejail/sbox.c b/src/firejail/sbox.c
index 77ca07a81..c3b68f3a8 100644
--- a/src/firejail/sbox.c
+++ b/src/firejail/sbox.c
@@ -26,6 +26,11 @@
  #include <sys/wait.h>
 #include "../include/seccomp.h"
 
+#include <fcntl.h>
+#ifndef O_PATH
+#define O_PATH 010000000
+#endif
+
 static struct sock_filter filter[] = {
         VALIDATE_ARCHITECTURE,
         EXAMINE_SYSCALL,
@@ -140,24 +145,19 @@ int sbox_run_v(unsigned filtermask, char * const arg[]) {
         if (child < 0)
                 errExit("fork");
         if (child == 0) {
+                int env_index = 0;
+                char *new_environment[256] = { NULL };
                 // preserve firejail-specific env vars
                 char *cl = getenv("FIREJAIL_FILE_COPY_LIMIT");
                 if (cl) {
-                        // duplicate the value, who knows what's going to happen with it in clearenv!
-                        cl = strdup(cl);
-                        if (!cl)
-                                errExit("strdup");
+                        if (asprintf(&new_environment[env_index++], "FIREJAIL_FILE_COPY_LIMIT=%s", cl) == -1)
+                                errExit("asprintf");
                 }
                 clearenv();
-                if (cl) {
-                        if (setenv("FIREJAIL_FILE_COPY_LIMIT", cl, 1) == -1)
-                                errExit("setenv");
-                        free(cl);
-                }
                 if (arg_quiet) // --quiet is passed as an environment variable
-                        setenv("FIREJAIL_QUIET", "yes", 1);
+                        new_environment[env_index++] = "FIREJAIL_QUIET=yes";
                 if (arg_debug) // --debug is passed as an environment variable
-                        setenv("FIREJAIL_DEBUG", "yes", 1);
+                        new_environment[env_index++] = "FIREJAIL_DEBUG=yes";
 
                 if (filtermask & SBOX_STDIN_FROM_FILE) {
                         int fd;
@@ -237,11 +237,32 @@ int sbox_run_v(unsigned filtermask, char * const arg[]) {
                 else if (filtermask & SBOX_USER)
                         drop_privs(1);
 
-                if (arg[0])     // get rid of scan-build warning
-                        execvp(arg[0], arg);
-                else
+                if (arg[0]) { // get rid of scan-build warning
+                        int fd = open(arg[0], O_PATH | O_CLOEXEC);
+                        if (fd == -1) {
+                                if (errno == ENOENT) {
+                                        fprintf(stderr, "Error: %s does not exist\n", arg[0]);
+                                        exit(1);
+                                } else {
+                                        errExit("open");
+                                }
+                        }
+                        struct stat s;
+                        if (fstat(fd, &s) == -1)
+                                errExit("fstat");
+                        if (s.st_uid != 0 && s.st_gid != 0) {
+                                fprintf(stderr, "Error: %s is not owned by root, refusing to execute\n", arg[0]);
+                                exit(1);
+                        }
+                        if (s.st_mode & 00002) {
+                                fprintf(stderr, "Error: %s is world writable, refusing to execute\n", arg[0]);
+                                exit(1);
+                        }
+                        fexecve(fd, arg, new_environment);
+                } else {
                         assert(0);
-                perror("execvp");
+                }
+                perror("fexecve");
                 _exit(1);
         }