9 files changed, 181 insertions, 104 deletions

diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index a3d3dfcd4..03ad25f75 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -551,6 +551,7 @@ void disable_file_or_dir(const char *fname);
 void disable_file_path(const char *path, const char *file);
 int safe_fd(const char *path, int flags);
 int invalid_sandbox(const pid_t pid);
+int has_handler(pid_t pid, int signal);
 void enter_network_namespace(pid_t pid);
 
 // Get info regarding the last kernel mount operation from /proc/self/mountinfo
diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index 5edcdd58f..184875f58 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -30,11 +30,12 @@
 #include <fcntl.h>
 
 #define MAX_BUF 4096
+#define EMPTY_STRING ("")
 // check noblacklist statements not matched by a proper blacklist in disable-*.inc files
 //#define TEST_NO_BLACKLIST_MATCHING
 
 
-static int mount_warning = 0; // remember if warning was printed already
+static int mount_warning = 0;
 static void fs_rdwr(const char *dir);
 static void fs_rdwr_rec(const char *dir);
 
@@ -467,12 +468,11 @@ void fs_tmpfs(const char *dir, unsigned check_owner) {
         char *options;
         if (asprintf(&options, "mode=%o,uid=%u,gid=%u", s.st_mode & 07777, s.st_uid, s.st_gid) == -1)
                 errExit("asprintf");
-        // preserve some mount flags
+        // preserve mount flags, but remove read-only flag
         struct statvfs buf;
         if (fstatvfs(fd, &buf) == -1)
                 errExit("fstatvfs");
-        unsigned long flags = buf.f_flag &  // remove read-only flag
-                (MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_MANDLOCK|MS_STRICTATIME|MS_NODIRATIME|MS_RELATIME|MS_NOATIME);
+        unsigned long flags = buf.f_flag & ~(MS_RDONLY|MS_BIND);
         // mount via the symbolic link in /proc/self/fd
         char *proc;
         if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
@@ -1215,9 +1215,8 @@ void fs_overlayfs(void) {
 void fs_check_chroot_dir(const char *rootdir) {
         EUID_ASSERT();
         assert(rootdir);
+        char *dir = EMPTY_STRING;
         struct stat s;
-        int fd = -1;
-        int parentfd = -1;
 
         char *overlay;
         if (asprintf(&overlay, "%s/.firejail", cfg.homedir) == -1)
@@ -1229,7 +1228,7 @@ void fs_check_chroot_dir(const char *rootdir) {
         free(overlay);
 
         // fails if there is any symlink or if rootdir is not a directory
-        parentfd = safe_fd(rootdir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        int parentfd = safe_fd(rootdir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         if (parentfd == -1) {
                 fprintf(stderr, "Error: invalid chroot directory %s\n", rootdir);
                 exit(1);
@@ -1248,78 +1247,58 @@ void fs_check_chroot_dir(const char *rootdir) {
         }
 
         // check /dev
-        char *dir = "dev";
-        fd = openat(parentfd, dir, O_PATH|O_CLOEXEC);
-        if (fd == -1) {
-                if (errno == ENOENT)
-                        goto error1;
-                else
-                        goto error2;
-        }
+        dir = "dev";
+        int fd = openat(parentfd, dir, O_PATH|O_CLOEXEC);
+        if (fd == -1)
+                goto error1;
         if (fstat(fd, &s) == -1)
                 errExit("fstat");
         if (!S_ISDIR(s.st_mode) || s.st_uid != 0)
-                goto error3;
+                goto error2;
         close(fd);
 
         // check /var/tmp
         dir = "var/tmp";
         fd = openat(parentfd, dir, O_PATH|O_CLOEXEC);
-        if (fd == -1) {
-                if (errno == ENOENT)
-                        goto error1;
-                else
-                        goto error2;
-        }
+        if (fd == -1)
+                goto error1;
         if (fstat(fd, &s) == -1)
                 errExit("fstat");
         if (!S_ISDIR(s.st_mode) || s.st_uid != 0)
-                goto error3;
+                goto error2;
         close(fd);
 
         // check /proc
         dir = "proc";
         fd = openat(parentfd, dir, O_PATH|O_CLOEXEC);
-        if (fd == -1) {
-                if (errno == ENOENT)
-                        goto error1;
-                else
-                        goto error2;
-        }
+        if (fd == -1)
+                goto error1;
         if (fstat(fd, &s) == -1)
                 errExit("fstat");
         if (!S_ISDIR(s.st_mode) || s.st_uid != 0)
-                goto error3;
+                goto error2;
         close(fd);
 
         // check /tmp
         dir = "tmp";
         fd = openat(parentfd, dir, O_PATH|O_CLOEXEC);
-        if (fd == -1) {
-                if (errno == ENOENT)
-                        goto error1;
-                else
-                        goto error2;
-        }
+        if (fd == -1)
+                goto error1;
         if (fstat(fd, &s) == -1)
                 errExit("fstat");
         if (!S_ISDIR(s.st_mode) || s.st_uid != 0)
-                goto error3;
+                goto error2;
         close(fd);
 
         // check /etc
         dir = "etc";
         fd = openat(parentfd, dir, O_PATH|O_CLOEXEC);
-        if (fd == -1) {
-                if (errno == ENOENT)
-                        goto error1;
-                else
-                        goto error2;
-        }
+        if (fd == -1)
+                goto error1;
         if (fstat(fd, &s) == -1)
                 errExit("fstat");
         if (!S_ISDIR(s.st_mode) || s.st_uid != 0)
-                goto error3;
+                goto error2;
         if (((S_IWGRP|S_IWOTH) & s.st_mode) != 0) {
                 fprintf(stderr, "Error: only root user should be given write permission on chroot /etc\n");
                 exit(1);
@@ -1358,16 +1337,12 @@ void fs_check_chroot_dir(const char *rootdir) {
         if (getenv("FIREJAIL_X11")) {
                 dir = "tmp/.X11-unix";
                 fd = openat(parentfd, dir, O_PATH|O_CLOEXEC);
-                if (fd == -1) {
-                        if (errno == ENOENT)
-                                goto error1;
-                        else
-                                goto error2;
-                }
+                if (fd == -1)
+                        goto error1;
                 if (fstat(fd, &s) == -1)
                         errExit("fstat");
                 if (!S_ISDIR(s.st_mode) || s.st_uid != 0)
-                        goto error3;
+                        goto error2;
                 close(fd);
         }
 
@@ -1375,13 +1350,14 @@ void fs_check_chroot_dir(const char *rootdir) {
         return;
 
 error1:
-        fprintf(stderr, "Error: cannot find /%s in chroot directory\n", dir);
+        if (errno == ENOENT)
+                fprintf(stderr, "Error: cannot find /%s in chroot directory\n", dir);
+        else {
+                perror("open");
+                fprintf(stderr, "Error: cannot open /%s in chroot directory\n", dir);
+        }
         exit(1);
 error2:
-        perror("open");
-        fprintf(stderr, "Error: cannot open /%s in chroot directory\n", dir);
-        exit(1);
-error3:
         fprintf(stderr, "Error: chroot /%s should be a directory owned by root\n", dir);
         exit(1);
 }
diff --git a/src/firejail/fs_whitelist.c b/src/firejail/fs_whitelist.c
index 829636b9c..74b9449be 100644
--- a/src/firejail/fs_whitelist.c
+++ b/src/firejail/fs_whitelist.c
@@ -119,7 +119,6 @@ static int mkpath(const char* path, mode_t mode) {
 static void whitelist_path(ProfileEntry *entry) {
         assert(entry);
         const char *path = entry->data + 10;
-        assert(path);
         const char *fname;
         char *wfile = NULL;
 
diff --git a/src/firejail/join.c b/src/firejail/join.c
index 4259644f7..60980fb2e 100644
--- a/src/firejail/join.c
+++ b/src/firejail/join.c
@@ -31,7 +31,6 @@
 
 static int apply_caps = 0;
 static uint64_t caps = 0;
-static int apply_seccomp = 0;
 static unsigned display = 0;
 #define BUFLEN 4096
 
@@ -41,7 +40,15 @@ static void signal_handler(int sig){
         exit(sig);
 }
 
+static void install_handler(void) {
+        struct sigaction sga;
 
+        // handle SIGTERM
+        sigemptyset(&sga.sa_mask);
+        sga.sa_handler = signal_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGTERM, &sga, NULL);
+}
 
 static void extract_x11_display(pid_t pid) {
         char *fname;
@@ -313,7 +320,7 @@ void join(pid_t pid, int argc, char **argv, int index) {
         EUID_ROOT();
         // in user mode set caps seccomp, cpu, cgroup, etc
         if (getuid() != 0) {
-                extract_nonewprivs(pid);  // redundant on Linux >= 4.10; duplicated in function extract_caps_seccomp
+                extract_nonewprivs(pid);  // redundant on Linux >= 4.10; duplicated in function extract_caps
                 extract_caps(pid);
                 extract_cpu(pid);
                 extract_cgroup(pid);
@@ -465,8 +472,19 @@ void join(pid_t pid, int argc, char **argv, int index) {
         }
 
         int status = 0;
+        //*****************************
+        // following code is signal-safe
+
+        install_handler();
+
         // wait for the child to finish
         waitpid(child, &status, 0);
+
+        // restore default signal action
+        signal(SIGTERM, SIG_DFL);
+
+        // end of signal-safe code
+        //*****************************
         flush_stdin();
 
         if (WIFEXITED(status)) {
diff --git a/src/firejail/main.c b/src/firejail/main.c
index faa4972e2..61f507f36 100644
--- a/src/firejail/main.c
+++ b/src/firejail/main.c
@@ -157,12 +157,35 @@ static void myexit(int rv) {
         exit(rv);
 }
 
-static void my_handler(int s){
-        EUID_ROOT();
+static void my_handler(int s) {
         fmessage("\nParent received signal %d, shutting down the child process...\n", s);
         logsignal(s);
-        kill(child, SIGTERM);
-        myexit(1);
+        if (waitpid(child, NULL, WNOHANG) == 0) {
+                if (has_handler(child, s)) // signals are not delivered if there is no handler yet
+                        kill(child, s);
+                else
+                        kill(child, SIGKILL);
+                waitpid(child, NULL, 0);
+        }
+        myexit(s);
+}
+
+static void install_handler(void) {
+        struct sigaction sga;
+
+        // block SIGTERM while handling SIGINT
+        sigemptyset(&sga.sa_mask);
+        sigaddset(&sga.sa_mask, SIGTERM);
+        sga.sa_handler = my_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGINT, &sga, NULL);
+
+        // block SIGINT while handling SIGTERM
+        sigemptyset(&sga.sa_mask);
+        sigaddset(&sga.sa_mask, SIGINT);
+        sga.sa_handler = my_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGTERM, &sga, NULL);
 }
 
 // return 1 if error, 0 if a valid pid was found
@@ -2601,16 +2624,25 @@ int main(int argc, char **argv) {
                 flock(lockfd_network, LOCK_UN);
                 close(lockfd_network);
         }
+        EUID_USER();
+
+        int status = 0;
+        //*****************************
+        // following code is signal-safe
 
         // handle CTRL-C in parent
-        signal (SIGINT, my_handler);
-        signal (SIGTERM, my_handler);
+        install_handler();
 
         // wait for the child to finish
-        EUID_USER();
-        int status = 0;
         waitpid(child, &status, 0);
 
+        // restore default signal actions
+        signal(SIGTERM, SIG_DFL);
+        signal(SIGINT, SIG_DFL);
+
+        // end of signal-safe code
+        //*****************************
+
         // free globals
         if (cfg.profile) {
                 ProfileEntry *prf = cfg.profile;
diff --git a/src/firejail/mountinfo.c b/src/firejail/mountinfo.c
index c89845ace..02c28cc5e 100644
--- a/src/firejail/mountinfo.c
+++ b/src/firejail/mountinfo.c
@@ -199,8 +199,7 @@ int get_mount_id(const char *path) {
         return -2;
 }
 
-// Check /proc/self/mountinfo if path has any submounts (or if path would have submounts
-// if it was made a mount point).
+// Check /proc/self/mountinfo if path contains any mounts points.
 // Returns an array that can be iterated over for recursive remounting.
 char **build_mount_array(const int mount_id, const char *path) {
         assert(path);
diff --git a/src/firejail/sandbox.c b/src/firejail/sandbox.c
index 475779f47..735bab684 100644
--- a/src/firejail/sandbox.c
+++ b/src/firejail/sandbox.c
@@ -53,6 +53,7 @@ static int force_nonewprivs = 0;
 
 static int monitored_pid = 0;
 static void sandbox_handler(int sig){
+        usleep(10000); // don't race to print a message
         fmessage("\nChild received signal %d, shutting down the sandbox...\n", sig);
 
         // broadcast sigterm to all processes in the group
@@ -81,10 +82,8 @@ static void sandbox_handler(int sig){
                         monsec--;
                 }
                 free(monfile);
-
         }
 
-
         // broadcast a SIGKILL
         kill(-1, SIGKILL);
         flush_stdin();
@@ -92,6 +91,23 @@ static void sandbox_handler(int sig){
         exit(sig);
 }
 
+static void install_handler(void) {
+        struct sigaction sga;
+
+        // block SIGTERM while handling SIGINT
+        sigemptyset(&sga.sa_mask);
+        sigaddset(&sga.sa_mask, SIGTERM);
+        sga.sa_handler = sandbox_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGINT, &sga, NULL);
+
+        // block SIGINT while handling SIGTERM
+        sigemptyset(&sga.sa_mask);
+        sigaddset(&sga.sa_mask, SIGINT);
+        sga.sa_handler = sandbox_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGTERM, &sga, NULL);
+}
 
 static void set_caps(void) {
         if (arg_caps_drop_all)
@@ -123,7 +139,6 @@ static void save_nogroups(void) {
                 fprintf(stderr, "Error: cannot save nogroups state\n");
                 exit(1);
         }
-
 }
 
 static void save_nonewprivs(void) {
@@ -235,9 +250,17 @@ static void chk_chroot(void) {
 }
 
 static int monitor_application(pid_t app_pid) {
+        EUID_ASSERT();
         monitored_pid = app_pid;
-        signal (SIGTERM, sandbox_handler);
-        EUID_USER();
+
+        // block signals and install handler
+        sigset_t oldmask, newmask;
+        sigemptyset(&oldmask);
+        sigemptyset(&newmask);
+        sigaddset(&newmask, SIGTERM);
+        sigaddset(&newmask, SIGINT);
+        sigprocmask(SIG_BLOCK, &newmask, &oldmask);
+        install_handler();
 
         // handle --timeout
         int options = 0;;
@@ -260,16 +283,25 @@ static int monitor_application(pid_t app_pid) {
 
                 pid_t rv;
                 do {
+                        // handle signals asynchronously
+                        sigprocmask(SIG_SETMASK, &oldmask, NULL);
+
                         rv = waitpid(-1, &status, options);
-                        if (rv == -1)
+
+                        // block signals again
+                        sigprocmask(SIG_BLOCK, &newmask, NULL);
+
+                        if (rv == -1) { // we can get here if we have processes joining the sandbox (ECHILD)
+                                sleep(1);
                                 break;
+                        }
 
                         // handle --timeout
                         if (options) {
                                 if (--timeout == 0)  {
                                         kill(-1, SIGTERM);
-                                        flush_stdin();
                                         sleep(1);
+                                        flush_stdin();
                                         _exit(1);
                                 }
                                 else
@@ -279,11 +311,6 @@ static int monitor_application(pid_t app_pid) {
                 while(rv != monitored_pid);
                 if (arg_debug)
                         printf("Sandbox monitor: waitpid %d retval %d status %d\n", monitored_pid, rv, status);
-                if (rv == -1) { // we can get here if we have processes joining the sandbox (ECHILD)
-                        if (arg_debug)
-                                perror("waitpid");
-                        sleep(1);
-                }
 
                 DIR *dir;
                 if (!(dir = opendir("/proc"))) {
@@ -1151,7 +1178,7 @@ int sandbox(void* sandbox_arg) {
         // drop privileges, fork the application and monitor it
         //****************************************
         drop_privs(arg_nogroups);
-        prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0); // kill the child in case the parent died
+        prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0); // kill the sandbox in case the parent died
         pid_t app_pid = fork();
         if (app_pid == -1)
                 errExit("fork");
diff --git a/src/firejail/shutdown.c b/src/firejail/shutdown.c
index be20cd353..743a256a4 100644
--- a/src/firejail/shutdown.c
+++ b/src/firejail/shutdown.c
@@ -26,20 +26,11 @@
 void shut(pid_t pid) {
         EUID_ASSERT();
 
-        pid_t parent = pid;
-        // if the pid is that of a firejail  process, use the pid of a child process inside the sandbox
         EUID_ROOT();
         char *comm = pid_proc_comm(pid);
         EUID_USER();
         if (comm) {
-                if (strcmp(comm, "firejail") == 0) {
-                        pid_t child;
-                        if (find_child(pid, &child) == 0) {
-                                pid = child;
-                                printf("Switching to pid %u, the first child process inside the sandbox\n", (unsigned) pid);
-                        }
-                }
-                else {
+                if (strcmp(comm, "firejail") != 0) {
                         fprintf(stderr, "Error: this is not a firejail sandbox\n");
                         exit(1);
                 }
@@ -58,7 +49,6 @@ void shut(pid_t pid) {
                 }
         }
 
-        EUID_ROOT();
         printf("Sending SIGTERM to %u\n", pid);
         kill(pid, SIGTERM);
 
@@ -94,14 +84,16 @@ void shut(pid_t pid) {
 
         // force SIGKILL
         if (!killdone) {
-                // kill the process and also the parent
+                // kill the process and its child
+                pid_t child;
+                if (find_child(pid, &child) == 0) {
+                        printf("Sending SIGKILL to %u\n", child);
+                        kill(child, SIGKILL);
+                }
                 printf("Sending SIGKILL to %u\n", pid);
                 kill(pid, SIGKILL);
-                if (parent != pid) {
-                        printf("Sending SIGKILL to %u\n", parent);
-                        kill(parent, SIGKILL);
-                }
         }
 
-        delete_run_files(parent);
+        EUID_ROOT();
+        delete_run_files(pid);
 }
diff --git a/src/firejail/util.c b/src/firejail/util.c
index 8c474f966..b1fba4226 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -454,7 +454,6 @@ void trim_trailing_slash_or_dot(char *path) {
         assert(path);
 
         char *end = strchr(path, '\0');
-        assert(end);
         if ((end - path) > 1) {
                 end--;
                 while (*end == '/' ||
@@ -941,9 +940,7 @@ int remove_overlay_directory(void) {
                 // wait for the child to finish
                 waitpid(child, NULL, 0);
                 // check if ~/.firejail was deleted
-                if (stat(path, &s) == -1)
-                        return 0;
-                else
+                if (stat(path, &s) == 0)
                         return 1;
         }
         return 0;
@@ -1093,8 +1090,13 @@ unsigned extract_timeout(const char *str) {
                 fprintf(stderr, "Error: invalid timeout, please use a hh:mm:ss format\n");
                 exit(1);
         }
+        unsigned timeout = h * 3600 + m * 60 + s;
+        if (timeout == 0) {
+                fprintf(stderr, "Error: invalid timeout\n");
+                exit(1);
+        }
 
-        return h * 3600 + m * 60 + s;
+        return timeout;
 }
 
 void disable_file_or_dir(const char *fname) {
@@ -1264,6 +1266,37 @@ int invalid_sandbox(const pid_t pid) {
         return 0;
 }
 
+int has_handler(pid_t pid, int signal) {
+        if (signal > 0) {
+                char *fname;
+                if (asprintf(&fname, "/proc/%d/status", pid) == -1)
+                        errExit("asprintf");
+                EUID_ROOT();
+                FILE *fp = fopen(fname, "re");
+                EUID_USER();
+                free(fname);
+                if (fp) {
+                        char buf[BUFLEN];
+                        while (fgets(buf, BUFLEN, fp)) {
+                                if (strncmp(buf, "SigCgt:", 7) == 0) {
+                                        char *ptr = buf + 7;
+                                        unsigned long long val;
+                                        if (sscanf(ptr, "%llx", &val) != 1) {
+                                                fprintf(stderr, "Error: cannot read /proc file\n");
+                                                exit(1);
+                                        }
+                                        val >>= (signal - 1);
+                                        val &= 1;
+                                        fclose(fp);
+                                        return val;  // 1 if process has a handler for the signal, else 0
+                                }
+                        }
+                        fclose(fp);
+                }
+        }
+        return 0;
+}
+
 void enter_network_namespace(pid_t pid) {
         // in case the pid is that of a firejail process, use the pid of the first child process
         pid_t child = switch_to_child(pid);