major cleanup and testing

9 files changed, 80 insertions, 147 deletions

diff --git a/src/firejail/appimage.c b/src/firejail/appimage.c
index 96c054048..a658173eb 100644
--- a/src/firejail/appimage.c
+++ b/src/firejail/appimage.c
@@ -94,14 +94,8 @@ void appimage_set(const char *appimage_path) {
         if (asprintf(&mntdir, "%s/.appimage-%u",  RUN_FIREJAIL_APPIMAGE_DIR, getpid()) == -1)
                 errExit("asprintf");
         EUID_ROOT();
-        if (mkdir(mntdir, 0700) == -1) {
-                fprintf(stderr, "Error: cannot create appimage mount point\n");
-                exit(1);
-        }
-        if (set_perms(mntdir, getuid(), getgid(), 0700))
-                errExit("set_perms");
+        mkdir_attr(mntdir, 0700, getuid(), getgid());
         EUID_USER();
-        ASSERT_PERMS(mntdir, getuid(), getgid(), 0700);
         
         // mount
         char *mode;
diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index 282271a64..d7ba539e6 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -461,6 +461,7 @@ void flush_stdin(void);
 void create_empty_dir_as_root(const char *dir, mode_t mode);
 void create_empty_file_as_root(const char *dir, mode_t mode);
 int set_perms(const char *fname, uid_t uid, gid_t gid, mode_t mode);
+void mkdir_attr(const char *fname, mode_t mode, uid_t uid, gid_t gid);
 
 // fs_var.c
 void fs_var_log(void);  // mounting /var/log
diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index 3a2fd8c38..7ff7e3c59 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -247,21 +247,13 @@ void fs_blacklist(void) {
 
                 // process bind command
                 if (strncmp(entry->data, "bind ", 5) == 0)  {
+                        struct stat s;
                         char *dname1 = entry->data + 5;
                         char *dname2 = split_comma(dname1);
-                        if (dname2 == NULL) {
-                                fprintf(stderr, "Error: second directory missing in bind command\n");
-                                entry = entry->next;
-                                continue;
-                        }
-                        struct stat s;
-                        if (stat(dname1, &s) == -1) {
-                                fprintf(stderr, "Error: cannot find %s for bind command\n", dname1);
-                                entry = entry->next;
-                                continue;
-                        }
-                        if (stat(dname2, &s) == -1) {
-                                fprintf(stderr, "Error: cannot find %s for bind command\n", dname2);
+                        if (dname2 == NULL ||
+                            stat(dname1, &s) == -1 ||
+                            stat(dname2, &s) == -1) {
+                                fprintf(stderr, "Error: invalid bind command, directory missing\n");
                                 entry = entry->next;
                                 continue;
                         }
@@ -410,10 +402,9 @@ void fs_rdonly(const char *dir) {
         int rv = stat(dir, &s);
         if (rv == 0) {
                 // mount --bind /bin /bin
-                if (mount(dir, dir, NULL, MS_BIND|MS_REC, NULL) < 0)
-                        errExit("mount read-only");
                 // mount --bind -o remount,ro /bin
-                if (mount(NULL, dir, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY|MS_REC, NULL) < 0)
+                if (mount(dir, dir, NULL, MS_BIND|MS_REC, NULL) < 0 ||
+                    mount(NULL, dir, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY|MS_REC, NULL) < 0)
                         errExit("mount read-only");
                 fs_logger2("read-only", dir);
         }
@@ -428,15 +419,15 @@ static void fs_rdwr(const char *dir) {
                 // if the file is outside /home directory, allow only root user
                 uid_t u = getuid();
                 if (u != 0 && s.st_uid != u) {
-                        fprintf(stderr, "Warning: you are not allowed to change %s to read-write\n", dir);
+                        if (!arg_quiet)
+                                fprintf(stderr, "Warning: you are not allowed to change %s to read-write\n", dir);
                         return;
                 }
                 
                 // mount --bind /bin /bin
-                if (mount(dir, dir, NULL, MS_BIND|MS_REC, NULL) < 0)
-                        errExit("mount read-write");
                 // mount --bind -o remount,rw /bin
-                if (mount(NULL, dir, NULL, MS_BIND|MS_REMOUNT|MS_REC, NULL) < 0)
+                if (mount(dir, dir, NULL, MS_BIND|MS_REC, NULL) < 0 || 
+                    mount(NULL, dir, NULL, MS_BIND|MS_REMOUNT|MS_REC, NULL) < 0)
                         errExit("mount read-write");
                 fs_logger2("read-write", dir);
         }
@@ -449,37 +440,16 @@ void fs_noexec(const char *dir) {
         int rv = stat(dir, &s);
         if (rv == 0) {
                 // mount --bind /bin /bin
-                if (mount(dir, dir, NULL, MS_BIND|MS_REC, NULL) < 0)
-                        errExit("mount noexec");
                 // mount --bind -o remount,ro /bin
-                if (mount(NULL, dir, NULL, MS_BIND|MS_REMOUNT|MS_NOEXEC|MS_NODEV|MS_NOSUID|MS_REC, NULL) < 0)
-                        errExit("mount read-only");
+                if (mount(dir, dir, NULL, MS_BIND|MS_REC, NULL) < 0 || 
+                    mount(NULL, dir, NULL, MS_BIND|MS_REMOUNT|MS_NOEXEC|MS_NODEV|MS_NOSUID|MS_REC, NULL) < 0)
+                        errExit("mount noexec");
                 fs_logger2("noexec", dir);
         }
 }
 
 
 
-void fs_rdonly_noexit(const char *dir) {
-        assert(dir);
-        // check directory exists
-        struct stat s;
-        int rv = stat(dir, &s);
-        if (rv == 0) {
-                int merr = 0;
-                // mount --bind /bin /bin
-                if (mount(dir, dir, NULL, MS_BIND|MS_REC, NULL) < 0)
-                        merr = 1;
-                // mount --bind -o remount,ro /bin
-                if (mount(NULL, dir, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY|MS_REC, NULL) < 0)
-                        merr = 1;
-                if (merr)
-                        fprintf(stderr, "Warning: cannot mount %s read-only\n", dir); 
-                else
-                        fs_logger2("read-only", dir);
-        }
-}
-
 // mount /proc and /sys directories
 void fs_proc_sys_dev_boot(void) {
         if (arg_debug)
@@ -489,10 +459,8 @@ void fs_proc_sys_dev_boot(void) {
         fs_logger("remount /proc");
 
         // remount /proc/sys readonly
-        if (mount("/proc/sys", "/proc/sys", NULL, MS_BIND | MS_REC, NULL) < 0)
-                errExit("mounting /proc/sys");
-
-        if (mount(NULL, "/proc/sys", NULL, MS_BIND | MS_REMOUNT | MS_RDONLY | MS_REC, NULL) < 0)
+        if (mount("/proc/sys", "/proc/sys", NULL, MS_BIND | MS_REC, NULL) < 0 ||
+            mount(NULL, "/proc/sys", NULL, MS_BIND | MS_REMOUNT | MS_RDONLY | MS_REC, NULL) < 0)
                 errExit("mounting /proc/sys");
         fs_logger("read-only /proc/sys");
 
@@ -646,12 +614,7 @@ char *fs_check_overlay_dir(const char *subdirname, int allow_reuse) {
         if (asprintf(&dirname, "%s/.firejail", cfg.homedir) == -1)
                 errExit("asprintf");
         if (stat(dirname, &s) == -1) {
-                /* coverity[toctou] */
-                if (mkdir(dirname, 0700))
-                        errExit("mkdir");
-                if (chmod(dirname, 0700) == -1)
-                        errExit("chmod");
-                ASSERT_PERMS(dirname, getuid(), getgid(), 0700);
+                mkdir_attr(dirname, 0700, 0, 0);
         }
         else if (is_link(dirname)) {
                 fprintf(stderr, "Error: invalid ~/.firejail directory\n");
@@ -733,11 +696,7 @@ void fs_overlayfs(void) {
         char *oroot;
         if(asprintf(&oroot, "%s/oroot", RUN_MNT_DIR) == -1)
                 errExit("asprintf");
-        if (mkdir(oroot, 0755))
-                errExit("mkdir");
-        if (chmod(oroot, 0755) == -1)
-                errExit("chmod");
-        ASSERT_PERMS(oroot, 0, 0, 0755);
+        mkdir_attr(oroot, 0755, 0, 0);
 
         struct stat s;
         char *basedir = RUN_MNT_DIR;
@@ -766,11 +725,9 @@ void fs_overlayfs(void) {
 
         // no need to check arg_overlay_reuse
         if (stat(odiff, &s) != 0) {
-                if (mkdir(odiff, 0755))
-                        errExit("mkdir");
+                mkdir_attr(odiff, 0755, 0, 0);
         }
-
-        if (set_perms(odiff, 0, 0, 0755))
+        else if (set_perms(odiff, 0, 0, 0755))
                 errExit("set_perms");
         
         char *owork;
@@ -779,11 +736,9 @@ void fs_overlayfs(void) {
 
         // no need to check arg_overlay_reuse
         if (stat(owork, &s) != 0) {
-                if (mkdir(owork, 0755))
-                        errExit("mkdir");
+                mkdir_attr(owork, 0755, 0, 0);
         }
-
-        if (set_perms(owork, 0, 0, 0755))
+        else if (set_perms(owork, 0, 0, 0755))
                 errExit("chown");
         
         // mount overlayfs
@@ -839,11 +794,9 @@ void fs_overlayfs(void) {
 
                                 // no need to check arg_overlay_reuse
                                 if (stat(hdiff, &s) != 0) {
-                                        if (mkdir(hdiff, S_IRWXU | S_IRWXG | S_IRWXO))
-                                                errExit("mkdir");
+                                        mkdir_attr(hdiff, S_IRWXU | S_IRWXG | S_IRWXO, 0, 0);
                                 }
-
-                                if (set_perms(hdiff, 0, 0, S_IRWXU  | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH))
+                                else if (set_perms(hdiff, 0, 0, S_IRWXU  | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH))
                                         errExit("set_perms");
                 
                                 if(asprintf(&hwork, "%s/hwork", basedir) == -1)
@@ -851,11 +804,9 @@ void fs_overlayfs(void) {
 
                                 // no need to check arg_overlay_reuse
                                 if (stat(hwork, &s) != 0) {
-                                        if (mkdir(hwork, S_IRWXU | S_IRWXG | S_IRWXO))
-                                                errExit("mkdir");
+                                        mkdir_attr(hwork, S_IRWXU | S_IRWXG | S_IRWXO, 0, 0);
                                 }
-
-                                if (set_perms(hwork, 0, 0, S_IRWXU  | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH))
+                                else if (set_perms(hwork, 0, 0, S_IRWXU  | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH))
                                         errExit("set_perms");
                 
                                 // no homedir in overlay so now mount another overlay for /home
diff --git a/src/firejail/fs_bin.c b/src/firejail/fs_bin.c
index c906e32c0..6cc1bf3ab 100644
--- a/src/firejail/fs_bin.c
+++ b/src/firejail/fs_bin.c
@@ -212,11 +212,7 @@ void fs_private_bin_list(void) {
         assert(private_list);
         
         // create /run/firejail/mnt/bin directory
-        if (mkdir(RUN_BIN_DIR, 0755) == -1)
-                errExit("mkdir");
-        if (chmod(RUN_BIN_DIR, 0755) == -1)
-                errExit("chmod");
-        ASSERT_PERMS(RUN_BIN_DIR, 0, 0, 0755);
+        mkdir_attr(RUN_BIN_DIR, 0755, 0, 0);
         
         // copy the list of files in the new etc directory
         // using a new child process without root privileges
diff --git a/src/firejail/fs_dev.c b/src/firejail/fs_dev.c
index ef5d67b55..d710e98f2 100644
--- a/src/firejail/fs_dev.c
+++ b/src/firejail/fs_dev.c
@@ -65,11 +65,7 @@ static void deventry_mount(void) {
                         if (arg_debug)
                                 printf("mounting %s %s\n", dev[i].run_fname, (dir)? "directory": "file");
                         if (dir) {
-                                if (mkdir(dev[i].dev_fname, 0755) == -1)
-                                        errExit("mkdir");
-                                if (chmod(dev[i].dev_fname, 0755) == -1)
-                                        errExit("chmod");
-                                ASSERT_PERMS(dev[i].dev_fname, 0, 0, 0755);
+                                mkdir_attr(dev[i].dev_fname, 0755, 0, 0);
                         }
                         else {
                                 struct stat s;
@@ -130,11 +126,7 @@ void fs_private_dev(void){
 
         // create DRI_DIR
         // keep a copy of dev directory
-        if (mkdir(RUN_DEV_DIR, 0755) == -1)
-                errExit("mkdir");
-        if (chmod(RUN_DEV_DIR, 0755) == -1)
-                errExit("chmod");
-        ASSERT_PERMS(RUN_DEV_DIR, 0, 0, 0755);
+        mkdir_attr(RUN_DEV_DIR, 0755, 0, 0);
         if (mount("/dev", RUN_DEV_DIR, NULL, MS_BIND|MS_REC, NULL) < 0)
                 errExit("mounting /dev/dri");
 
@@ -179,12 +171,7 @@ void fs_private_dev(void){
         // create /dev/shm
         if (arg_debug)
                 printf("Create /dev/shm directory\n");
-        if (mkdir("/dev/shm", 01777) == -1)
-                errExit("mkdir");
-        // mkdir sets only the file permission bits
-        if (chmod("/dev/shm", 01777) < 0)
-                errExit("chmod");
-        ASSERT_PERMS("/dev/shm", 0, 0, 01777);
+        mkdir_attr("/dev/shm", 01777, 0, 0);
         fs_logger("mkdir /dev/shm");
 
         // create devices
@@ -206,11 +193,7 @@ void fs_private_dev(void){
 #endif
 
         // pseudo-terminal
-        if (mkdir("/dev/pts", 0755) == -1)
-                errExit("mkdir");
-        if (chmod("/dev/pts", 0755) == -1)
-                errExit("chmod");
-        ASSERT_PERMS("/dev/pts", 0, 0, 0755);
+        mkdir_attr("/dev/pts", 0755, 0, 0);
         fs_logger("mkdir /dev/pts");
         create_char_dev("/dev/pts/ptmx", 0666, 5, 2); //"mknod -m 666 /dev/pts/ptmx c 5 2");
         fs_logger("mknod /dev/pts/ptmx");
@@ -260,12 +243,7 @@ void fs_dev_shm(void) {
                 if (lnk) {
                         if (!is_dir(lnk)) {
                                 // create directory
-                                if (mkdir(lnk, 01777))
-                                        errExit("mkdir");
-                                // mkdir sets only the file permission bits
-                                if (chmod(lnk, 01777))
-                                        errExit("chmod");
-                                ASSERT_PERMS(lnk, 0, 0, 01777);
+                                mkdir_attr(lnk, 01777, 0, 0);
                         }
                         if (arg_debug)
                                 printf("Mounting tmpfs on %s on behalf of /dev/shm\n", lnk);
diff --git a/src/firejail/fs_etc.c b/src/firejail/fs_etc.c
index ebcde29a3..7e18840fd 100644
--- a/src/firejail/fs_etc.c
+++ b/src/firejail/fs_etc.c
@@ -132,11 +132,7 @@ void fs_private_etc_list(void) {
         }
 
         // create /run/firejail/mnt/etc directory
-        if (mkdir(RUN_ETC_DIR, 0755) == -1)
-                errExit("mkdir");
-        if (chmod(RUN_ETC_DIR, 0755) == -1)
-                errExit("chmod");
-        ASSERT_PERMS(RUN_ETC_DIR, 0, 0, 0755);
+        mkdir_attr(RUN_ETC_DIR, 0755, 0, 0);
         fs_logger("tmpfs /etc");
         
         fs_logger_print();      // save the current log
diff --git a/src/firejail/fs_var.c b/src/firejail/fs_var.c
index 4ff00f3ba..ca50685ad 100644
--- a/src/firejail/fs_var.c
+++ b/src/firejail/fs_var.c
@@ -98,10 +98,7 @@ static void build_dirs(void) {
         // create directories under /var/log
         DirData *ptr = dirlist;
         while (ptr) {
-                if (mkdir(ptr->name, ptr->st_mode))
-                        errExit("mkdir");
-                if (chown(ptr->name, ptr->st_uid, ptr->st_gid))
-                        errExit("chown");
+                mkdir_attr(ptr->name, ptr->st_mode, ptr->st_uid, ptr->st_gid);
                 fs_logger2("mkdir", ptr->name);
                 ptr = ptr->next;
         }
@@ -223,18 +220,10 @@ void fs_var_cache(void) {
                         gid = p->pw_gid;
                 }
                 
-                int rv = mkdir("/var/cache/lighttpd/compress", 0755);
-                if (rv == -1)
-                        errExit("mkdir");
-                if (chown("/var/cache/lighttpd/compress", uid, gid) < 0)
-                        errExit("chown");
+                mkdir_attr("/var/cache/lighttpd/compress", 0755, uid, gid);
                 fs_logger("mkdir /var/cache/lighttpd/compress");
 
-                rv = mkdir("/var/cache/lighttpd/uploads", 0755);
-                if (rv == -1)
-                        errExit("mkdir");
-                if (chown("/var/cache/lighttpd/uploads", uid, gid) < 0)
-                        errExit("chown");
+                mkdir_attr("/var/cache/lighttpd/uploads", 0755, uid, gid);
                 fs_logger("/var/cache/lighttpd/uploads");
         }                       
 }
@@ -268,11 +257,7 @@ void fs_var_lock(void) {
                 if (lnk) {
                         if (!is_dir(lnk)) {
                                 // create directory
-                                if (mkdir(lnk, S_IRWXU|S_IRWXG|S_IRWXO))
-                                        errExit("mkdir");
-                                if (chmod(lnk, S_IRWXU|S_IRWXG|S_IRWXO))
-                                        errExit("chmod");
-                                ASSERT_PERMS(lnk, 0, 0, S_IRWXU|S_IRWXG|S_IRWXO);
+                                mkdir_attr(lnk, S_IRWXU|S_IRWXG|S_IRWXO, 0, 0);
                         }
                         if (arg_debug)
                                 printf("Mounting tmpfs on %s on behalf of /var/lock\n", lnk);
diff --git a/src/firejail/join.c b/src/firejail/join.c
index 899166447..628002d35 100644
--- a/src/firejail/join.c
+++ b/src/firejail/join.c
@@ -229,15 +229,11 @@ void join(pid_t pid, int argc, char **argv, int index) {
                         exit(1);
         }
         else {
-                if (join_namespace(pid, "ipc"))
-                        exit(1);
-                if (join_namespace(pid, "net"))
-                        exit(1);
-                if (join_namespace(pid, "pid"))
-                        exit(1);
-                if (join_namespace(pid, "uts"))
-                        exit(1);
-                if (join_namespace(pid, "mnt"))
+                if (join_namespace(pid, "ipc") ||
+                    join_namespace(pid, "net") ||
+                    join_namespace(pid, "pid") ||
+                    join_namespace(pid, "uts") ||
+                    join_namespace(pid, "mnt"))
                         exit(1);
         }
 
diff --git a/src/firejail/util.c b/src/firejail/util.c
index 3424d8ab6..d928c6b42 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -731,4 +731,40 @@ int set_perms(const char *fname, uid_t uid, gid_t gid, mode_t mode) {
         return 0;
 }
 
+void mkdir_attr(const char *fname, mode_t mode, uid_t uid, gid_t gid) {
+        assert(fname);
+        mode &= 07777;
+#if 0   
+        printf("fname %s, uid %d, gid %d, mode %x - ", fname, uid, gid, (unsigned) mode);
+        if (S_ISLNK(mode))
+                printf("l");
+        else if (S_ISDIR(mode))
+                printf("d");
+        else if (S_ISCHR(mode))
+                printf("c");
+        else if (S_ISBLK(mode))
+                printf("b");
+        else if (S_ISSOCK(mode))
+                printf("s");
+        else
+                printf("-");
+        printf( (mode & S_IRUSR) ? "r" : "-");
+        printf( (mode & S_IWUSR) ? "w" : "-");
+        printf( (mode & S_IXUSR) ? "x" : "-");
+        printf( (mode & S_IRGRP) ? "r" : "-");
+        printf( (mode & S_IWGRP) ? "w" : "-");
+        printf( (mode & S_IXGRP) ? "x" : "-");
+        printf( (mode & S_IROTH) ? "r" : "-");
+        printf( (mode & S_IWOTH) ? "w" : "-");
+        printf( (mode & S_IXOTH) ? "x" : "-");
+        printf("\n");
+#endif  
+        if (mkdir(fname, mode) == -1 ||
+            chmod(fname, mode) == -1 ||
+            chown(fname, uid, gid)) {
+                fprintf(stderr, "Error: failed to create %s directory\n", fname);
+                errExit("mkdir/chmod");
+        }
 
+        ASSERT_PERMS(fname, uid, gid, mode);
+}