introduce safer_openat function

10 files changed, 51 insertions, 48 deletions

diff --git a/src/firejail/chroot.c b/src/firejail/chroot.c
index d7e96cf4c..757ffb1f7 100644
--- a/src/firejail/chroot.c
+++ b/src/firejail/chroot.c
@@ -131,9 +131,9 @@ void fs_chroot(const char *rootdir) {
         assert(rootdir);
 
         // fails if there is any symlink or if rootdir is not a directory
-        int parentfd = safe_fd(rootdir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        int parentfd = safer_openat(-1, rootdir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         if (parentfd == -1)
-                errExit("safe_fd");
+                errExit("safer_openat");
         // rootdir has to be owned by root and is not allowed to be generally writable,
         // this also excludes /tmp and friends
         struct stat s;
@@ -215,12 +215,12 @@ void fs_chroot(const char *rootdir) {
 
                 if (arg_debug)
                         printf("Mounting %s on chroot %s\n", orig_pulse, orig_pulse);
-                int src = safe_fd(orig_pulse, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+                int src = safer_openat(-1, orig_pulse, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
                 if (src == -1) {
                         fprintf(stderr, "Error: cannot open %s\n", orig_pulse);
                         exit(1);
                 }
-                int dst = safe_fd(pulse, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+                int dst = safer_openat(-1, pulse, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
                 if (dst == -1) {
                         fprintf(stderr, "Error: cannot open %s\n", pulse);
                         exit(1);
diff --git a/src/firejail/dbus.c b/src/firejail/dbus.c
index 658b84537..b8aa2c974 100644
--- a/src/firejail/dbus.c
+++ b/src/firejail/dbus.c
@@ -416,7 +416,7 @@ void dbus_proxy_stop(void) {
 }
 
 static void socket_overlay(char *socket_path, char *proxy_path) {
-        int fd = safe_fd(proxy_path, O_PATH | O_NOFOLLOW | O_CLOEXEC);
+        int fd = safer_openat(-1, proxy_path, O_PATH | O_NOFOLLOW | O_CLOEXEC);
         if (fd == -1)
                 errExit("opening DBus proxy socket");
         struct stat s;
diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index ca4c988fa..6691b9570 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -528,7 +528,7 @@ void mkdir_attr(const char *fname, mode_t mode, uid_t uid, gid_t gid);
 unsigned extract_timeout(const char *str);
 void disable_file_or_dir(const char *fname);
 void disable_file_path(const char *path, const char *file);
-int safe_fd(const char *path, int flags);
+int safer_openat(int dirfd, const char *path, int flags);
 int has_handler(pid_t pid, int signal);
 void enter_network_namespace(pid_t pid);
 int read_pid(const char *name, pid_t *pid);
diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index fc67a15f3..09de11de9 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -453,7 +453,7 @@ void fs_tmpfs(const char *dir, unsigned check_owner) {
         if (arg_debug)
                 printf("Mounting tmpfs on %s, check owner: %s\n", dir, (check_owner)? "yes": "no");
         // get a file descriptor for dir, fails if there is any symlink
-        int fd = safe_fd(dir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        int fd = safer_openat(-1, dir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         if (fd == -1)
                 errExit("while opening directory");
         struct stat s;
@@ -493,7 +493,7 @@ static void fs_remount_simple(const char *path, OPERATION op) {
         assert(path);
 
         // open path without following symbolic links
-        int fd1 = safe_fd(path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        int fd1 = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
         if (fd1 == -1)
                 goto out;
         struct stat s1;
@@ -559,7 +559,7 @@ static void fs_remount_simple(const char *path, OPERATION op) {
 
         // mount --bind -o remount,ro path
         // need to open path again without following symbolic links
-        int fd2 = safe_fd(path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        int fd2 = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
         if (fd2 == -1)
                 errExit("open");
         struct stat s2;
@@ -992,9 +992,9 @@ void fs_overlayfs(void) {
                 char *firejail;
                 if (asprintf(&firejail, "%s/.firejail", cfg.homedir) == -1)
                         errExit("asprintf");
-                int fd = safe_fd(firejail, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+                int fd = safer_openat(-1, firejail, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
                 if (fd == -1)
-                        errExit("safe_fd");
+                        errExit("safer_openat");
                 free(firejail);
                 // create basedir if it doesn't exist
                 // the new directory will be owned by root
diff --git a/src/firejail/fs_home.c b/src/firejail/fs_home.c
index 46f32d7ad..e3b044a3b 100644
--- a/src/firejail/fs_home.c
+++ b/src/firejail/fs_home.c
@@ -262,10 +262,10 @@ void fs_private_homedir(void) {
         if (arg_debug)
                 printf("Mount-bind %s on top of %s\n", private_homedir, homedir);
         // get file descriptors for homedir and private_homedir, fails if there is any symlink
-        int src = safe_fd(private_homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        int src = safer_openat(-1, private_homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         if (src == -1)
                 errExit("opening private directory");
-        int dst = safe_fd(homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        int dst = safer_openat(-1, homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         if (dst == -1)
                 errExit("opening home directory");
         // both mount source and target should be owned by the user
@@ -576,7 +576,7 @@ void fs_private_home_list(void) {
         if (arg_debug)
                 printf("Mount-bind %s on top of %s\n", RUN_HOME_DIR, homedir);
 
-        int fd = safe_fd(homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        int fd = safer_openat(-1, homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         if (fd == -1)
                 errExit("opening home directory");
         // home directory should be owned by the user
diff --git a/src/firejail/fs_whitelist.c b/src/firejail/fs_whitelist.c
index 698d47b69..23310d92d 100644
--- a/src/firejail/fs_whitelist.c
+++ b/src/firejail/fs_whitelist.c
@@ -28,7 +28,7 @@
 
 #include <fcntl.h>
 #ifndef O_PATH
-# define O_PATH 010000000
+#define O_PATH 010000000
 #endif
 
 // mountinfo functionality test;
@@ -220,7 +220,7 @@ static void whitelist_path(ProfileEntry *entry) {
         // confirm again the mount source exists and there is no symlink
         struct stat wfilestat;
         EUID_USER();
-        int fd = safe_fd(wfile, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        int fd = safer_openat(-1, wfile, O_PATH|O_NOFOLLOW|O_CLOEXEC);
         EUID_ROOT();
         if (fd == -1) {
                 if (arg_debug || arg_debug_whitelists)
@@ -317,9 +317,9 @@ static void whitelist_path(ProfileEntry *entry) {
         if (mptr->dir == strrchr(mptr->dir, '/'))
                 errLogExit("invalid whitelist mount");
         // confirm the right file was mounted by comparing device and inode numbers
-        int fd4 = safe_fd(path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        int fd4 = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
         if (fd4 == -1)
-                errExit("safe_fd");
+                errExit("safer_openat");
         struct stat s;
         if (fstat(fd4, &s) == -1)
                 errExit("fstat");
@@ -1059,9 +1059,9 @@ void fs_whitelist(void) {
                 if (stat(cfg.homedir, &s) == 0) {
                         // keep a copy of real home dir in RUN_WHITELIST_HOME_USER_DIR
                         mkdir_attr(RUN_WHITELIST_HOME_USER_DIR, 0755, getuid(), getgid());
-                        int fd = safe_fd(cfg.homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+                        int fd = safer_openat(-1, cfg.homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
                         if (fd == -1)
-                                errExit("safe_fd");
+                                errExit("safer_openat");
                         char *proc;
                         if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
                                 errExit("asprintf");
diff --git a/src/firejail/pulseaudio.c b/src/firejail/pulseaudio.c
index 4b9203c36..a50134893 100644
--- a/src/firejail/pulseaudio.c
+++ b/src/firejail/pulseaudio.c
@@ -131,7 +131,7 @@ void pulseaudio_init(void) {
 
         // if ~/.config/pulse exists and there are no symbolic links, mount the new directory
         // else set environment variable
-        int fd = safe_fd(homeusercfg, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        int fd = safer_openat(-1, homeusercfg, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         if (fd == -1) {
                 pulseaudio_fallback(pulsecfg);
                 goto out;
diff --git a/src/firejail/restrict_users.c b/src/firejail/restrict_users.c
index a0ca4c02c..8368d84a1 100644
--- a/src/firejail/restrict_users.c
+++ b/src/firejail/restrict_users.c
@@ -73,7 +73,7 @@ static void sanitize_home(void) {
         if (arg_debug)
                 printf("Cleaning /home directory\n");
         // open user home directory in order to keep it around
-        int fd = safe_fd(cfg.homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        int fd = safer_openat(-1, cfg.homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
         if (fd == -1)
                 goto errout;
         if (fstat(fd, &s) == -1) { // FUSE
diff --git a/src/firejail/util.c b/src/firejail/util.c
index 8966521cb..d16354d9d 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -915,9 +915,9 @@ int remove_overlay_directory(void) {
                         errExit("fork");
                 if (child == 0) {
                         // open ~/.firejail, fails if there is any symlink
-                        int fd = safe_fd(path, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+                        int fd = safer_openat(-1, path, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
                         if (fd == -1)
-                                errExit("safe_fd");
+                                errExit("safer_openat");
                         // chdir to ~/.firejail
                         if (fchdir(fd) == -1)
                                 errExit("fchdir");
@@ -1136,13 +1136,13 @@ void disable_file_path(const char *path, const char *file) {
 }
 
 // open an existing file without following any symbolic link
-int safe_fd(const char *path, int flags) {
-        flags |= O_NOFOLLOW;
+// relative paths are interpreted relative to dirfd
+// ignore dirfd if path is absolute
+// https://web.archive.org/web/20180419120236/https://blogs.gnome.org/jamesh/2018/04/19/secure-mounts
+int safer_openat(int dirfd, const char *path, int flags) {
         assert(path);
-        if (*path != '/' || strstr(path, "..")) {
-                fprintf(stderr, "Error: invalid path %s\n", path);
-                exit(1);
-        }
+        flags |= O_NOFOLLOW;
+
         int fd = -1;
 
 #ifdef __NR_openat2 // kernel 5.6 or better
@@ -1150,7 +1150,7 @@ int safe_fd(const char *path, int flags) {
         memset(&oh, 0, sizeof(oh));
         oh.flags = flags;
         oh.resolve = RESOLVE_NO_SYMLINKS;
-        fd = syscall(__NR_openat2, -1, path, &oh, sizeof(struct open_how));
+        fd = syscall(__NR_openat2, dirfd, path, &oh, sizeof(struct open_how));
         if (fd != -1 || errno != ENOSYS)
                 return fd;
 #endif
@@ -1161,18 +1161,23 @@ int safe_fd(const char *path, int flags) {
         if (!dup)
                 errExit("strdup");
         char *tok = strtok(dup, "/");
-        if (!tok) { // root directory
+        if (!tok) { // nothing to do, path is either empty string or the root directory
                 free(dup);
-                return open("/", flags);
+                return openat(dirfd, path, flags);
         }
         char *last_tok = EMPTY_STRING;
-        int parentfd = open("/", O_PATH|O_CLOEXEC);
+
+        int parentfd;
+        if (path[0] == '/')
+                parentfd = open("/", O_PATH|O_CLOEXEC);
+        else
+                parentfd = fcntl(dirfd, F_DUPFD_CLOEXEC, 0);
         if (parentfd == -1)
-                errExit("open");
+                errExit("open/fcntl");
 
-        while(1) {
+        while (1) {
                 // open path component, assuming it is a directory; this fails with ENOTDIR if it is a symbolic link
-                // if token is a single dot, the previous directory is reopened
+                // if token is a single dot, the directory referred to by parentfd is reopened
                 fd = openat(parentfd, tok, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
                 if (fd == -1) {
                         // if the following token is NULL, the current token is the final path component
@@ -1303,13 +1308,11 @@ pid_t require_pid(const char *name) {
 // return 1 if there is a link somewhere in path of directory
 static int has_link(const char *dir) {
         assert(dir);
-        int fd = safe_fd(dir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
-        if (fd == -1) {
-                if ((errno == ELOOP || errno == ENOTDIR) && is_dir(dir))
-                        return 1;
-        }
-        else
+        int fd = safer_openat(-1, dir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+        if (fd != -1)
                 close(fd);
+        else if (errno == ELOOP || (errno == ENOTDIR && is_dir(dir)))
+                return 1;
         return 0;
 }
 
diff --git a/src/firejail/x11.c b/src/firejail/x11.c
index 1dabf272e..da0acc69c 100644
--- a/src/firejail/x11.c
+++ b/src/firejail/x11.c
@@ -1239,9 +1239,9 @@ void x11_xorg(void) {
                 }
         }
         // get a file descriptor for ~/.Xauthority
-        int dst = safe_fd(dest, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        int dst = safer_openat(-1, dest, O_PATH|O_NOFOLLOW|O_CLOEXEC);
         if (dst == -1)
-                errExit("safe_fd");
+                errExit("safer_openat");
         // check if the actual mount destination is a user owned regular file
         if (fstat(dst, &s) == -1)
                 errExit("fstat");
@@ -1263,9 +1263,9 @@ void x11_xorg(void) {
         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);
+        int src = safer_openat(-1, tmpfname, O_PATH|O_NOFOLLOW|O_CLOEXEC);
         if (src == -1)
-                errExit("safe_fd");
+                errExit("safer_openat");
         if (fstat(src, &s) == -1)
                 errExit("fstat");
         if (!S_ISREG(s.st_mode)) {
@@ -1373,7 +1373,7 @@ void fs_x11(void) {
         char *wx11file;
         if (asprintf(&wx11file, "%s/X%d", RUN_WHITELIST_X11_DIR, display) == -1)
                 errExit("asprintf");
-        fd = safe_fd(wx11file, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+        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