various x11 xorg enhancements

1) copy xauth binary into the sandbox and set mode to 0711, so it runs
with cleared dumpable flag for unprivileged users

2) run xauth in an sbox sandbox

3) generate Xauthority file in runtime directory instead of /tmp;
this way xauth is able to connect to the X11 socket even if the
abstract socket doesn't exist, for example because a new network
namespace was instantiated

2 files changed, 72 insertions, 106 deletions

diff --git a/src/firejail/x11.c b/src/firejail/x11.c
index 98ac184d9..1ecb688af 100644
--- a/src/firejail/x11.c
+++ b/src/firejail/x11.c
@@ -34,7 +34,7 @@
 
 #include <fcntl.h>
 #ifndef O_PATH
-# define O_PATH 010000000
+#define O_PATH 010000000
 #endif
 
 
@@ -1129,28 +1129,17 @@ void x11_start(int argc, char **argv) {
 }
 #endif
 
-// Porting notes:
-//
-// 1. merge #1100 from zackw:
-//     Attempting to run xauth -f directly on a file in /run/firejail/mnt/ directory fails on Debian 8
-//     with this message:
-//            xauth:  timeout in locking authority file /run/firejail/mnt/sec.Xauthority-Qt5Mu4
-//            Failed to create untrusted X cookie: xauth: exit 1
-//     For this reason we run xauth on a file in a tmpfs filesystem mounted on /tmp. This was
-//     a partial merge.
-//
-// 2. Since we cannot deal with the TOCTOU condition when mounting .Xauthority in user home
-// directory, we need to make sure /usr/bin/xauth executable is the real thing, and not
-// something picked up on $PATH.
-//
-// 3. If for any reason xauth command fails, we exit the sandbox. On Debian 8 this happens
-// when using a network namespace. Somehow, xauth tries to connect to the abstract socket,
-// and it fails because of the network namespace - it should try to connect to the regular
-// Unix socket! If we ignore the fail condition, the program will be started on X server without
-// the security extension loaded.
+
 void x11_xorg(void) {
 #ifdef HAVE_X11
 
+        // get DISPLAY env
+        char *display = getenv("DISPLAY");
+        if (!display) {
+                fputs("Error: --x11=xorg requires an 'outer' X11 server to use.\n", stderr);
+                exit(1);
+        }
+
         // check xauth utility is present in the system
         struct stat s;
         if (stat("/usr/bin/xauth", &s) == -1) {
@@ -1160,26 +1149,27 @@ void x11_xorg(void) {
                 fprintf(stderr, "   Fedora: sudo dnf install xorg-x11-xauth\n");
                 exit(1);
         }
-        if (s.st_uid != 0 && s.st_gid != 0) {
+        if ((s.st_uid != 0 && s.st_gid != 0) || (s.st_mode & S_IWOTH)) {
                 fprintf(stderr, "Error: invalid /usr/bin/xauth executable\n");
                 exit(1);
         }
-
-        // get DISPLAY env
-        char *display = getenv("DISPLAY");
-        if (!display) {
-                fputs("Error: --x11=xorg requires an 'outer' X11 server to use.\n", stderr);
+        if (s.st_size > 1024 * 1024) {
+                fprintf(stderr, "Error: /usr/bin/xauth executable is too large\n");
                 exit(1);
         }
-
-        // temporarily mount a tempfs on top of /tmp directory
-        if (mount("tmpfs", "/tmp", "tmpfs", MS_NOSUID | MS_STRICTATIME,  "mode=1777,gid=0") < 0)
-                errExit("mounting /tmp");
-
-        // create the temporary .Xauthority file
+        // copy /usr/bin/xauth in the sandbox and set mode to 0711
+        // users are not able to trace the running xauth this way
         if (arg_debug)
-                printf("Generating a new .Xauthority file\n");
-        char tmpfname[] = "/tmp/.tmpXauth-XXXXXX";
+                printf("Copying /usr/bin/xauth to %s\n", RUN_XAUTH_FILE);
+        if (copy_file("/usr/bin/xauth", RUN_XAUTH_FILE, 0, 0, 0711)) {
+                fprintf(stderr, "Error: cannot copy /usr/bin/xauth executable\n");
+                exit(1);
+        }
+
+        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
+        char tmpfname[] = RUN_XAUTHORITY_SEC_DIR "/.Xauth-XXXXXX";
         int fd = mkstemp(tmpfname);
         if (fd == -1) {
                 fprintf(stderr, "Error: cannot create .Xauthority file\n");
@@ -1189,64 +1179,17 @@ void x11_xorg(void) {
                 errExit("chown");
         close(fd);
 
-        pid_t child = fork();
-        if (child < 0)
-                errExit("fork");
-        if (child == 0) {
-                drop_privs(1);
-                clearenv();
-#ifdef HAVE_GCOV
-                __gcov_flush();
-#endif
-                if (arg_debug) {
-                        execlp("/usr/bin/xauth", "/usr/bin/xauth", "-v", "-f", tmpfname,
-                        "generate", display, "MIT-MAGIC-COOKIE-1", "untrusted", NULL);
-                }
-                else {
-                        execlp("/usr/bin/xauth", "/usr/bin/xauth", "-f", tmpfname,
-                        "generate", display, "MIT-MAGIC-COOKIE-1", "untrusted", NULL);
-                }
-
-                _exit(127);
-        }
-
-        // wait for the xauth process to finish
-        int status;
-        if (waitpid(child, &status, 0) != child)
-                errExit("waitpid");
-        if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
-                /* success */
-        }
-        else if (WIFEXITED(status)) {
-                fprintf(stderr, "Failed to create untrusted X cookie: xauth: exit %d\n",
-                        WEXITSTATUS(status));
-                exit(1);
-        }
-        else if (WIFSIGNALED(status)) {
-                fprintf(stderr, "Failed to create untrusted X cookie: xauth: %s\n",
-                        strsignal(WTERMSIG(status)));
-                exit(1);
-        }
-        else {
-                fprintf(stderr, "Failed to create untrusted X cookie: "
-                        "xauth: un-decodable exit status %04x\n", status);
-                exit(1);
-        }
-
-        // move the temporary file in RUN_XAUTHORITY_SEC_FILE in order to have it deleted
-        // automatically when the sandbox is closed (rename doesn't work)
+        // run xauth
         if (arg_debug)
-                printf("Copying the new .Xauthority file\n");
-        copy_file_from_user_to_root(tmpfname, RUN_XAUTHORITY_SEC_FILE, getuid(), getgid(), 0600);
-        
-        /* coverity[toctou] */
-        unlink(tmpfname);
-        umount("/tmp");
-
-        // mount RUN_XAUTHORITY_SEC_FILE noexec, nodev, nosuid
-        fs_remount(RUN_XAUTHORITY_SEC_FILE, MOUNT_NOEXEC, 0);
+                sbox_run(SBOX_USER | SBOX_CAPS_NONE | SBOX_SECCOMP, 8, RUN_XAUTH_FILE, "-v", "-f", tmpfname,
+                        "generate", display, "MIT-MAGIC-COOKIE-1", "untrusted");
+        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 in the usual location, so that bind-mount below will work.
+        // ensure there is already a file ~/.Xauthority, so that bind-mount below will work.
         char *dest;
         if (asprintf(&dest, "%s/.Xauthority", cfg.homedir) == -1)
                 errExit("asprintf");
@@ -1257,13 +1200,12 @@ void x11_xorg(void) {
                         exit(1);
                 }
         }
-
-        // get a file descriptor for .Xauthority
-        fd = safe_fd(dest, O_PATH|O_NOFOLLOW|O_CLOEXEC);
-        if (fd == -1)
+        // get a file descriptor for ~/.Xauthority
+        int dst = safe_fd(dest, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        if (dst == -1)
                 errExit("safe_fd");
         // check if the actual mount destination is a user owned regular file
-        if (fstat(fd, &s) == -1)
+        if (fstat(dst, &s) == -1)
                 errExit("fstat");
         if (!S_ISREG(s.st_mode) || s.st_uid != getuid()) {
                 if (S_ISLNK(s.st_mode))
@@ -1274,31 +1216,49 @@ void x11_xorg(void) {
         }
         // preserve a read-only mount
         struct statvfs vfs;
-        if (fstatvfs(fd, &vfs) == -1)
+        if (fstatvfs(dst, &vfs) == -1)
                 errExit("fstatvfs");
         if ((vfs.f_flag & MS_RDONLY) == MS_RDONLY)
-                fs_remount(RUN_XAUTHORITY_SEC_FILE, MOUNT_READONLY, 0);
+                fs_remount(RUN_XAUTHORITY_SEC_DIR, MOUNT_READONLY, 0);
+
+        // always mounting the new Xauthority file noexec,nodev,nosuid
+        fs_remount(RUN_XAUTHORITY_SEC_DIR, MOUNT_NOEXEC, 0);
+
+        // get a file descriptor for the new Xauthority file
+        int src = safe_fd(tmpfname, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        if (src == -1)
+                errExit("safe_fd");
+        if (fstat(src, &s) == -1)
+                errExit("fstat");
+        if (!S_ISREG(s.st_mode)) {
+                errno = EPERM;
+                errExit("mounting Xauthority file");
+        }
 
         // mount via the link in /proc/self/fd
         if (arg_debug)
-                printf("Mounting %s on %s\n", RUN_XAUTHORITY_SEC_FILE, dest);
-        char *proc;
-        if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
+                printf("Mounting %s on %s\n", tmpfname, dest);
+        char *proc_src, *proc_dst;
+        if (asprintf(&proc_src, "/proc/self/fd/%d", src) == -1)
+                errExit("asprintf");
+        if (asprintf(&proc_dst, "/proc/self/fd/%d", dst) == -1)
                 errExit("asprintf");
-        if (mount(RUN_XAUTHORITY_SEC_FILE, proc, "none", MS_BIND, "mode=0600") == -1) {
+        if (mount(proc_src, proc_dst, NULL, MS_BIND, NULL) == -1) {
                 fprintf(stderr, "Error: cannot mount the new .Xauthority file\n");
                 exit(1);
         }
-        free(proc);
-        close(fd);
         // 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)
                 errLogExit("invalid .Xauthority mount");
+        free(proc_src);
+        free(proc_dst);
+        close(src);
+        close(dst);
 
         ASSERT_PERMS(dest, getuid(), getgid(), 0600);
 
-        // blacklist .Xauthority file if it is not masked already
+        // blacklist user .Xauthority file if it is not masked already
         char *envar = getenv("XAUTHORITY");
         if (envar) {
                 char *rp = realpath(envar, NULL);
@@ -1312,6 +1272,11 @@ void x11_xorg(void) {
         if (setenv("XAUTHORITY", dest, 1) < 0)
                 errExit("setenv");
         free(dest);
+
+        // mask RUN_XAUTHORITY_SEC_DIR
+        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);
 #endif
 }
 
diff --git a/src/include/rundefs.h b/src/include/rundefs.h
index f8bcdec52..d56623907 100644
--- a/src/include/rundefs.h
+++ b/src/include/rundefs.h
@@ -99,8 +99,9 @@
 #define RUN_WHITELIST_SHARE_DIR         RUN_MNT_DIR "/orig-share"
 #define RUN_WHITELIST_MODULE_DIR        RUN_MNT_DIR "/orig-module"
 
-#define RUN_XAUTHORITY_FILE             RUN_MNT_DIR "/.Xauthority"
-#define RUN_XAUTHORITY_SEC_FILE         RUN_MNT_DIR "/sec.Xauthority"
+#define RUN_XAUTHORITY_FILE             RUN_MNT_DIR "/.Xauthority"              // private options
+#define RUN_XAUTH_FILE                  RUN_MNT_DIR "/xauth"                    // x11=xorg
+#define RUN_XAUTHORITY_SEC_DIR          RUN_MNT_DIR "/.sec.Xauthority"          // x11=xorg
 #define RUN_ASOUNDRC_FILE               RUN_MNT_DIR "/.asoundrc"
 #define RUN_HOSTNAME_FILE               RUN_MNT_DIR "/hostname"
 #define RUN_HOSTS_FILE                  RUN_MNT_DIR "/hosts"