From 923d7ada73f9600cda12a4ceb59b90928e4ce0d6 Mon Sep 17 00:00:00 2001 From: smitsohu Date: Mon, 3 May 2021 01:09:05 +0200 Subject: introduce safer_openat function --- src/firejail/chroot.c | 8 ++++---- src/firejail/dbus.c | 2 +- src/firejail/firejail.h | 2 +- src/firejail/fs.c | 10 +++++----- src/firejail/fs_home.c | 6 +++--- src/firejail/fs_whitelist.c | 12 ++++++------ src/firejail/pulseaudio.c | 2 +- src/firejail/restrict_users.c | 2 +- src/firejail/util.c | 45 +++++++++++++++++++++++-------------------- src/firejail/x11.c | 10 +++++----- 10 files changed, 51 insertions(+), 48 deletions(-) (limited to 'src') 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 #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 -- cgit v1.2.3-54-g00ecf