1 files changed, 78 insertions, 85 deletions

diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index 09de11de9..eaf23e603 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -54,16 +54,9 @@ static char *opstr[] = {
         [MOUNT_RDWR_NOCHECK] = "read-write",
 };
 
-typedef enum {
-        UNSUCCESSFUL,
-        SUCCESSFUL
-} LAST_DISABLE_OPERATION;
-LAST_DISABLE_OPERATION last_disable = UNSUCCESSFUL;
-
 static void disable_file(OPERATION op, const char *filename) {
         assert(filename);
         assert(op <OPERATION_MAX);
-        last_disable = UNSUCCESSFUL;
 
         // Resolve all symlinks
         char* fname = realpath(filename, NULL);
@@ -71,20 +64,22 @@ static void disable_file(OPERATION op, const char *filename) {
                 return;
         }
         if (fname == NULL && errno == EACCES) {
-                if (arg_debug)
-                        printf("Debug: no access to file %s, forcing mount\n", filename);
                 // realpath and stat functions will fail on FUSE filesystems
                 // they don't seem to like a uid of 0
                 // force mounting
-                int rv = mount(RUN_RO_DIR, filename, "none", MS_BIND, "mode=400,gid=0");
-                if (rv == 0)
-                        last_disable = SUCCESSFUL;
-                else {
-                        rv = mount(RUN_RO_FILE, filename, "none", MS_BIND, "mode=400,gid=0");
-                        if (rv == 0)
-                                last_disable = SUCCESSFUL;
+                int fd = open(filename, O_PATH|O_CLOEXEC);
+                if (fd < 0) {
+                        if (arg_debug)
+                                printf("Warning (blacklisting): cannot open %s: %s\n", filename, strerror(errno));
+                        return;
                 }
-                if (last_disable == SUCCESSFUL) {
+
+                int err = bind_mount_path_to_fd(RUN_RO_DIR, fd);
+                if (err < 0)
+                        err = bind_mount_path_to_fd(RUN_RO_FILE, fd);
+                close(fd);
+
+                if (err == 0) {
                         if (arg_debug)
                                 printf("Disable %s\n", filename);
                         if (op == BLACKLIST_FILE)
@@ -92,21 +87,18 @@ static void disable_file(OPERATION op, const char *filename) {
                         else
                                 fs_logger2("blacklist-nolog", filename);
                 }
-                else {
-                        if (arg_debug)
-                                printf("Warning (blacklisting): %s is an invalid file, skipping...\n", filename);
-                }
+                else if (arg_debug)
+                        printf("Warning (blacklisting): cannot mount on %s\n", filename);
 
                 return;
         }
 
         // if the file is not present, do nothing
+        assert(fname);
         struct stat s;
-        if (fname == NULL)
-                return;
-        if (stat(fname, &s) == -1) {
+        if (stat(fname, &s) < 0) {
                 if (arg_debug)
-                        fwarning("%s does not exist, skipping...\n", fname);
+                        printf("Warning (blacklisting): cannot access %s: %s\n", fname, strerror(errno));
                 free(fname);
                 return;
         }
@@ -115,8 +107,10 @@ static void disable_file(OPERATION op, const char *filename) {
         // we migth have a file found in ${PATH} pointing to /usr/bin/firejail
         // blacklisting it here will end up breaking situations like user clicks on a link in Thunderbird
         //     and expects Firefox to open in the same sandbox
-        if (strcmp(BINDIR "/firejail", fname) == 0)
+        if (strcmp(BINDIR "/firejail", fname) == 0) {
+                free(fname);
                 return;
+        }
 
         // modify the file
         if (op == BLACKLIST_FILE || op == BLACKLIST_NOLOG) {
@@ -141,15 +135,23 @@ static void disable_file(OPERATION op, const char *filename) {
                                         printf(" - no logging\n");
                         }
 
+                        int fd = open(fname, O_PATH|O_CLOEXEC);
+                        if (fd < 0) {
+                                if (arg_debug)
+                                        printf("Warning (blacklisting): cannot open %s: %s\n", fname, strerror(errno));
+                                free(fname);
+                                return;
+                        }
                         if (S_ISDIR(s.st_mode)) {
-                                if (mount(RUN_RO_DIR, fname, "none", MS_BIND, "mode=400,gid=0") < 0)
+                                if (bind_mount_path_to_fd(RUN_RO_DIR, fd) < 0)
                                         errExit("disable file");
                         }
                         else {
-                                if (mount(RUN_RO_FILE, fname, "none", MS_BIND, "mode=400,gid=0") < 0)
+                                if (bind_mount_path_to_fd(RUN_RO_FILE, fd) < 0)
                                         errExit("disable file");
                         }
-                        last_disable = SUCCESSFUL;
+                        close(fd);
+
                         if (op == BLACKLIST_FILE)
                                 fs_logger2("blacklist", fname);
                         else
@@ -158,7 +160,6 @@ static void disable_file(OPERATION op, const char *filename) {
         }
         else if (op == MOUNT_READONLY || op == MOUNT_RDWR || op == MOUNT_NOEXEC) {
                 fs_remount_rec(fname, op);
-                // todo: last_disable = SUCCESSFUL;
         }
         else if (op == MOUNT_TMPFS) {
                 if (S_ISDIR(s.st_mode)) {
@@ -171,7 +172,6 @@ static void disable_file(OPERATION op, const char *filename) {
                         }
                         fs_tmpfs(fname, getuid());
                         selinux_relabel_path(fname, fname);
-                        last_disable = SUCCESSFUL;
                 }
                 else
                         fwarning("%s is not a directory; cannot mount a tmpfs on top of it.\n", fname);
@@ -493,35 +493,38 @@ static void fs_remount_simple(const char *path, OPERATION op) {
         assert(path);
 
         // open path without following symbolic links
-        int fd1 = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
-        if (fd1 == -1)
+        int fd = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        if (fd < 0)
                 goto out;
-        struct stat s1;
-        if (fstat(fd1, &s1) == -1) {
+
+        struct stat s;
+        if (fstat(fd, &s) < 0) {
                 // fstat can fail with EACCES if path is a FUSE mount,
                 // mounted without 'allow_root' or 'allow_other'
                 if (errno != EACCES)
                         errExit("fstat");
-                close(fd1);
+                close(fd);
                 goto out;
         }
         // get mount flags
         struct statvfs buf;
-        if (fstatvfs(fd1, &buf) == -1)
-                errExit("fstatvfs");
+        if (fstatvfs(fd, &buf) < 0) {
+                close(fd);
+                goto out;
+        }
         unsigned long flags = buf.f_flag;
 
         // read-write option
         if (op == MOUNT_RDWR || op == MOUNT_RDWR_NOCHECK) {
                 // nothing to do if there is no read-only flag
                 if ((flags & MS_RDONLY) == 0) {
-                        close(fd1);
+                        close(fd);
                         return;
                 }
                 // allow only user owned directories, except the user is root
-                if (op != MOUNT_RDWR_NOCHECK && getuid() != 0 && s1.st_uid != getuid()) {
+                if (op != MOUNT_RDWR_NOCHECK && getuid() != 0 && s.st_uid != getuid()) {
                         fwarning("you are not allowed to change %s to read-write\n", path);
-                        close(fd1);
+                        close(fd);
                         return;
                 }
                 flags &= ~MS_RDONLY;
@@ -530,7 +533,7 @@ static void fs_remount_simple(const char *path, OPERATION op) {
         else if (op == MOUNT_NOEXEC) {
                 // nothing to do if path is mounted noexec already
                 if ((flags & (MS_NOEXEC|MS_NODEV|MS_NOSUID)) == (MS_NOEXEC|MS_NODEV|MS_NOSUID)) {
-                        close(fd1);
+                        close(fd);
                         return;
                 }
                 flags |= MS_NOEXEC|MS_NODEV|MS_NOSUID;
@@ -539,7 +542,7 @@ static void fs_remount_simple(const char *path, OPERATION op) {
         else if (op == MOUNT_READONLY) {
                 // nothing to do if path is mounted read-only already
                 if ((flags & MS_RDONLY) == MS_RDONLY) {
-                        close(fd1);
+                        close(fd);
                         return;
                 }
                 flags |= MS_RDONLY;
@@ -549,29 +552,33 @@ static void fs_remount_simple(const char *path, OPERATION op) {
 
         if (arg_debug)
                 printf("Mounting %s %s\n", opstr[op], path);
+
+        // make path a mount point:
         // mount --bind path path
-        char *proc;
-        if (asprintf(&proc, "/proc/self/fd/%d", fd1) == -1)
-                errExit("asprintf");
-        if (mount(proc, proc, NULL, MS_BIND|MS_REC, NULL) < 0)
-                errExit("mount");
-        free(proc);
+        int err = bind_mount_by_fd(fd, fd);
+        if (err) {
+                close(fd);
+                goto out;
+        }
 
-        // mount --bind -o remount,ro path
-        // need to open path again without following symbolic links
+        // remount the mount point
+        // need to open path again
         int fd2 = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
-        if (fd2 == -1)
-                errExit("open");
+        close(fd); // earliest timepoint to close fd
+        if (fd2 < 0)
+                goto out;
+
+        // device and inode number should be the same
         struct stat s2;
-        if (fstat(fd2, &s2) == -1)
+        if (fstat(fd2, &s2) < 0)
                 errExit("fstat");
-        // device and inode number should be the same
-        if (s1.st_dev != s2.st_dev || s1.st_ino != s2.st_ino)
+        if (s.st_dev != s2.st_dev || s.st_ino != s2.st_ino)
                 errLogExit("invalid %s mount", opstr[op]);
-        if (asprintf(&proc, "/proc/self/fd/%d", fd2) == -1)
-                errExit("asprintf");
-        if (mount(NULL, proc, NULL, flags|MS_BIND|MS_REMOUNT, NULL) < 0)
-                errExit("mount");
+
+        err = remount_by_fd(fd2, flags);
+        close(fd2);
+        if (err)
+                goto out;
 
         // run a sanity check on /proc/self/mountinfo and confirm that target of the last
         // mount operation was path; if there are other mount points contained inside path,
@@ -582,10 +589,8 @@ static void fs_remount_simple(const char *path, OPERATION op) {
            (*(mptr->dir + len) != '\0' && *(mptr->dir + len) != '/'))
            && strcmp(path, "/") != 0) // support read-only=/
                 errLogExit("invalid %s mount", opstr[op]);
+
         fs_logger2(opstr[op], path);
-        free(proc);
-        close(fd1);
-        close(fd2);
         return;
 
 out:
@@ -743,8 +748,6 @@ void fs_proc_sys_dev_boot(void) {
 
         // disable various ipc sockets in /run/user
         if (!arg_writable_run_user) {
-                struct stat s;
-
                 char *fname;
                 if (asprintf(&fname, "/run/user/%d", getuid()) == -1)
                         errExit("asprintf");
@@ -755,8 +758,7 @@ void fs_proc_sys_dev_boot(void) {
                                 errExit("asprintf");
                         if (create_empty_dir_as_user(fnamegpg, 0700))
                                 fs_logger2("create", fnamegpg);
-                        if (stat(fnamegpg, &s) == 0)
-                                disable_file(BLACKLIST_FILE, fnamegpg);
+                        disable_file(BLACKLIST_FILE, fnamegpg);
                         free(fnamegpg);
 
                         // disable /run/user/{uid}/systemd
@@ -765,8 +767,7 @@ void fs_proc_sys_dev_boot(void) {
                                 errExit("asprintf");
                         if (create_empty_dir_as_user(fnamesysd, 0755))
                                 fs_logger2("create", fnamesysd);
-                        if (stat(fnamesysd, &s) == 0)
-                                disable_file(BLACKLIST_FILE, fnamesysd);
+                        disable_file(BLACKLIST_FILE, fnamesysd);
                         free(fnamesysd);
                 }
                 free(fname);
@@ -781,26 +782,18 @@ void fs_proc_sys_dev_boot(void) {
 
 // disable firejail configuration in ~/.config/firejail
 void disable_config(void) {
-        struct stat s;
-
         char *fname;
         if (asprintf(&fname, "%s/.config/firejail", cfg.homedir) == -1)
                 errExit("asprintf");
-        if (stat(fname, &s) == 0)
-                disable_file(BLACKLIST_FILE, fname);
+        disable_file(BLACKLIST_FILE, fname);
         free(fname);
 
         // disable run time information
-        if (stat(RUN_FIREJAIL_NETWORK_DIR, &s) == 0)
-                disable_file(BLACKLIST_FILE, RUN_FIREJAIL_NETWORK_DIR);
-        if (stat(RUN_FIREJAIL_BANDWIDTH_DIR, &s) == 0)
-                disable_file(BLACKLIST_FILE, RUN_FIREJAIL_BANDWIDTH_DIR);
-        if (stat(RUN_FIREJAIL_NAME_DIR, &s) == 0)
-                disable_file(BLACKLIST_FILE, RUN_FIREJAIL_NAME_DIR);
-        if (stat(RUN_FIREJAIL_PROFILE_DIR, &s) == 0)
-                disable_file(BLACKLIST_FILE, RUN_FIREJAIL_PROFILE_DIR);
-        if (stat(RUN_FIREJAIL_X11_DIR, &s) == 0)
-                disable_file(BLACKLIST_FILE, RUN_FIREJAIL_X11_DIR);
+        disable_file(BLACKLIST_FILE, RUN_FIREJAIL_NETWORK_DIR);
+        disable_file(BLACKLIST_FILE, RUN_FIREJAIL_BANDWIDTH_DIR);
+        disable_file(BLACKLIST_FILE, RUN_FIREJAIL_NAME_DIR);
+        disable_file(BLACKLIST_FILE, RUN_FIREJAIL_PROFILE_DIR);
+        disable_file(BLACKLIST_FILE, RUN_FIREJAIL_X11_DIR);
 }
 
 
@@ -1241,8 +1234,8 @@ void fs_private_tmp(void) {
 
         // whitelist x11 directory
         profile_add("whitelist /tmp/.X11-unix");
-        // read-only x11 directory
-        profile_add("read-only /tmp/.X11-unix");
+        // read-only x11 directory
+        profile_add("read-only /tmp/.X11-unix");
 
         // whitelist any pulse* file in /tmp directory
         // some distros use PulseAudio sockets under /tmp instead of the socket in /urn/user