On Mon, Feb 27, 2017 at 03:31:47PM +0100, Greg Kurz wrote: > On Mon, 27 Feb 2017 12:44:30 +0000 > Stefan Hajnoczi <[email protected]> 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 '/'.
I see, thanks for explaining. Stefan
signature.asc
Description: PGP signature
