On Thu, Feb 23, 2017 at 12:56:21PM +0100, Greg Kurz wrote: > On Thu, 23 Feb 2017 11:16:44 +0000 > Stefan Hajnoczi <stefa...@gmail.com> 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 <ja...@google.com> > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > > > --- > > > 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 <gr...@kaod.org> > > > + * > > > + * 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
