[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 04/28] 9pfs: introduce openat_nofollow() help
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v2 04/28] 9pfs: introduce openat_nofollow() helper |
Date: |
Mon, 27 Feb 2017 15:32:52 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
On Mon, Feb 27, 2017 at 03:31:47PM +0100, Greg Kurz wrote:
> On Mon, 27 Feb 2017 12:44:30 +0000
> Stefan Hajnoczi <address@hidden> 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
- [Qemu-devel] [PATCH v2 00/28] Series short description, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 01/28] 9pfs: local: move xattr security ops to 9p-xattr.c, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 02/28] 9pfs: remove side-effects in local_init(), Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 03/28] 9pfs: remove side-effects in local_open() and local_opendir(), Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 04/28] 9pfs: introduce openat_nofollow() helper, Greg Kurz, 2017/02/26
- Re: [Qemu-devel] [PATCH v2 04/28] 9pfs: introduce openat_nofollow() helper, Eric Blake, 2017/02/27
- [Qemu-devel] [PATCH v2 05/28] 9pfs: local: keep a file descriptor on the shared folder, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 06/28] 9pfs: local: open/opendir: don't follow symlinks, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 07/28] 9pfs: local: lgetxattr: don't follow symlinks, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 08/28] 9pfs: local: llistxattr: don't follow symlinks, Greg Kurz, 2017/02/26