From edece29b01471d2e2ea50389fc1164c3ce57b69b Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 20 May 2024 15:47:27 -0400 Subject: [PATCH 1/2] Do not use path containing "/" in linkat() or stat() This defeats the protections provided by opendir_safe(). Instead, use the already-open file descriptor for the file's containing directory. It is unclear whether this can be used to escape a bind mount, as linkat() might fail with -EXDEV in this case. However, it is definitely wrong and needs to be fixed. A search for "untrusted_name" in qrexec-lib/unpack.c finds that these are the only places where an untrusted path that may contain "/" is used as a path in a system call argument. In all other cases, either the path is trusted or only paths that are guaranteed to not contain "/" are used, ensuring that the vulnerability in Qubes Security Bulletin 014 can never be a problem again. Fixes: ce2df9184c30 ("Initial work on safe open") --- qrexec-lib/unpack.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c index b3c3f456..a6781949 100644 --- a/qrexec-lib/unpack.c +++ b/qrexec-lib/unpack.c @@ -226,8 +226,6 @@ void process_one_file_reg(struct file_header *untrusted_hdr, fdout = openat(safe_dirfd, last_segment, O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC | O_NOCTTY, 0000); if (fdout < 0) do_exit(errno, untrusted_name); - if (safe_dirfd != AT_FDCWD) - close(safe_dirfd); /* sizes are signed elsewhere */ if (untrusted_hdr->filelen > LLONG_MAX || (bytes_limit && untrusted_hdr->filelen > bytes_limit)) @@ -251,10 +249,12 @@ void process_one_file_reg(struct file_header *untrusted_hdr, char fd_str[11]; if ((unsigned)snprintf(fd_str, sizeof(fd_str), "%d", fdout) >= sizeof(fd_str)) abort(); - if (linkat(procdir_fd, fd_str, AT_FDCWD, untrusted_name, AT_SYMLINK_FOLLOW) < 0) + if (linkat(procdir_fd, fd_str, safe_dirfd, last_segment, AT_SYMLINK_FOLLOW) < 0) do_exit(errno, untrusted_name); } fix_times_and_perms(fdout, untrusted_hdr, NULL, untrusted_name); + if (safe_dirfd != AT_FDCWD) + close(safe_dirfd); close(fdout); free(path_dup); } @@ -281,7 +281,7 @@ void process_one_file_dir(struct file_header *untrusted_hdr, } if (errno != EEXIST) do_exit(errno, untrusted_name); - if (stat(untrusted_name,&buf) < 0) + if (fstatat(safe_dirfd, last_segment, &buf, AT_SYMLINK_NOFOLLOW) < 0) do_exit(errno, untrusted_name); total_bytes += buf.st_size; /* size accumulated after the fact, so don't check limit here */ From 075dfe462639f7d9a205138558cbf5a3a64d2366 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 8 May 2024 17:41:55 -0400 Subject: [PATCH 2/2] Do not use fchmodat() because it follows symlinks This is not a security vulnerability, since an attacker cannot cause a symbolic link to replace a file and the symlink restrictions mean that the attacker could only change permissions of paths inside ~/QubesIncoming/VMNAME/TOPLEVEL_DIR anyway. Still, not following symbolic links is the right thing to do. With pre-6.6 Linux kernels, fchmodat(fd, path, AT_SYMLINK_NOFOLLOW) is broken in a chroot without /proc mounted, so it cannot be used. Instead, open the path and call fchmod() on the file descriptor. Fixes: ed68c01716b4 ("Use FD-based versions chmod and utime") --- qrexec-lib/unpack.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c index a6781949..5715fd26 100644 --- a/qrexec-lib/unpack.c +++ b/qrexec-lib/unpack.c @@ -130,7 +130,6 @@ static long validate_utime_nsec(uint32_t untrusted_nsec) static void fix_times_and_perms(const int fd, const struct file_header *const untrusted_hdr, - const char *const last_segment, const char *const untrusted_name) { const struct timespec times[2] = @@ -144,21 +143,12 @@ static void fix_times_and_perms(const int fd, .tv_nsec = validate_utime_nsec(untrusted_hdr->mtime_nsec) }, }; - if (last_segment == NULL) { - /* Do not change the mode of symbolic links */ - if (!S_ISLNK(untrusted_hdr->mode) && + /* Do not change the mode of symbolic links */ + if (!S_ISLNK(untrusted_hdr->mode) && fchmod(fd, untrusted_hdr->mode & 07777)) - do_exit(errno, untrusted_name); - if (futimens(fd, times)) /* as above */ - do_exit(errno, untrusted_name); - } else { - /* Do not change the mode of what a symbolic link points to */ - if (!S_ISLNK(untrusted_hdr->mode) && - fchmodat(fd, last_segment, untrusted_hdr->mode & 07777, 0)) - do_exit(errno, untrusted_name); - if (utimensat(fd, last_segment, times, AT_SYMLINK_NOFOLLOW)) /* as above */ - do_exit(errno, untrusted_name); - } + do_exit(errno, untrusted_name); + if (futimens(fd, times)) /* as above */ + do_exit(errno, untrusted_name); } // Open the second-to-last component of a path, enforcing O_NOFOLLOW for every @@ -252,7 +242,7 @@ void process_one_file_reg(struct file_header *untrusted_hdr, if (linkat(procdir_fd, fd_str, safe_dirfd, last_segment, AT_SYMLINK_FOLLOW) < 0) do_exit(errno, untrusted_name); } - fix_times_and_perms(fdout, untrusted_hdr, NULL, untrusted_name); + fix_times_and_perms(fdout, untrusted_hdr, untrusted_name); if (safe_dirfd != AT_FDCWD) close(safe_dirfd); close(fdout); @@ -281,11 +271,13 @@ void process_one_file_dir(struct file_header *untrusted_hdr, } if (errno != EEXIST) do_exit(errno, untrusted_name); - if (fstatat(safe_dirfd, last_segment, &buf, AT_SYMLINK_NOFOLLOW) < 0) + int new_dirfd = openat(safe_dirfd, last_segment, O_RDONLY | O_NOFOLLOW | O_CLOEXEC | O_DIRECTORY); + if (new_dirfd < 0 || fstat(new_dirfd, &buf) < 0) do_exit(errno, untrusted_name); total_bytes += buf.st_size; /* size accumulated after the fact, so don't check limit here */ - fix_times_and_perms(safe_dirfd, untrusted_hdr, last_segment, untrusted_name); + fix_times_and_perms(new_dirfd, untrusted_hdr, untrusted_name); + close(new_dirfd); if (safe_dirfd != AT_FDCWD) close(safe_dirfd); free(path_dup);