Merge pull request #3241 from kris7t/sbox-harden-exec

Harden sbox_run by using fexecve instead of execvp

2 files changed, 38 insertions, 27 deletions

diff --git a/src/firejail/dhcp.c b/src/firejail/dhcp.c
index 4a7806a7f..37547a985 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;
@@ -149,10 +143,6 @@ void dhcp_start(void) {
                         exit(1);
                 }
         }
-        if (s.st_uid != 0 && s.st_gid != 0) {
-                fprintf(stderr, "Error: invalid dhclient executable\n");
-                exit(1);
-        }
 
         EUID_ROOT();
         if (mkdir(RUN_DHCLIENT_DIR, 0700))
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);
         }