merge #1100 from zackw: rework DISPLAY environment parsing, rework masking X11 sockets in /tmp/.X11-unix directory

2 files changed, 95 insertions, 16 deletions

diff --git a/README b/README
index 617ec3ec8..52c5f7dd2 100644
--- a/README
+++ b/README
@@ -103,6 +103,8 @@ valoq (https://github.com/valoq)
         - added iridium browser profile
 Zack Weinberg (https://github.com/zackw)
         - removed libconnect
+        - fixed memory corruption in noblacklist processing
+        - rework DISPLAY environment parsing, rework masking X11 sockets in /tmp/.X11-unix directory
 Igor Bukanov (https://github.com/ibukanov)
         - found/fiixed privilege escalation in --hosts-file option
 Cat (https://github.com/ecat3)
diff --git a/src/firejail/x11.c b/src/firejail/x11.c
index 2fedcc355..b551a2d2a 100644
--- a/src/firejail/x11.c
+++ b/src/firejail/x11.c
@@ -27,6 +27,8 @@
 #include <dirent.h>
 #include <sys/mount.h>
 #include <sys/wait.h>
+#include <errno.h>
+#include <limits.h>
 int mask_x11_abstract_socket = 0;
 
 #ifdef HAVE_X11
@@ -103,21 +105,44 @@ static int random_display_number(void) {
 }
 #endif
 
-// return display number, -1 if not configured
+
+// Parse the DISPLAY environment variable and return a display number.
+// Returns -1 if DISPLAY is not set, or is set to anything other than :ddd.
 int x11_display(void) {
-        // extract display
-        char *d = getenv("DISPLAY");
-        if (!d)
-                return - 1;
-        
-        int display;
-        int rv = sscanf(d, ":%d", &display);
-        if (rv != 1)
-                return -1;
-        if (arg_debug)
-                printf("DISPLAY %s, %d\n", d, display);
-        
-        return display;
+        const char *display_str = getenv("DISPLAY");
+        char *endp;
+        unsigned long display;
+
+        if (!display_str) {
+                if (arg_debug)
+                        fputs("DISPLAY is not set\n", stderr);
+                return -1;
+        }
+
+        if (display_str[0] != ':' || display_str[1] < '0' || display_str[1] > '9') {
+                if (arg_debug)
+                        fprintf(stderr, "unsupported DISPLAY form '%s'\n", display_str);
+                return -1;
+        }
+
+        errno = 0;
+        display = strtoul(display_str+1, &endp, 10);
+        if (endp == display_str+1 || (*endp != '\0' && *endp != '.')) { // handling DISPLAY=:0 and also :0.0
+                if (arg_debug)
+                        fprintf(stderr, "unsupported DISPLAY form '%s'\n", display_str);
+                return -1;
+        }
+        if (errno || display > (unsigned long)INT_MAX) {
+                if (arg_debug)
+                        fprintf(stderr, "display number %s is outside the valid range\n",
+                                display_str+1);
+                return -1;
+        }
+
+        if (arg_debug)
+                fprintf(stderr, "DISPLAY=%s parsed as %lu\n", display_str, display);
+
+        return (int)display;
 }
 
 void fs_x11(void) {
@@ -129,10 +154,60 @@ void fs_x11(void) {
         char *x11file;
         if (asprintf(&x11file, "/tmp/.X11-unix/X%d", display) == -1)
                 errExit("asprintf");
-        struct stat s;
-        if (stat(x11file, &s) == -1)
+        struct stat x11stat;
+        if (stat(x11file, &x11stat) == -1 || !S_ISSOCK(x11stat.st_mode)) {
+                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.
+                int rv = mkdir(RUN_WHITELIST_X11_DIR, 0700);
+                if (rv == -1)
+                        errExit("mkdir");
+                if (set_perms(RUN_WHITELIST_X11_DIR, 0, 0, 0700))
+                        errExit("set_perms");
+
+                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, or Xlib will barf.
+                if (mount("tmpfs", "/tmp/.X11-unix", "tmpfs",
+                          MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_STRICTATIME | MS_REC,
+                          "mode=1777,uid=0,gid=0") < 0)
+                        errExit("mounting tmpfs on /tmp/.X11-unix");
+                fs_logger("tmpfs /tmp/.X11-unix");
+
+                // create an empty file which will have the desired socket bind-mounted over it
+                int fd = open(x11file, O_RDWR|O_CREAT|O_EXCL, x11stat.st_mode & ~S_IFMT);
+                if (fd < 0)
+                        errExit(x11file);
+                if (fchown(fd, x11stat.st_uid, x11stat.st_gid))
+                        errExit("fchown");
+                close(fd);
+
+                // do the mount
+                char *wx11file;
+                if (asprintf(&wx11file, "%s/X%d", RUN_WHITELIST_X11_DIR, display) == -1)
+                        errExit("asprintf");
+                if (mount(wx11file, x11file, NULL, MS_BIND|MS_REC, NULL) < 0)
+                        errExit("mount bind");
+                fs_logger2("whitelist", x11file);
+
+                free(x11file);
+                free(wx11file);
+
+                // 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);
+
+
+#if 0
         // keep a copy of real /tmp/.X11-unix directory in WHITELIST_TMP_DIR
         int rv = mkdir(RUN_WHITELIST_X11_DIR, 1777);
         if (rv == -1)
@@ -177,6 +252,8 @@ void fs_x11(void) {
                 errExit("mount");
          fs_logger2("blacklist", RUN_WHITELIST_X11_DIR);
 #endif
+
+#endif
 }