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

[Eric Blake]

On 08/08/2017 12:28 PM, 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.
> 
> 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

Did your attempt at a kernel patch for AT_SYMLINK_NOFOLLOW ever get
anywhere?

> 
> 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.
> 
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  hw/9pfs/9p-local.c |   44 ++++++++++++++++++++++++++++----------------
>  hw/9pfs/9p-util.h  |   10 +++++++---
>  2 files changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 6e478f4765ef..b178d627c764 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -333,30 +333,42 @@ update_map_file:
>  
>  static int fchmodat_nofollow(int dirfd, const char *name, mode_t mode)
>  {
> +    struct stat stbuf;
>      int fd, ret;
> +    char *proc_path;
>  
>      /* FIXME: this should be handled with fchmodat(AT_SYMLINK_NOFOLLOW).
> -     * Unfortunately, the linux kernel doesn't implement it yet. As an
> -     * alternative, let's open the file and use fchmod() instead. This
> -     * may fail depending on the permissions of the file, but it is the
> -     * best we can do to avoid TOCTTOU. We first try to open read-only
> -     * in case name points to a directory. If that fails, we try write-only
> -     * in case name doesn't point to a directory.
> +     * Unfortunately, the linux kernel doesn't implement it yet.
>       */
> -    fd = openat_file(dirfd, name, O_RDONLY, 0);
> -    if (fd == -1) {
> -        /* In case the file is writable-only and isn't a directory. */
> -        if (errno == EACCES) {
> -            fd = openat_file(dirfd, name, O_WRONLY, 0);
> -        }
> -        if (fd == -1 && errno == EISDIR) {
> -            errno = EACCES;
> -        }
> +    if (fstatat(dirfd, name, &stbuf, AT_SYMLINK_NOFOLLOW)) {
> +        return -1;
>      }

Checking the file...

> +
> +    if (S_ISLNK(stbuf.st_mode)) {
> +        errno = ELOOP;
> +        return -1;
> +    }
> +
> +    fd = openat_file(dirfd, name, O_RDONLY | O_PATH, 0);

...and then opening the file is a TOCTTOU race (although it works most
of the time and avoids the open where it is easy)...

>      if (fd == -1) {
>          return -1;
>      }
> -    ret = fchmod(fd, mode);
> +
> +    ret = fstat(fd, &stbuf);
> +    if (ret) {
> +        goto out;
> +    }

...so you are double-checking that you got a non-symlink after all (your
insurance against the race having done the wrong thing [but see below])...

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

...at which point you now have a valid file name that represents the
file you wanted to chmod() in the first place (even if another rename
occurs in the meantime, you are changing the mode tied to the
non-symlink fd that you double-checked, which ends up behaving as if you
had won the race and made the chmod() call before the rename).

> +    g_free(proc_path);
> +out:
>      close_preserve_errno(fd);
>      return ret;

Might be worth littering some comments in the code explaining why you
have to call both fstatat() and stat(), or perhaps you could drop the
first fstatat() and just always do the open().

Reading 'man open', it looks like O_PATH will chase symlinks UNLESS you
also use O_NOFOLLOW.  So I had to code up a simple test program to
verify if things work...

=============
#define _GNU_SOURCE 1
#include <sys/stat.h>
#include <stdio.h>
#include <errno.h>
#include <unistd.h>
#include <fcntl.h>

int main(void) {
    struct stat st;
    int fd;

    remove("f");
    remove("l");
    if (creat("f", 0) < 0)
        return 1;
    if (symlink("f", "l") < 0)
        return 2;

    fd = open("l", O_RDONLY | O_PATH);
    if (fd < 0)
        return 3;
    if (fstat(fd, &st) < 0)
        return 4;
    if (S_ISLNK(st.st_mode)) {
        printf("got a link\n");
    } else {
        printf("dereferenced\n");
    }
    close(fd);

    fd = open("l", O_RDONLY | O_PATH | O_NOFOLLOW);
    if (fd < 0)
        return 5;
    if (fstat(fd, &st) < 0)
        return 6;
    if (S_ISLNK(st.st_mode)) {
        printf("got a link\n");
    } else {
        printf("dereferenced\n");
    }
    close(fd);

    remove("f");
    remove("l");
    return 0;
}
============
$ ./foo
dereferenced
got a link

Ouch - you need to fix your patch to use open(O_NOFOLLOW | O_PATH).

Furthermore, I'm worried that your patch is regressing commit 918112c0,
when compiling for older platforms where either O_PATH does not exist,
or where libc exposes the constant but the kernel is too old to
understand it.  (I guess the latter problem is already existing in our
code base, so we really only need to worry about the former of compiling
when the constant does not exist).  So if supporting old platforms is
desired, you MUST keep BOTH approaches intact, gated by whether O_PATH
is defined to a non-zero value, rather than just assuming O_PATH works.

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

signature.asc
Description: OpenPGP digital signature