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: ed68c01 ("Use FD-based versions chmod and utime")

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);