Merge pull request #2033 from smitsohu/whitelist

additional whitelist hardening

2 files changed, 127 insertions, 118 deletions

diff --git a/src/firejail/fs_whitelist.c b/src/firejail/fs_whitelist.c
index d52b3996a..9fbe45726 100644
--- a/src/firejail/fs_whitelist.c
+++ b/src/firejail/fs_whitelist.c
@@ -191,189 +191,192 @@ static int mkpath(const char* path, mode_t mode) {
 
 static void whitelist_path(ProfileEntry *entry) {
         assert(entry);
-        char *path = entry->data + 10;
+        const char *path = entry->data + 10;
         assert(path);
         const char *fname;
         char *wfile = NULL;
 
         if (entry->home_dir) {
-                if (strncmp(path, cfg.homedir, strlen(cfg.homedir)) == 0) {
-                        fname = path + strlen(cfg.homedir);
-                        if (*fname == '\0')
-                                goto errexit;
-                }
-                else
+                if (strncmp(path, cfg.homedir, strlen(cfg.homedir)) != 0)
                         // symlink pointing outside /home, skip the mount
                         return;
 
+                fname = path + strlen(cfg.homedir);
+
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_HOME_USER_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->tmp_dir) {
                 fname = path + 5; // strlen("/tmp/")
-#ifndef TEST_MOUNTINFO
-                if (*fname == '\0')
-                        errLogExit("whitelisting /tmp problem");
-#endif
 
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_TMP_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->media_dir) {
                 fname = path + 7; // strlen("/media/")
-                if (*fname == '\0')
-                        goto errexit;
 
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_MEDIA_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->mnt_dir) {
                 fname = path + 5; // strlen("/mnt/")
-                if (*fname == '\0')
-                        goto errexit;
 
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_MNT_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->var_dir) {
-                if (strncmp(path, "/var/", 5) == 0) {
-                        fname = path + 5; // strlen("/var/")
-                        if (*fname == '\0')
-                                goto errexit;
-                }
-                else
+                if (strncmp(path, "/var/", 5) != 0)
                         // symlink pointing outside /var, skip the mount
                         return;
 
+                fname = path + 5; // strlen("/var/")
+
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_VAR_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->dev_dir) {
-                if (strncmp(path, "/dev/", 5) == 0) {
-                        fname = path + 5; // strlen("/dev/")
-                        if (*fname == '\0')
-                                goto errexit;
-                }
-                else
+                if (strncmp(path, "/dev/", 5) != 0)
                         // symlink pointing outside /dev, skip the mount
                         return;
 
+                fname = path + 5; // strlen("/dev/")
+
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_DEV_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->opt_dir) {
                 fname = path + 5; // strlen("/opt/")
-                if (*fname == '\0')
-                        goto errexit;
 
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_OPT_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->srv_dir) {
                 fname = path + 5; // strlen("/srv/")
-                if (*fname == '\0')
-                        goto errexit;
 
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_SRV_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->etc_dir) {
-                if (strncmp(path, "/etc/", 5) == 0) {
-                        fname = path + 5; // strlen("/etc/")
-                        if (*fname == '\0')
-                                goto errexit;
-                }
-                else
+                if (strncmp(path, "/etc/", 5) != 0)
                         // symlink pointing outside /etc, skip the mount
                         return;
 
+                fname = path + 5; // strlen("/etc/")
+
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_ETC_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->share_dir) {
                 fname = path + 11; // strlen("/usr/share/")
-                if (*fname == '\0')
-                        goto errexit;
 
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_SHARE_DIR, fname) == -1)
                         errExit("asprintf");
         }
         else if (entry->module_dir) {
                 fname = path + 12; // strlen("/sys/module/")
-                if (*fname == '\0')
-                        goto errexit;
 
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_MODULE_DIR, fname) == -1)
                         errExit("asprintf");
         }
         assert(wfile);
 
-        // check if the file exists
+        // check if the file exists, confirm again there is no symlink
         EUID_USER();
