x11 hardening

1 files changed, 33 insertions, 11 deletions

diff --git a/src/firejail/x11.c b/src/firejail/x11.c
index e707ab8bd..944c24bc7 100644
--- a/src/firejail/x11.c
+++ b/src/firejail/x11.c
@@ -1263,11 +1263,22 @@ void fs_x11(void) {
         if (display <= 0)
                 return;
 
+        struct stat s1, s2;
+        if (stat("/tmp", &s1) != 0 || lstat("/tmp/.X11-unix", &s2) != 0)
+                return;
+        if ((s1.st_mode & S_ISVTX) == 0) {
+                fwarning("cannot mask X11 sockets: sticky bit not set on /tmp directory\n");
+                return;
+        }
+        if (s2.st_uid != 0) {
+                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");
         struct stat x11stat;
-        if (stat(x11file, &x11stat) == -1 || !S_ISSOCK(x11stat.st_mode)) {
+        if (lstat(x11file, &x11stat) != 0 || !S_ISSOCK(x11stat.st_mode)) {
                 free(x11file);
                 return;
         }
@@ -1278,12 +1289,8 @@ void fs_x11(void) {
         // 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)
+        if (mkdir(RUN_WHITELIST_X11_DIR, 0700) == -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");
 
@@ -1302,21 +1309,36 @@ void fs_x11(void) {
                 errExit("fchown");
         close(fd);
 
-        // do the mount
+        // 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");
-        if (mount(wx11file, x11file, NULL, MS_BIND|MS_REC, NULL) < 0)
+        fd = safe_fd(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)
+                errExit("asprintf");
+        if (mount(proc, x11file, NULL, MS_BIND|MS_REC, NULL) < 0)
                 errExit("mount bind");
         fs_logger2("whitelist", x11file);
-
-        free(x11file);
-        free(wx11file);
+        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
 }