harden read-write mounts, cleanup

3 files changed, 45 insertions, 29 deletions

diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index 88f92ad74..f3ed67928 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -484,29 +484,44 @@ void fs_rdonly(const char *dir) {
 
 static void fs_rdwr(const char *dir) {
         assert(dir);
-        // check directory exists
+        // check directory exists and ensure we have a resolved path
+        // the resolved path allows to run a sanity check after the mount
+        char *path = realpath(dir, NULL);
+        if (path == NULL)
+                return;
+        // allow only user owned directories, except the user is root
+        uid_t u = getuid();
         struct stat s;
-        int rv = stat(dir, &s);
-        if (rv == 0) {
-                // if the file is outside /home directory, allow only root user
-                uid_t u = getuid();
-                if (u != 0 && s.st_uid != u) {
-                        fwarning("you are not allowed to change %s to read-write\n", dir);
-                        return;
-                }
-
-                // mount --bind /bin /bin
-                // mount --bind -o remount,rw /bin
-                unsigned long flags = 0;
-                get_mount_flags(dir, &flags);
-                if ((flags & MS_RDONLY) == 0)
-                        return;
-                flags &= ~MS_RDONLY;
-                if (mount(dir, dir, NULL, MS_BIND|MS_REC, NULL) < 0 ||
-                    mount(NULL, dir, NULL, flags|MS_BIND|MS_REMOUNT|MS_REC, NULL) < 0)
-                        errExit("mount read-write");
-                fs_logger2("read-write", dir);
+        int rv = stat(path, &s);
+        if (rv) {
+                free(path);
+                return;
+        }
+        if (u != 0 && s.st_uid != u) {
+                fwarning("you are not allowed to change %s to read-write\n", path);
+                free(path);
+                return;
+        }
+        // mount --bind /bin /bin
+        // mount --bind -o remount,rw /bin
+        unsigned long flags = 0;
+        get_mount_flags(path, &flags);
+        if ((flags & MS_RDONLY) == 0) {
+                free(path);
+                return;
         }
+        flags &= ~MS_RDONLY;
+        if (mount(path, path, NULL, MS_BIND|MS_REC, NULL) < 0 ||
+            mount(NULL, path, NULL, flags|MS_BIND|MS_REMOUNT|MS_REC, NULL) < 0)
+                errExit("mount read-write");
+        fs_logger2("read-write", path);
+
+        // run a check on /proc/self/mountinfo to validate the mount
+        MountData *mptr = get_last_mount();
+        if (strncmp(mptr->dir, path, strlen(path)) != 0)
+                errLogExit("invalid read-write mount");
+
+        free(path);
 }
 
 void fs_noexec(const char *dir) {
diff --git a/src/firejail/pulseaudio.c b/src/firejail/pulseaudio.c
index eaaba86c0..15d44e4cc 100644
--- a/src/firejail/pulseaudio.c
+++ b/src/firejail/pulseaudio.c
@@ -178,10 +178,10 @@ void pulseaudio_init(void) {
 
                 // check /proc/self/mountinfo to confirm the mount is ok
                 MountData *mptr = get_last_mount();
-                if (strncmp(mptr->dir, homeusercfg, strlen(homeusercfg)) != 0)
-                        errLogExit("invalid mount on top of %s (should be %s)\n", mptr->dir, homeusercfg);
-                if (strncmp(mptr->fstype, "tmpfs", 5) != 0)
-                        errLogExit("invalid mount on top of %s (filesystem type is %s)\n", mptr->dir, mptr->fstype);
+                if (strcmp(mptr->dir, homeusercfg) != 0)
+                        errLogExit("invalid pulseaudio mount");
+                if (strcmp(mptr->fstype, "tmpfs") != 0)
+                        errLogExit("invalid pulseaudio mount");
 
                 char *p;
                 if (asprintf(&p, "%s/client.conf", homeusercfg) == -1)
diff --git a/src/firejail/x11.c b/src/firejail/x11.c
index 0eace3215..ec8775370 100644
--- a/src/firejail/x11.c
+++ b/src/firejail/x11.c
@@ -1196,10 +1196,11 @@ void x11_xorg(void) {
 
         // check /proc/self/mountinfo to confirm the mount is ok
         MountData *mptr = get_last_mount();
-        if (strncmp(mptr->dir, dest, strlen(dest)) != 0)
-                errLogExit("invalid mount on top of %s (should be %s)\n", mptr->dir, dest);
-        if (strncmp(mptr->fstype, "tmpfs", 5) != 0)
-                errLogExit("invalid mount on top of %s (filesystem type is %s)\n", mptr->dir, mptr->fstype);
+        if (strcmp(mptr->dir, dest) != 0)
+                errLogExit("invalid .Xauthority mount");
+        if (strcmp(mptr->fstype, "tmpfs") != 0)
+                errLogExit("invalid .Xauthority mount");
+
         free(dest);
 #endif
 }