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

[Stefan Hajnoczi]

On Thu, Feb 23, 2017 at 12:56:21PM +0100, Greg Kurz wrote:
> On Thu, 23 Feb 2017 11:16:44 +0000
> Stefan Hajnoczi <address@hidden> wrote:
> 
> > On Mon, Feb 20, 2017 at 03:39:51PM +0100, Greg Kurz wrote:
> > > When using the passthrough security mode, symbolic links created by the
> > > guest are actual symbolic links on the host file system.
> > > 
> > > Since the resolution of symbolic links during path walk is supposed to
> > > occur on the client side. The server should hence never receive any path
> > > pointing to an actual symbolic link. This isn't guaranteed by the protocol
> > > though, and malicious code in the guest can trick the server to issue
> > > various syscalls on paths whose one or more elements are symbolic links.
> > > In the case of the "local" backend using the "passthrough" or "none"
> > > security modes, the guest can directly create symbolic links to arbitrary
> > > locations on the host (as per spec). The "mapped-xattr" and "mapped-file"
> > > security modes are also affected to a lesser extent as they require some
> > > help from an external entity to create actual symbolic links on the host,
> > > i.e. anoter guest using "passthrough" mode for example.  
> > 
> > s/anoter/another/
> > 
> > > 
> > > The current code hence relies on O_NOFOLLOW and "l*()" variants of system
> > > calls. Unfortunately, this only applies to the rightmost path component.
> > > A guest could maliciously replace any component in a trusted path with a
> > > symbolic link. This could allow any guest to escape a virtfs shared 
> > > folder.
> > > 
> > > This patch introduces a variant of the openat() syscall that successively
> > > opens each path element with O_NOFOLLOW. When passing a file descriptor
> > > pointing to a trusted directory, one is guaranteed to be returned a
> > > file descriptor pointing to a path which is beneath the trusted directory.
> > > This will be used by subsequent patches to implement symlink-safe path 
> > > walk
> > > for any access to the backend.
> > > 
> > > Suggested-by: Jann Horn <address@hidden>
> > > Signed-off-by: Greg Kurz <address@hidden>
> > > ---
> > >  hw/9pfs/9p-util.c     |   69 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  hw/9pfs/9p-util.h     |   25 ++++++++++++++++++
> > >  hw/9pfs/Makefile.objs |    2 +
> > >  3 files changed, 95 insertions(+), 1 deletion(-)
> > >  create mode 100644 hw/9pfs/9p-util.c
> > >  create mode 100644 hw/9pfs/9p-util.h
> > > 
> > > diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> > > new file mode 100644
> > > index 000000000000..48292d948401
> > > --- /dev/null
> > > +++ b/hw/9pfs/9p-util.c
> > > @@ -0,0 +1,69 @@
> > > +/*
> > > + * 9p utilities
> > > + *
> > > + * Copyright IBM, Corp. 2017
> > > + *
> > > + * Authors:
> > > + *  Greg Kurz <address@hidden>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > > later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "9p-util.h"
> > > +
> > > +int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode) 
> > >  
> > 
> > This function doesn't handle absolute paths?  It ignores leading '/' and
> > therefore treats all paths as relative paths.
> > 
> 
> Yes because any path coming from the client is supposed (*) to be relative to 
> the
> shared directory and openat(2) says:

Please change the function name since this isn't openat with nofollow
behavior, it's a subset of openat that only takes relative paths with
nofollow behavior.

signature.asc
Description: PGP signature