simplify X11 socket whitelisting

2 files changed, 22 insertions, 45 deletions

diff --git a/src/firejail/x11.c b/src/firejail/x11.c
index 257d376a1..f4f093138 100644
--- a/src/firejail/x11.c
+++ b/src/firejail/x11.c
@@ -1327,7 +1327,7 @@ void fs_x11(void) {
         struct stat s1, s2;
         if (stat("/tmp", &s1) != 0 || lstat("/tmp/.X11-unix", &s2) != 0)
                 return;
-        if ((s1.st_mode & S_ISVTX) == 0) {
+        if ((s1.st_mode & S_ISVTX) != S_ISVTX) {
                 fwarning("cannot mask X11 sockets: sticky bit not set on /tmp directory\n");
                 return;
         }
@@ -1335,26 +1335,26 @@ void fs_x11(void) {
                 fwarning("cannot mask X11 sockets: /tmp/.X11-unix not owned by root user\n");
                 return;
         }
+
         char *x11file;
         if (asprintf(&x11file, "/tmp/.X11-unix/X%d", display) == -1)
                 errExit("asprintf");
+        int src = open(x11file, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        if (src < 0) {
+                free(x11file);
+                return;
+        }
         struct stat x11stat;
-        if (lstat(x11file, &x11stat) != 0 || !S_ISSOCK(x11stat.st_mode)) {
+        if (fstat(src, &x11stat) < 0)
+                errExit("fstat");
+        if (!S_ISSOCK(x11stat.st_mode)) {
+                close(src);
                 free(x11file);
                 return;
         }
 
         if (arg_debug || arg_debug_whitelists)
                 fprintf(stderr, "Masking all X11 sockets except %s\n", x11file);
-
-        // Move the real /tmp/.X11-unix to a scratch location
-        // so we can still access x11file after we mount a
-        // tmpfs over /tmp/.X11-unix.
-        if (mkdir(RUN_WHITELIST_X11_DIR, 0700) == -1)
-                errExit("mkdir");
-        if (mount("/tmp/.X11-unix", RUN_WHITELIST_X11_DIR, 0, MS_BIND|MS_REC, 0) < 0)
-                errExit("mount bind");
-
         // This directory must be mode 1777
         if (mount("tmpfs", "/tmp/.X11-unix", "tmpfs",
                 MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_STRICTATIME,
@@ -1363,40 +1363,21 @@ void fs_x11(void) {
         fs_logger("tmpfs /tmp/.X11-unix");
 
         // create an empty root-owned file which will have the desired socket bind-mounted over it
-        int fd = open(x11file, O_RDONLY|O_CREAT|O_EXCL|O_CLOEXEC, S_IRUSR | S_IWUSR);
-        if (fd < 0)
-                errExit(x11file);
-        close(fd);
+        int dst = open(x11file, O_RDONLY|O_CREAT|O_EXCL|O_CLOEXEC, S_IRUSR | S_IWUSR);
+        if (dst < 0)
+                errExit("open");
 
-        // the mount source is under control of the user, so be careful and
-        // mount without following symbolic links, using a file descriptor
-        char *wx11file;
-        if (asprintf(&wx11file, "%s/X%d", RUN_WHITELIST_X11_DIR, display) == -1)
-                errExit("asprintf");
-        fd = safer_openat(-1, wx11file, O_PATH|O_NOFOLLOW|O_CLOEXEC);
-        if (fd == -1)
-                errExit("opening X11 socket");
-        // confirm once more we are mounting a socket
-        if (fstat(fd, &x11stat) == -1)
-                errExit("fstat");
-        if (!S_ISSOCK(x11stat.st_mode)) {
-                errno = ENOTSOCK;
-                errExit("mounting X11 socket");
-        }
-        char *proc;
-        if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
+        char *proc_src, *proc_dst;
+        if (asprintf(&proc_src, "/proc/self/fd/%d", src) == -1 ||
+            asprintf(&proc_dst, "/proc/self/fd/%d", dst) == -1)
                 errExit("asprintf");
-        if (mount(proc, x11file, NULL, MS_BIND|MS_REC, NULL) < 0)
+        if (mount(proc_src, proc_dst, NULL, MS_BIND | MS_REC, NULL) < 0)
                 errExit("mount bind");
+        free(proc_src);
+        free(proc_dst);
+        close(src);
+        close(dst);
         fs_logger2("whitelist", x11file);
-        close(fd);
-        free(proc);
-
-        // block access to RUN_WHITELIST_X11_DIR
-        if (mount(RUN_RO_DIR, RUN_WHITELIST_X11_DIR, 0, MS_BIND, 0) < 0)
-                errExit("mount");
-        fs_logger2("blacklist", RUN_WHITELIST_X11_DIR);
-        free(wx11file);
         free(x11file);
 #endif
 }
diff --git a/src/include/rundefs.h b/src/include/rundefs.h
index a172dd511..3db750da3 100644
--- a/src/include/rundefs.h
+++ b/src/include/rundefs.h
@@ -79,12 +79,8 @@
 #define PATH_SECCOMP_MDWX_32            LIBDIR "/firejail/seccomp.mdwx.32"
 #define PATH_SECCOMP_BLOCK_SECONDARY    LIBDIR "/firejail/seccomp.block_secondary"      // secondary arch blocking filter built during make
 
-
 #define RUN_DEV_DIR                     RUN_MNT_DIR "/dev"
 #define RUN_DEVLOG_FILE                 RUN_MNT_DIR "/devlog"
-
-#define RUN_WHITELIST_X11_DIR           RUN_MNT_DIR "/orig-x11"
-
 #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