regression: fix whitelisting of symlinks to other home dirs, small improvements

handling of home dir paths is more explicit and rigorous now, which should
make it easier to audit. Also this should come handy if one day fs_private()
supports home directories outside /home

rf. #2123

1 files changed, 27 insertions, 20 deletions

diff --git a/src/firejail/fs_whitelist.c b/src/firejail/fs_whitelist.c
index 9bedcc708..531cc4e48 100644
--- a/src/firejail/fs_whitelist.c
+++ b/src/firejail/fs_whitelist.c
@@ -41,8 +41,10 @@ static int mkpath(const char* path, mode_t mode) {
         mode |= 0111;
 
         // create directories with uid/gid as root or as current user if inside home directory
+        int userhome = 0;
         if (strncmp(path, cfg.homedir, strlen(cfg.homedir)) == 0) {
                 EUID_USER();
+                userhome = 1;
         }
 
         // work on a copy of the path
@@ -77,7 +79,9 @@ static int mkpath(const char* path, mode_t mode) {
                                         perror("mkdir");
                                 close(parentfd);
                                 free(dup);
-                                EUID_ROOT();
+                                if (userhome) {
+                                        EUID_ROOT();
+                                }
                                 return -1;
                         }
                 }
@@ -90,7 +94,9 @@ static int mkpath(const char* path, mode_t mode) {
                                 perror("open");
                         close(parentfd);
                         free(dup);
-                        EUID_ROOT();
+                        if (userhome) {
+                                EUID_ROOT();
+                        }
                         return -1;
                 }
                 // move on to next path segment
@@ -103,7 +109,9 @@ static int mkpath(const char* path, mode_t mode) {
                 fs_logger2("mkpath", path);
 
         free(dup);
-        EUID_ROOT();
+        if (userhome) {
+                EUID_ROOT();
+        }
         return fd;
 }
 
@@ -115,11 +123,12 @@ static void whitelist_path(ProfileEntry *entry) {
         char *wfile = NULL;
 
         if (entry->home_dir) {
-                if (strncmp(path, cfg.homedir, strlen(cfg.homedir)) != 0)
-                        // symlink pointing outside /home, skip the mount
+                if (strncmp(path, cfg.homedir, strlen(cfg.homedir)) != 0 || path[strlen(cfg.homedir)] != '/')
+                        // either symlink pointing outside home directory
+                        // or entire home directory, skip the mount
                         return;
 
-                fname = path + strlen(cfg.homedir);
+                fname = path + strlen(cfg.homedir) + 1; // strlen("/home/user/")
 
                 if (asprintf(&wfile, "%s/%s", RUN_WHITELIST_HOME_USER_DIR, fname) == -1)
                         errExit("asprintf");
@@ -291,13 +300,15 @@ static void whitelist_path(ProfileEntry *entry) {
         // check the last mount operation
         MountData *mptr = get_last_mount(); // will do exit(1) if the mount cannot be found
 
+        // confirm the file was mounted on the right target
+        // strcmp does not work here, because mptr->dir can be a child mount
         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");
-        // confirm the correct file is mounted on path
+        // confirm the right file was mounted
         int fd4 = safe_fd(path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
         if (fd4 == -1)
                 errExit("safe_fd");
@@ -374,7 +385,8 @@ void fs_whitelist(void) {
                 }
 
                 // remove trailing slashes and single dots
-                trim_trailing_slash_or_dot(new_name);
+                if (!nowhitelist_flag)
+                        trim_trailing_slash_or_dot(new_name);
 
                 if (arg_debug || arg_debug_whitelists)
                         fprintf(stderr, "Debug %d: new_name #%s#, %s\n", __LINE__, new_name, (nowhitelist_flag)? "nowhitelist": "whitelist");
@@ -387,7 +399,7 @@ void fs_whitelist(void) {
                 }
 
                 // extract the absolute path of the file
-                // realpath function will fail with ENOENT if the file is not found
+                // realpath function will fail with ENOENT if the file is not found or with EACCES if user has no permission
                 // special processing for /dev/fd, /dev/stdin, /dev/stdout and /dev/stderr
                 char *fname;
                 if (strcmp(new_name, "/dev/fd") == 0)
@@ -462,7 +474,7 @@ void fs_whitelist(void) {
                 }
 
                 // check for supported directories
-                if (strncmp(new_name, cfg.homedir, strlen(cfg.homedir)) == 0) {
+                if (strncmp(new_name, cfg.homedir, strlen(cfg.homedir)) == 0 && new_name[strlen(cfg.homedir)] == '/') {
                         // whitelisting home directory is disabled if --private option is present
                         if (arg_private) {
                                 if (arg_debug || arg_debug_whitelists)
@@ -479,17 +491,10 @@ void fs_whitelist(void) {
                                 fprintf(stderr, "Debug %d: fname #%s#, cfg.homedir #%s#\n",
                                         __LINE__, fname, cfg.homedir);
 
-                        // both path and absolute path are under /home
-                        if (strncmp(fname, cfg.homedir, strlen(cfg.homedir)) == 0) {
-                                // entire home directory is not allowed
-                                if (*(fname + strlen(cfg.homedir)) != '/') {
-                                        free(fname);
-                                        goto errexit;
-                                }
-                        }
-                        else {
+                        // both path and absolute path are in user home,
+                        // if not check if the symlink destination is owned by the user
+                        if (strncmp(fname, cfg.homedir, strlen(cfg.homedir)) != 0 || fname[strlen(cfg.homedir)] != '/') {
                                 if (checkcfg(CFG_FOLLOW_SYMLINK_AS_USER)) {
-                                        // check if the file is owned by the user
                                         if (stat(fname, &s) == 0 && s.st_uid != getuid()) {
                                                 free(fname);
                                                 goto errexit;
@@ -891,6 +896,7 @@ void fs_whitelist(void) {
                                         if (arg_debug || arg_debug_whitelists)
                                                 printf("Debug %d: cannot create symbolic link %s\n", __LINE__, entry->link);
                                         free(entry->link);
+                                        entry->link = NULL;
                                         entry = entry->next;
                                         continue;
                                 }
@@ -909,6 +915,7 @@ void fs_whitelist(void) {
                                 close(fd);
                         }
                         free(entry->link);
+                        entry->link = NULL;
                 }
 
                 entry = entry->next;