Re: [Qemu-devel] [PATCH v2 04/28] 9pfs: introduce openat_nofollow() helper

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]

On Mon, 27 Feb 2017 12:44:30 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Sun, Feb 26, 2017 at 11:42:03PM +0100, Greg Kurz wrote:
> > +int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode)
> > +{
> > +    int fd;
> > +
> > +    fd = dup(dirfd);
> > +    if (fd == -1) {
> > +        return -1;
> > +    }
> > +
> > +    while (*path) {
> > +        const char *c;
> > +        int next_fd;
> > +        char *head;
> > +
> > +        head = g_strdup(path);
> > +        c = strchr(path, '/');
> > +        if (c) {
> > +            head[c - path] = 0;
> > +            next_fd = openat_dir(fd, head);
> > +        } else {
> > +            next_fd = openat_file(fd, head, flags, mode);
> > +        }
> > +        g_free(head);
> > +        if (next_fd == -1) {
> > +            close_preserve_errno(fd);
> > +            return -1;
> > +        }
> > +        close(fd);
> > +        fd = next_fd;
> > +
> > +        if (!c) {
> > +            break;
> > +        }
> > +        path = c + 1;
> > +    }
> > +
> > +    return fd;
> > +}  
> 
> If I understand the Linux openat(2) implementation correctly this
> function fails with ENOENT if:
> 
> 1. An absolute path is given
> 2. A path contains consecutive slashes ("a///b")
> 
> Both of these behaviors are problematic.  If the function doesn't
> support absolute paths it should be called relative_openat_nofollow()
> and have an error if path[0] == '/'.
> 

I agree with the name change since we don't want this function to deal
with absolute names, per-design. It shouldn't even return an error but
rather assert (see below for more details).

> I believe guests can pass in paths with consecutive slashes, so the
> function must cope with them.

Actually no. The 9P protocol implements paths as an array of path
elements, and the frontend prohibits '/' characters in individual
path elements (look for name_is_illegal in hw/9pfs/9p.c).

We cannot have consecutive '/' characters in the intermediate part or at
the end of the path.  But we do have '//' at the beginning.

This lies lies in the way the frontend internally forges paths to access
files.

A typical path is in the form:

${root}/${elem1}/${elem2}/${elem3}/ ... etc... /${elemN}

where ${root} is set to '/' when the client mounts the 9pfs share (see
v9fs_attach() in hw/9pfs/9p.c). Since all paths are supposed to be
relative to the path of shared directory on the host, I believe ${root}
should rather be '.' than '/'. Unfortunately, this contradicts this
snippet of code in the handle backend code (hw/9pfs/9p-handle.c):

static int handle_name_to_path(FsContext *ctx, V9fsPath *dir_path,
                              const char *name, V9fsPath *target)
{
[...]
    /* "." and ".." are not allowed */
    if (!strcmp(name, ".") || !strcmp(name, "..")) {
        errno = EINVAL;
        return -1;

    }

I should probably dig some more to see why it is coded that way. But since
I couldn't reproduce the vulnerability with the handle backend and this series
is already huge, I've opted to keep the hardcoded '/' in v9fs_attach().

The consequence is that I have to strip these leading '/' when calling openat().

But then the code can safely assume that paths don't have consecutive '/'.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]