-        struct stat s;
-        if (stat(wfile, &s) == 0) {
-                if (arg_debug || arg_debug_whitelists)
-                        printf("Whitelisting %s\n", path);
-        }
-        else {
+        int fd = safe_fd(wfile, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        if (fd == -1) {
                 free(wfile);
                 EUID_ROOT();
                 return;
         }
-        EUID_ROOT();
+        struct stat wfilestat;
+        if (fstat(fd, &wfilestat) == -1)
+                errExit("fstat");
+        if (S_ISLNK(wfilestat.st_mode)) {
+                fprintf(stderr, "Error: unexpected symbolic link %s\n", path);
+                exit(1);
+        }
+        close(fd);
 
-        // create the path if necessary
-        mkpath(path, s.st_mode);
+        if (arg_debug || arg_debug_whitelists)
+                printf("Whitelisting %s\n", path);
         fs_logger2("whitelist", path);
 
-        // process directory
-        if (S_ISDIR(s.st_mode)) {
-                // create directory
-                int rv = mkdir(path, 0755);
-                (void) rv;
-        }
-
-        // process regular file
-        else {
-                if (access(path, R_OK)) {
-                        // create an empty file
-                        FILE *fp = fopen(path, "w");
-                        if (!fp) {
-                                fprintf(stderr, "Error: cannot create empty file in home directory\n");
+        // create the path if necessary
+        EUID_ROOT();
+        struct stat s;
+        if (stat(path, &s) == -1) {
+                mkpath(path, 0755);
+                if (S_ISDIR(wfilestat.st_mode)) {
+                        int rv = mkdir(path, 0755);
+                        if (rv) {
+                                fprintf(stderr, "Error: cannot create directory %s\n", path);
                                 exit(1);
                         }
-                        // set file properties
-                        SET_PERMS_STREAM(fp, s.st_uid, s.st_gid, s.st_mode);
-                        fclose(fp);
                 }
                 else {
+                        int fd2 = open(path, O_RDONLY|O_CREAT|O_EXCL|O_CLOEXEC, S_IRUSR|S_IWUSR);
+                        if (fd2 == -1) {
+                                fprintf(stderr, "Error: cannot create empty file %s\n", path);
+                                exit(1);
+                        }
+                        close(fd2);
+                }
+        }
+        else {
+                if (!S_ISDIR(s.st_mode)) {
                         free(wfile);
                         return; // the file is already present
                 }
         }
 
-        // mount
-        if (mount(wfile, path, NULL, MS_BIND|MS_REC, NULL) < 0)
+        // get a file descriptor for path; if path contains anything other than directories
+        // or a regular file, assume it is whitelisted already
+        int fd3 = safe_fd(path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        if (fd3 == -1) {
+                free(wfile);
+                return;
+        }
+        if (fstat(fd3, &s) == -1)
+                errExit("fstat");
+        if (!(S_ISDIR(s.st_mode) || S_ISREG(s.st_mode))) {
+                free(wfile);
+                close(fd3);
+                return;
+        }
+
+        // mount via the link in /proc/self/fd
+        char *proc;
+        if (asprintf(&proc, "/proc/self/fd/%d", fd3) == -1)
+                errExit("asprintf");
+        if (mount(wfile, proc, NULL, MS_BIND|MS_REC, NULL) < 0)
                 errExit("mount bind");
+        free(proc);
+        close(fd3);
 
         // check the last mount operation
         MountData *mptr = get_last_mount(); // will do exit(1) if the mount cannot be found
 
+        if (strncmp(mptr->dir, path, strlen(path)) != 0)
+                errLogExit("invalid whitelist mount");
         // No mounts are allowed on top level directories. A destination such as "/etc" is very bad!
         //  - there should be more than one '/' char in dest string
         if (mptr->dir == strrchr(mptr->dir, '/'))
-                errLogExit("invalid whitelist mount\n");
+                errLogExit("invalid whitelist mount");
+        // confirm the correct file is mounted on path
+        int fd4 = safe_fd(path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        if (fd4 == -1)
+                errExit("safe_fd");
+        if (fstat(fd4, &s) == -1)
+                errExit("fstat");
+        if (s.st_dev != wfilestat.st_dev || s.st_ino != wfilestat.st_ino)
+                errLogExit("invalid whitelist mount");
+        close(fd4);
 
         free(wfile);
         return;
-
-errexit:
-        fprintf(stderr, "Error: file %s is not in the whitelisted directory\n", path);
-        exit(1);
 }
 
 
-// whitelist for /home/user directory
 void fs_whitelist(void) {
         char *homedir = cfg.homedir;
         assert(homedir);
@@ -402,6 +405,7 @@ void fs_whitelist(void) {
 
         // verify whitelist files, extract symbolic links, etc.
         EUID_USER();
+        struct stat s;
         while (entry) {
                 int nowhitelist_flag = 0;
 
@@ -544,10 +548,14 @@ void fs_whitelist(void) {
                                         __LINE__, fname, cfg.homedir);
 
                         // both path and absolute path are under /home
-                        if (strncmp(fname, cfg.homedir, strlen(cfg.homedir)) != 0) {
+                        if (strncmp(fname, cfg.homedir, strlen(cfg.homedir)) == 0) {
+                                // entire home directory is not allowed
+                                if (*(fname + strlen(cfg.homedir)) != '/')
+                                        goto errexit;
+                        }
+                        else {
                                 if (checkcfg(CFG_FOLLOW_SYMLINK_AS_USER)) {
                                         // check if the file is owned by the user
-                                        struct stat s;
                                         if (stat(fname, &s) == 0 && s.st_uid != getuid())
                                                 goto errexit;
                                 }
@@ -709,20 +717,25 @@ void fs_whitelist(void) {
         free(nowhitelist);
 
         EUID_ROOT();
-        // /home/user
+        // /home/user mountpoint
         if (home_dir) {
-                // keep a copy of real home dir in RUN_WHITELIST_HOME_USER_DIR
-                mkdir_attr(RUN_WHITELIST_HOME_USER_DIR, 0755, getuid(), getgid());
-                if (mount(cfg.homedir, RUN_WHITELIST_HOME_USER_DIR, NULL, MS_BIND|MS_REC, NULL) < 0)
-                        errExit("mount bind");
+                // check if /home/user directory exists
+                if (stat(cfg.homedir, &s) == 0) {
+                        // keep a copy of real home dir in RUN_WHITELIST_HOME_USER_DIR
+                        mkdir_attr(RUN_WHITELIST_HOME_USER_DIR, 0755, getuid(), getgid());
+                        if (mount(cfg.homedir, RUN_WHITELIST_HOME_USER_DIR, NULL, MS_BIND|MS_REC, NULL) < 0)
+                                errExit("mount bind");
 
-                // mount a tmpfs and initialize /home/user
-                 fs_private();
+                        // mount a tmpfs and initialize /home/user
+                        fs_private();
+                }
+                else
+                        home_dir = 0;
         }
 
         // /tmp mountpoint
         if (tmp_dir) {
-                // keep a copy of real /tmp directory in
+                // keep a copy of real /tmp directory in RUN_WHITELIST_TMP_DIR
                 mkdir_attr(RUN_WHITELIST_TMP_DIR, 1777, 0, 0);
                 if (mount("/tmp", RUN_WHITELIST_TMP_DIR, NULL, MS_BIND|MS_REC, NULL) < 0)
                         errExit("mount bind");
@@ -738,7 +751,6 @@ void fs_whitelist(void) {
         // /media mountpoint
         if (media_dir) {
                 // some distros don't have a /media directory
-                struct stat s;
                 if (stat("/media", &s) == 0) {
                         // keep a copy of real /media directory in RUN_WHITELIST_MEDIA_DIR
                         mkdir_attr(RUN_WHITELIST_MEDIA_DIR, 0755, 0, 0);
@@ -759,7 +771,6 @@ void fs_whitelist(void) {
         // /mnt mountpoint
         if (mnt_dir) {
                 // check if /mnt directory exists
-                struct stat s;
                 if (stat("/mnt", &s) == 0) {
                         // keep a copy of real /mnt directory in RUN_WHITELIST_MNT_DIR
                         mkdir_attr(RUN_WHITELIST_MNT_DIR, 0755, 0, 0);
@@ -810,23 +821,27 @@ void fs_whitelist(void) {
 
         // /opt mountpoint
         if (opt_dir) {
-                // keep a copy of real /opt directory in RUN_WHITELIST_OPT_DIR
-                mkdir_attr(RUN_WHITELIST_OPT_DIR, 0755, 0, 0);
-                if (mount("/opt", RUN_WHITELIST_OPT_DIR, NULL, MS_BIND|MS_REC, NULL) < 0)
-                        errExit("mount bind");
+                // check if /opt directory exists
+                if (stat("/opt", &s) == 0) {
+                        // keep a copy of real /opt directory in RUN_WHITELIST_OPT_DIR
+                        mkdir_attr(RUN_WHITELIST_OPT_DIR, 0755, 0, 0);
+                        if (mount("/opt", RUN_WHITELIST_OPT_DIR, NULL, MS_BIND|MS_REC, NULL) < 0)
+                                errExit("mount bind");
 
-                // mount tmpfs on /opt
-                if (arg_debug || arg_debug_whitelists)
-                        printf("Mounting tmpfs on /opt directory\n");
-                if (mount("tmpfs", "/opt", "tmpfs", MS_NOSUID | MS_STRICTATIME | MS_REC,  "mode=755,gid=0") < 0)
-                        errExit("mounting tmpfs on /opt");
-                fs_logger("tmpfs /opt");
+                        // mount tmpfs on /opt
+                        if (arg_debug || arg_debug_whitelists)
+                                printf("Mounting tmpfs on /opt directory\n");
+                        if (mount("tmpfs", "/opt", "tmpfs", MS_NOSUID | MS_STRICTATIME | MS_REC,  "mode=755,gid=0") < 0)
+                                errExit("mounting tmpfs on /opt");
+                        fs_logger("tmpfs /opt");
+                }
+                else
+                        opt_dir = 0;
         }
 
         // /srv mountpoint
         if (srv_dir) {
                 // check if /srv directory exists
-                struct stat s;
                 if (stat("/srv", &s) == 0) {
                         // keep a copy of real /srv directory in RUN_WHITELIST_SRV_DIR
                         mkdir_attr(RUN_WHITELIST_SRV_DIR, 0755, 0, 0);
@@ -847,7 +862,6 @@ void fs_whitelist(void) {
         // /etc mountpoint
         if (etc_dir) {
                 // check if /etc directory exists
-                struct stat s;
                 if (stat("/etc", &s) == 0) {
                         // keep a copy of real /etc directory in RUN_WHITELIST_ETC_DIR
                         mkdir_attr(RUN_WHITELIST_ETC_DIR, 0755, 0, 0);
@@ -868,7 +882,6 @@ void fs_whitelist(void) {
         // /usr/share mountpoint
         if (share_dir) {
                 // check if /usr/share directory exists
-                struct stat s;
                 if (stat("/usr/share", &s) == 0) {
                         // keep a copy of real /usr/share directory in RUN_WHITELIST_ETC_DIR
                         mkdir_attr(RUN_WHITELIST_SHARE_DIR, 0755, 0, 0);
@@ -889,7 +902,6 @@ void fs_whitelist(void) {
         // /sys/module mountpoint
         if (module_dir) {
                 // check if /sys/module directory exists
-                struct stat s;
                 if (stat("/sys/module", &s) == 0) {
                         // keep a copy of real /sys/module directory in RUN_WHITELIST_MODULE_DIR
                         mkdir_attr(RUN_WHITELIST_MODULE_DIR, 0755, 0, 0);
@@ -924,24 +936,15 @@ void fs_whitelist(void) {
                 // create the link if any
                 if (entry->link) {
                         // if the link is already there, do not bother
-                        struct stat s;
-                        if (stat(entry->link, &s) != 0) {
+                        if (lstat(entry->link, &s) != 0) {
                                 // create the path if necessary
-                                mkpath(entry->link, s.st_mode);
+                                mkpath(entry->link, 0755);
 
                                 int rv = symlink(entry->data + 10, entry->link);
                                 if (rv)
                                         fprintf(stderr, "Warning cannot create symbolic link %s\n", entry->link);
                                 else if (arg_debug || arg_debug_whitelists)
                                         printf("Created symbolic link %s -> %s\n", entry->link, entry->data + 10);
-
-                                // check again for files in /tmp directory
-                                if (strncmp(entry->link, "/tmp/", 5) == 0) {
-                                        char *path = realpath(entry->link, NULL);
-                                        if (path == NULL || strncmp(path, "/tmp/", 5) != 0)
-                                                errLogExit("invalid whitelist symlink %s\n", entry->link);
-                                        free(path);
-                                }
                         }
                 }
 
diff --git a/src/firejail/util.c b/src/firejail/util.c
index eb59e36be..1d36980bb 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -1116,20 +1116,26 @@ errexit:
 // user controlled paths. Passed flags are ignored if path is a top level directory.
 int safe_fd(const char *path, int flags) {
         assert(path);
-        int fd;
+        int fd = -1;
 
         // work with a copy of path
         char *dup = strdup(path);
         if (dup == NULL)
                 errExit("strdup");
-        if (*dup != '/')
-                errExit("relative path"); // or empty string
+        // reject relative path and empty string
+        if (*dup != '/') {
+                fprintf(stderr, "Error: invalid pathname: %s\n", path);
+                exit(1);
+        }
 
         char *p = strrchr(dup, '/');
         if (p == NULL)
                 errExit("strrchr");
-        if (*(p + 1) == '\0')
-                errExit("trailing slash"); // or root dir
+        // reject trailing slash and root dir
+        if (*(p + 1) == '\0') {
+                fprintf(stderr, "Error: invalid pathname: %s\n", path);
+                exit(1);
+        }
 
         int parentfd = open("/", O_PATH|O_DIRECTORY|O_CLOEXEC);
         if (parentfd == -1)