Re: [Qemu-devel] [for-2.10 PATCH v2] 9pfs: local: fix fchmodat_nofollow() limitations

[Eric Blake]

On 08/09/2017 09:23 AM, Greg Kurz wrote:
> This function has to ensure it doesn't follow a symlink that could be used
> to escape the virtfs directory. This could be easily achieved if fchmodat()
> on linux honored the AT_SYMLINK_NOFOLLOW flag as described in POSIX, but
> it doesn't.

Might be worth including a URL of the LKML discussion on the last
version of that patch attempt.

> 
> The current implementation covers most use-cases, but it notably fails if:
> - the target path has access rights equal to 0000 (openat() returns EPERM),
>   => once you've done chmod(0000) on a file, you can never chmod() again
> - the target path is UNIX domain socket (openat() returns ENXIO)
>   => bind() of UNIX domain sockets fails if the file is on 9pfs
> 
> The solution is to use O_PATH: openat() now succeeds in both cases, and we
> can ensure the path isn't a symlink with fstat(). The associated entry in
> "/proc/self/fd" can hence be safely passed to the regular chmod() syscall.

Hey - should we point this out as a viable solution to the glibc folks,
since their current user-space emulation of AT_SYMLINK_NOFOLLOW is broken?

> 
> The previous behavior is kept for older systems that don't have O_PATH.
> 
> Signed-off-by: Greg Kurz <address@hidden>
> ---
> v2: - renamed OPENAT_DIR_O_PATH to O_PATH_9P_UTIL and use it as a replacement
>       for O_PATH to avoid build breaks on O_PATH-less systems
>     - keep current behavior for O_PATH-less systems
>     - added comments
>     - TODO in 2.11: add _nofollow suffix to openat_dir() and openat_file()
> ---
>  hw/9pfs/9p-local.c |   41 +++++++++++++++++++++++++++++++++--------
>  hw/9pfs/9p-util.h  |   24 +++++++++++++++---------
>  2 files changed, 48 insertions(+), 17 deletions(-)
> 
> +     /* First, we clear non-racing symlinks out of the way. */
> +    if (fstatat(dirfd, name, &stbuf, AT_SYMLINK_NOFOLLOW)) {
> +        return -1;
> +    }
> +    if (S_ISLNK(stbuf.st_mode)) {
> +        errno = ELOOP;

I don't know if ELOOP is the best errno value to use here, but I don't
have any better suggestions so I'm okay with it.

> +        return -1;
> +    }
> +
> +    /* Access modes are ignored when O_PATH is supported. We try O_RDONLY and
> +     * O_WRONLY for old-systems that don't support O_PATH.
>       */
> -    fd = openat_file(dirfd, name, O_RDONLY, 0);
> +    fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL, 0);
>      if (fd == -1) {
>          /* In case the file is writable-only and isn't a directory. */
>          if (errno == EACCES) {
> @@ -356,7 +366,22 @@ static int fchmodat_nofollow(int dirfd, const char 
> *name, mode_t mode)
>      if (fd == -1) {
>          return -1;
>      }
> -    ret = fchmod(fd, mode);
> +
> +    /* Now we handle racing symlinks. */
> +    ret = fstat(fd, &stbuf);
> +    if (ret) {
> +        goto out;
> +    }
> +    if (S_ISLNK(stbuf.st_mode)) {
> +        errno = ELOOP;
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    proc_path = g_strdup_printf("/proc/self/fd/%d", fd);
> +    ret = chmod(proc_path, mode);

Nope, unsafe when O_PATH_9P_UTIL is 0.  This needs to be more like:

/* Now we handle racing symlinks.  On kernels without O_PATH, we will
 * fail on some corner cases, but that's better than dereferencing a
 * symlink that was injected during the TOCTTOU between our initial
 * fstatat() and openat_file().
 */
if (O_PATH_9P_UTIL) {
    fstat, S_ISLINK, proc_path, chmod()
} else {
    fchmod()
}

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature