follow-up

PR #4349

5 files changed, 57 insertions, 56 deletions

diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index 10133142a..c442a97bf 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -45,6 +45,15 @@
                 assert(s.st_gid == gid);\
                 assert((s.st_mode & 07777) == (mode));\
         } while (0)
+#define ASSERT_PERMS_AS_USER(file, uid, gid, mode) \
+        do { \
+                assert(file);\
+                struct stat s;\
+                if (stat_as_user(file, &s) == -1) errExit("stat");\
+                assert(s.st_uid == uid);\
+                assert(s.st_gid == gid);\
+                assert((s.st_mode & 07777) == (mode));\
+        } while (0)
 #define ASSERT_PERMS_FD(fd, uid, gid, mode) \
         do { \
                 struct stat s;\
diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index 01182bd2c..bf78f8a17 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -77,7 +77,7 @@ static void disable_file(OPERATION op, const char *filename) {
 
                 EUID_ROOT();
                 int err = bind_mount_path_to_fd(RUN_RO_DIR, fd);
-                if (err < 0)
+                if (err != 0)
                         err = bind_mount_path_to_fd(RUN_RO_FILE, fd);
                 EUID_USER();
                 close(fd);
@@ -655,8 +655,13 @@ static void fs_remount_rec(const char *dir, OPERATION op) {
 // resolve a path and remount it
 void fs_remount(const char *path, OPERATION op, int rec) {
         assert(path);
-        assert(geteuid() == 0);
-        EUID_USER();
+
+        int called_as_root = 0;
+        if (geteuid() == 0)
+                called_as_root = 1;
+
+        if (called_as_root)
+                EUID_USER();
 
         char *rpath = realpath(path, NULL);
         if (rpath) {
@@ -666,7 +671,9 @@ void fs_remount(const char *path, OPERATION op, int rec) {
                         fs_remount_simple(rpath, op);
                 free(rpath);
         }
-        EUID_ROOT();
+
+        if (called_as_root)
+                EUID_ROOT();
 }
 
 // Disable /mnt, /media, /run/mount and /run/media access
@@ -821,7 +828,6 @@ void disable_config(void) {
 
 
 // build a basic read-only filesystem
-// top level directories could be links, run no after-mount checks
 void fs_basic_fs(void) {
         uid_t uid = getuid();
 
@@ -831,6 +837,7 @@ void fs_basic_fs(void) {
         if (mount("proc", "/proc", "proc", MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_REC, NULL) < 0)
                 errExit("mounting /proc");
 
+        EUID_USER();
         if (arg_debug)
                 printf("Basic read-only filesystem:\n");
         if (!arg_writable_etc) {
@@ -850,6 +857,7 @@ void fs_basic_fs(void) {
         fs_remount("/lib64", MOUNT_READONLY, 1);
         fs_remount("/lib32", MOUNT_READONLY, 1);
         fs_remount("/libx32", MOUNT_READONLY, 1);
+        EUID_ROOT();
 
         // update /var directory in order to support multiple sandboxes running on the same root directory
         fs_var_lock();
diff --git a/src/firejail/pulseaudio.c b/src/firejail/pulseaudio.c
index be0f5aea6..f8d4c2f3c 100644
--- a/src/firejail/pulseaudio.c
+++ b/src/firejail/pulseaudio.c
@@ -75,31 +75,34 @@ void pulseaudio_disable(void) {
         closedir(dir);
 }
 
-static void pulseaudio_fallback(const char *path) {
-        assert(path);
-
-        fmessage("Cannot mount tmpfs on %s/.config/pulse\n", cfg.homedir);
-        env_store_name_val("PULSE_CLIENTCONFIG", path, SETENV);
-}
-
 // disable shm in pulseaudio (issue #69)
 void pulseaudio_init(void) {
-        struct stat s;
-
         // do we have pulseaudio in the system?
-        if (stat(PULSE_CLIENT_SYSCONF, &s) == -1) {
+        if (access(PULSE_CLIENT_SYSCONF, R_OK)) {
                 if (arg_debug)
-                        printf("%s not found\n", PULSE_CLIENT_SYSCONF);
+                        printf("Cannot read %s\n", PULSE_CLIENT_SYSCONF);
                 return;
         }
 
+        // create ~/.config/pulse directory if not present
+        char *homeusercfg = NULL;
+        if (asprintf(&homeusercfg, "%s/.config", cfg.homedir) == -1)
+                errExit("asprintf");
+        if (create_empty_dir_as_user(homeusercfg, 0700))
+                fs_logger2("create", homeusercfg);
+
+        free(homeusercfg);
+        if (asprintf(&homeusercfg, "%s/.config/pulse", cfg.homedir) == -1)
+                errExit("asprintf");
+        if (create_empty_dir_as_user(homeusercfg, 0700))
+                fs_logger2("create", homeusercfg);
+
         // create the new user pulseaudio directory
+        // that will be mounted over ~/.config/pulse
         if (mkdir(RUN_PULSE_DIR, 0700) == -1)
                 errExit("mkdir");
-        selinux_relabel_path(RUN_PULSE_DIR, RUN_PULSE_DIR);
-        // mount it nosuid, noexec, nodev
+        selinux_relabel_path(RUN_PULSE_DIR, homeusercfg);
         fs_remount(RUN_PULSE_DIR, MOUNT_NOEXEC, 0);
-
         // create the new client.conf file
         char *pulsecfg = NULL;
         if (asprintf(&pulsecfg, "%s/client.conf", RUN_PULSE_DIR) == -1)
@@ -116,37 +119,14 @@ void pulseaudio_init(void) {
         if (set_perms(RUN_PULSE_DIR, getuid(), getgid(), 0700))
                 errExit("set_perms");
 
-        // create ~/.config/pulse directory if not present
-        char *homeusercfg = NULL;
-        if (asprintf(&homeusercfg, "%s/.config", cfg.homedir) == -1)
-                errExit("asprintf");
-        if (create_empty_dir_as_user(homeusercfg, 0700))
-                fs_logger2("create", homeusercfg);
-
-        free(homeusercfg);
-        if (asprintf(&homeusercfg, "%s/.config/pulse", cfg.homedir) == -1)
-                errExit("asprintf");
-        if (create_empty_dir_as_user(homeusercfg, 0700))
-                fs_logger2("create", homeusercfg);
-
         // if ~/.config/pulse exists and there are no symbolic links, mount the new directory
         // else set environment variable
+        EUID_USER();
         int fd = safer_openat(-1, homeusercfg, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        EUID_ROOT();
         if (fd == -1) {
-                pulseaudio_fallback(pulsecfg);
-                goto out;
-        }
-        // confirm the actual mount destination is owned by the user
-        if (fstat(fd, &s) == -1) { // FUSE
-                if (errno != EACCES)
-                        errExit("fstat");
-                close(fd);
-                pulseaudio_fallback(pulsecfg);
-                goto out;
-        }
-        if (s.st_uid != getuid()) {
-                close(fd);
-                pulseaudio_fallback(pulsecfg);
+                fwarning("not mounting tmpfs on %s\n", homeusercfg);
+                env_store_name_val("PULSE_CLIENTCONFIG", pulsecfg, SETENV);
                 goto out;
         }
         // preserve a read-only mount
diff --git a/src/firejail/util.c b/src/firejail/util.c
index b8643ff60..edd08bb41 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -981,7 +981,7 @@ int remove_overlay_directory(void) {
                         int fd = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
                         if (fd == -1) {
                                 fprintf(stderr, "Error: cannot open %s\n", path);
-                                _exit(1);
+                                exit(1);
                         }
                         struct stat s;
                         if (fstat(fd, &s) == -1)
@@ -991,11 +991,11 @@ int remove_overlay_directory(void) {
                                         fprintf(stderr, "Error: %s is a symbolic link\n", path);
                                 else
                                         fprintf(stderr, "Error: %s is not a directory\n", path);
-                                _exit(1);
+                                exit(1);
                         }
                         if (s.st_uid != getuid()) {
                                 fprintf(stderr, "Error: %s is not owned by the current user\n", path);
-                                _exit(1);
+                                exit(1);
                         }
                         // chdir to ~/.firejail
                         if (fchdir(fd) == -1)
@@ -1187,7 +1187,6 @@ unsigned extract_timeout(const char *str) {
 
 void disable_file_or_dir(const char *fname) {
         assert(fname);
-        assert(geteuid() == 0);
 
         EUID_USER();
         int fd = open(fname, O_PATH|O_CLOEXEC);
@@ -1207,7 +1206,7 @@ void disable_file_or_dir(const char *fname) {
                 printf("blacklist %s\n", fname);
         if (S_ISDIR(s.st_mode)) {
                 if (bind_mount_path_to_fd(RUN_RO_DIR, fd) < 0)
-                                errExit("disable directory");
+                        errExit("disable directory");
         }
         else {
                 if (bind_mount_path_to_fd(RUN_RO_FILE, fd) < 0)
diff --git a/src/firejail/x11.c b/src/firejail/x11.c
index 09956b903..0619ff380 100644
--- a/src/firejail/x11.c
+++ b/src/firejail/x11.c
@@ -1204,14 +1204,13 @@ void x11_xorg(void) {
         fmessage("Generating a new .Xauthority file\n");
         mkdir_attr(RUN_XAUTHORITY_SEC_DIR, 0700, getuid(), getgid());
         // create new Xauthority file in RUN_XAUTHORITY_SEC_DIR
+        EUID_USER();
         char tmpfname[] = RUN_XAUTHORITY_SEC_DIR "/.Xauth-XXXXXX";
         int fd = mkstemp(tmpfname);
         if (fd == -1) {
                 fprintf(stderr, "Error: cannot create .Xauthority file\n");
                 exit(1);
         }
-        if (fchown(fd, getuid(), getgid()) == -1)
-                errExit("chown");
         close(fd);
 
         // run xauth
@@ -1221,8 +1220,6 @@ void x11_xorg(void) {
         else
                 sbox_run(SBOX_USER | SBOX_CAPS_NONE | SBOX_SECCOMP, 7, RUN_XAUTH_FILE, "-f", tmpfname,
                         "generate", display, "MIT-MAGIC-COOKIE-1", "untrusted");
-        // remove xauth copy
-        unlink(RUN_XAUTH_FILE);
 
         // ensure there is already a file ~/.Xauthority, so that bind-mount below will work.
         char *dest;
@@ -1273,10 +1270,12 @@ void x11_xorg(void) {
         // mount via the link in /proc/self/fd
         if (arg_debug)
                 printf("Mounting %s on %s\n", tmpfname, dest);
+        EUID_ROOT();
         if (bind_mount_by_fd(src, dst)) {
                 fprintf(stderr, "Error: cannot mount the new .Xauthority file\n");
                 exit(1);
         }
+        EUID_USER();
         // check /proc/self/mountinfo to confirm the mount is ok
         MountData *mptr = get_last_mount();
         if (strcmp(mptr->dir, dest) != 0 || strcmp(mptr->fstype, "tmpfs") != 0)
@@ -1289,9 +1288,10 @@ void x11_xorg(void) {
         // blacklist user .Xauthority file if it is not masked already
         const char *envar = env_get("XAUTHORITY");
         if (envar) {
-                char *rp = realpath_as_user(envar);
+                char *rp = realpath(envar, NULL);
                 if (rp) {
                         if (strcmp(rp, dest) != 0)
+                                // disable_file_or_dir returns with EUID 0
                                 disable_file_or_dir(rp);
                         free(rp);
                 }
@@ -1301,9 +1301,13 @@ void x11_xorg(void) {
         free(dest);
 
         // mask RUN_XAUTHORITY_SEC_DIR
+        EUID_ROOT();
         if (mount("tmpfs", RUN_XAUTHORITY_SEC_DIR, "tmpfs", MS_NOSUID | MS_NODEV | MS_STRICTATIME,  "mode=755,gid=0") < 0)
                 errExit("mounting tmpfs");
         fs_logger2("tmpfs", RUN_XAUTHORITY_SEC_DIR);
+
+        // cleanup
+        unlink(RUN_XAUTH_FILE);
 #endif
 }
 
@@ -1352,6 +1356,7 @@ void fs_x11(void) {
                 MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_STRICTATIME,
                 "mode=1777,uid=0,gid=0") < 0)
                 errExit("mounting tmpfs on /tmp/.X11-unix");
+        selinux_relabel_path("/tmp/.X11-unix", "/tmp/.X11-unix");
         fs_logger("tmpfs /tmp/.X11-unix");
 
         // create an empty root-owned file which will have the desired socket bind-mounted over it