Merge pull request #4985 from smitsohu/whitelist

whitelist restructuring

2 files changed, 101 insertions, 79 deletions

diff --git a/src/firejail/fs_whitelist.c b/src/firejail/fs_whitelist.c
index 2acde5837..3377b2592 100644
--- a/src/firejail/fs_whitelist.c
+++ b/src/firejail/fs_whitelist.c
@@ -49,21 +49,32 @@ static void whitelist_error(const char *path) {
         exit(1);
 }
 
-static int whitelist_mkpath(const char* path, mode_t mode) {
+static int whitelist_mkpath(const char *parentdir, const char *relpath, mode_t mode) {
+        // starting from top level directory
+        int parentfd = safer_openat(-1, parentdir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        if (parentfd < 0)
+                errExit("open");
+
+        // top level directory mount id
+        int mountid = get_mount_id(parentfd);
+        if (mountid < 0) {
+                close(parentfd);
+                return -1;
+        }
+
         // work on a copy of the path
-        char *dup = strdup(path);
+        char *dup = strdup(relpath);
         if (!dup)
                 errExit("strdup");
 
         // only create leading directories, don't create the file
         char *p = strrchr(dup, '/');
-        assert(p);
+        if (!p) { // nothing to do
+                free(dup);
+                return parentfd;
+        }
         *p = '\0';
 
-        int parentfd = open("/", O_PATH|O_DIRECTORY|O_CLOEXEC);
-        if (parentfd == -1)
-                errExit("open");
-
         // traverse the path, return -1 if a symlink is encountered
         int fd = -1;
         int done = 0;
@@ -91,30 +102,50 @@ static int whitelist_mkpath(const char* path, mode_t mode) {
                         free(dup);
                         return -1;
                 }
+                // different mount id indicates earlier whitelist mount
+                if (get_mount_id(fd) != mountid) {
+                        if (arg_debug || arg_debug_whitelists)
+                                printf("Debug %d: whitelisted already\n", __LINE__);
+                        close(parentfd);
+                        close(fd);
+                        free(dup);
+                        return -1;
+                }
                 // move on to next path segment
                 close(parentfd);
                 parentfd = fd;
                 tok = strtok(NULL, "/");
         }
 
-        if (done)
-                fs_logger2("mkpath", path);
+        if (done) {
+                char *abspath;
+                if (asprintf(&abspath, "%s/%s", parentdir, relpath) < 0)
+                        errExit("asprintf");
+                fs_logger2("mkpath", abspath);
+                free(abspath);
+        }
 
         free(dup);
         return fd;
 }
 
-static void whitelist_file(int dirfd, const char *relpath, const char *path) {
+static void whitelist_file(const TopDir * const top, const char *path) {
         EUID_ASSERT();
-        assert(relpath && path);
+        assert(top && path);
+
+        // check if path is inside top level directory
+        size_t top_pathlen = strlen(top->path);
+        if (strncmp(top->path, path, top_pathlen) != 0 || path[top_pathlen] != '/')
+                return;
+        const char *relpath = path + top_pathlen + 1;
 
         // open mount source, using a file descriptor that refers to the
         // top level directory
         // as the top level directory was opened before mounting the tmpfs
         // we still have full access to all directory contents
-        // take care to not follow symbolic links (dirfd was obtained without
+        // take care to not follow symbolic links (top->fd was obtained without
         // following a link, too)
-        int fd = safer_openat(dirfd, relpath, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        int fd = safer_openat(top->fd, relpath, O_PATH|O_NOFOLLOW|O_CLOEXEC);
         if (fd == -1) {
                 if (arg_debug || arg_debug_whitelists)
                         printf("Debug %d: skip whitelist %s\n", __LINE__, path);
@@ -130,17 +161,15 @@ static void whitelist_file(int dirfd, const char *relpath, const char *path) {
                 return;
         }
 
+        // now modify the tmpfs:
         // create mount target as root, except if inside home or run/user/$UID directory
-        if ((strncmp(path, cfg.homedir, homedir_len) != 0 || path[homedir_len] != '/') &&
-            (strncmp(path, runuser, runuser_len) != 0 || path[runuser_len] != '/'))
+        if (strcmp(top->path, cfg.homedir) != 0 &&
+            strcmp(top->path, runuser) != 0)
                 EUID_ROOT();
 
         // create path of the mount target
-        int fd2 = whitelist_mkpath(path, 0755);
+        int fd2 = whitelist_mkpath(top->path, relpath, 0755);
         if (fd2 == -1) {
-                // something went wrong during path creation or a symlink was found;
-                // if there is a symlink somewhere in the path of the mount target,
-                // assume the file is whitelisted already
                 if (arg_debug || arg_debug_whitelists)
                         printf("Debug %d: skip whitelist %s\n", __LINE__, path);
                 close(fd);
@@ -149,13 +178,14 @@ static void whitelist_file(int dirfd, const char *relpath, const char *path) {
         }
 
         // get file name of the mount target
-        const char *file = gnu_basename(path);
+        const char *file = gnu_basename(relpath);
 
-        // create mount target itself and open it, a symlink is rejected
+        // create mount target itself if necessary
+        // and open it, a symlink is not allowed
         int fd3 = -1;
         if (S_ISDIR(s.st_mode)) {
-                // directory foo can exist already:
-                // firejail --whitelist=~/foo/bar --whitelist=~/foo
+                // directory bar can exist already:
+                // firejail --whitelist=/foo/bar/baz --whitelist=/foo/bar
                 if (mkdirat(fd2, file, 0755) == -1 && errno != EEXIST) {
                         if (arg_debug || arg_debug_whitelists) {
                                 perror("mkdir");
@@ -169,8 +199,8 @@ static void whitelist_file(int dirfd, const char *relpath, const char *path) {
                 fd3 = openat(fd2, file, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         }
         else
-                // create an empty file, fails with EEXIST if it is whitelisted already:
-                // firejail --whitelist=/foo --whitelist=/foo/bar
+                // create an empty file
+                // fails with EEXIST if it is whitelisted already
                 fd3 = openat(fd2, file, O_RDONLY|O_CREAT|O_EXCL|O_CLOEXEC, S_IRUSR|S_IWUSR);
 
         if (fd3 == -1) {
@@ -212,16 +242,23 @@ static void whitelist_file(int dirfd, const char *relpath, const char *path) {
         fs_logger2("whitelist", path);
 }
 
-static void whitelist_symlink(const char *link, const char *target) {
+static void whitelist_symlink(const TopDir * const top, const char *link, const char *target) {
         EUID_ASSERT();
-        assert(link && target);
+        assert(top && link && target);
+
+        // confirm link is inside top level directory
+        // this should never fail
+        size_t top_pathlen = strlen(top->path);
+        assert(strncmp(top->path, link, top_pathlen) == 0 && link[top_pathlen] == '/');
 
-        // create files as root, except if inside home or run/user/$UID directory
-        if ((strncmp(link, cfg.homedir, homedir_len) != 0 || link[homedir_len] != '/') &&
-            (strncmp(link, runuser, runuser_len) != 0 || link[runuser_len] != '/'))
+        const char *relpath = link + top_pathlen + 1;
+
+        // create link as root, except if inside home or run/user/$UID directory
+        if (strcmp(top->path, cfg.homedir) != 0 &&
+            strcmp(top->path, runuser) != 0)
                 EUID_ROOT();
 
-        int fd = whitelist_mkpath(link, 0755);
+        int fd = whitelist_mkpath(top->path, relpath, 0755);
         if (fd == -1) {
                 if (arg_debug || arg_debug_whitelists)
                         printf("Debug %d: cannot create symbolic link %s\n", __LINE__, link);
@@ -230,7 +267,7 @@ static void whitelist_symlink(const char *link, const char *target) {
         }
 
         // get file name of symlink
-        const char *file = gnu_basename(link);
+        const char *file = gnu_basename(relpath);
 
         // create the link
         if (symlinkat(target, fd, file) == -1) {
@@ -285,7 +322,7 @@ static void globbing(const char *pattern) {
 }
 
 // mount tmpfs on all top level directories
-static void tmpfs_topdirs(const TopDir *topdirs) {
+static void tmpfs_topdirs(const TopDir * const topdirs) {
         int tmpfs_home = 0;
         int tmpfs_runuser = 0;
 
@@ -329,9 +366,7 @@ static void tmpfs_topdirs(const TopDir *topdirs) {
                         fs_logger2("whitelist", RUN_FIREJAIL_DIR);
 
                         // restore /run/user/$UID directory
-                        // get path relative to /run
-                        const char *rel = runuser + 5;
-                        whitelist_file(topdirs[i].fd, rel, runuser);
+                        whitelist_file(&topdirs[i], runuser);
                 }
                 else if (strcmp(topdirs[i].path, "/tmp") == 0) {
                         // fix pam-tmpdir (#2685)
@@ -371,12 +406,7 @@ static void tmpfs_topdirs(const TopDir *topdirs) {
                 // restore user home directory if it is masked by the tmpfs
                 // creates path owned by root
                 // does nothing if user home directory doesn't exist
-                size_t topdir_len = strlen(topdirs[i].path);
-                if (strncmp(topdirs[i].path, cfg.homedir, topdir_len) == 0 && cfg.homedir[topdir_len] == '/') {
-                        // get path relative to top level directory
-                        const char *rel = cfg.homedir + topdir_len + 1;
-                        whitelist_file(topdirs[i].fd, rel, cfg.homedir);
-                }
+                whitelist_file(&topdirs[i], cfg.homedir);
         }
 
         // user home directory
@@ -720,9 +750,6 @@ void fs_whitelist(void) {
                 entry = entry->next;
         }
 
-        // release nowhitelist memory
-        free(nowhitelist);
-
         // mount tmpfs on all top level directories
         tmpfs_topdirs(topdirs);
 
@@ -732,24 +759,15 @@ void fs_whitelist(void) {
                 if (entry->wparam) {
                         char *file = entry->wparam->file;
                         char *link = entry->wparam->link;
-                        const char *topdir = entry->wparam->top->path;
-                        size_t topdir_len = strlen(topdir);
-                        int dirfd = entry->wparam->top->fd;
+                        const TopDir * const current_top = entry->wparam->top;
 
                         // top level directories of link and file can differ
-                        // whitelist the file only if it is in same top level directory
-                        if (strncmp(file, topdir, topdir_len) == 0 && file[topdir_len] == '/') {
-                                // get path relative to top level directory
-                                const char *rel = file + topdir_len + 1;
-
-                                if (arg_debug || arg_debug_whitelists)
-                                        printf("Debug %d: file: %s; dirfd: %d; topdir: %s; rel: %s\n", __LINE__, file, dirfd, topdir, rel);
-                                whitelist_file(dirfd, rel, file);
-                        }
+                        // will whitelist the file only if it is in same top level directory
+                        whitelist_file(current_top, file);
 
                         // create the link if any
                         if (link) {
-                                whitelist_symlink(link, file);
+                                whitelist_symlink(current_top, link, file);
                                 free(link);
                         }
 
@@ -762,12 +780,15 @@ void fs_whitelist(void) {
         }
 
         // release resources
-        free(runuser);
-
         size_t i;
+        for (i = 0; i < nowhitelist_c; i++)
+                free(nowhitelist[i]);
+        free(nowhitelist);
+
         for (i = 0; i < TOP_MAX && topdirs[i].path; i++) {
                 free(topdirs[i].path);
                 close(topdirs[i].fd);
         }
         free(topdirs);
+        free(runuser);
 }
diff --git a/src/firejail/mountinfo.c b/src/firejail/mountinfo.c
index 7d30d21d9..56c0bda30 100644
--- a/src/firejail/mountinfo.c
+++ b/src/firejail/mountinfo.c
@@ -146,11 +146,10 @@ MountData *get_last_mount(void) {
 
 // Returns mount id, or -1 if fd refers to a procfs or sysfs file
 static int get_mount_id_from_handle(int fd) {
-        EUID_ASSERT();
-
         char *proc;
         if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
                 errExit("asprintf");
+
         struct file_handle *fh = malloc(sizeof *fh);
         if (!fh)
                 errExit("malloc");
@@ -172,40 +171,42 @@ static int get_mount_id_from_handle(int fd) {
 
 // Returns mount id, or -1 on kernels < 3.15
 static int get_mount_id_from_fdinfo(int fd) {
-        EUID_ASSERT();
-        int rv = -1;
-
         char *proc;
         if (asprintf(&proc, "/proc/self/fdinfo/%d", fd) == -1)
                 errExit("asprintf");
-        EUID_ROOT();
+
+        int called_as_root = 0;
+        if (geteuid() == 0)
+                called_as_root = 1;
+
+        if (called_as_root == 0)
+                EUID_ROOT();
+
         FILE *fp = fopen(proc, "re");
-        EUID_USER();
-        if (!fp)
-                goto errexit;
+        if (!fp) {
+                fprintf(stderr, "Error: cannot read proc file\n");
+                exit(1);
+        }
 
+        if (called_as_root == 0)
+                EUID_USER();
+
+        int rv = -1;
         char buf[MAX_BUF];
         while (fgets(buf, MAX_BUF, fp)) {
-                if (strncmp(buf, "mnt_id:", 7) == 0) {
-                        if (sscanf(buf + 7, "%d", &rv) == 1)
+                if (sscanf(buf, "mnt_id: %d", &rv) == 1)
                                 break;
-                        goto errexit;
-                }
         }
 
         free(proc);
         fclose(fp);
         return rv;
-
-errexit:
-        fprintf(stderr, "Error: cannot read proc file\n");
-        exit(1);
 }
 
 int get_mount_id(int fd) {
-        int rv = get_mount_id_from_fdinfo(fd);
+        int rv = get_mount_id_from_handle(fd);
         if (rv < 0)
-                rv = get_mount_id_from_handle(fd);
+                rv = get_mount_id_from_fdinfo(fd);
         return rv;
 }