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. another guest using "passthrough" mode for example.
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.
Symbolic links aren't the only threats actually: a malicious guest could
change a path element to point to other types of file with undesirable
effects:
- a named pipe or any other thing that would cause openat() to block
- a terminal device which would become QEMU's controlling terminal
These issues can be addressed with O_NONBLOCK and O_NOCTTY.
Two helpers are introduced: one to open intermediate path elements and one
to open the rightmost path element.
Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - introduced openat_dir() and openat_file() helpers
- move stripping of leading '/' characters to caller
---
hw/9pfs/9p-util.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/9pfs/9p-util.h | 48 ++++++++++++++++++++++++++++++++++++++++++++
hw/9pfs/Makefile.objs | 2 +-
3 files changed, 102 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..62fd7a76212a
--- /dev/null
+++ b/hw/9pfs/9p-util.c
@@ -0,0 +1,53 @@
+/*
+ * 9p utilities
+ *
+ * Copyright IBM, Corp. 2017
+ *
+ * Authors:
+ * Greg Kurz <groug@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)
+{
+ 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;
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
new file mode 100644
index 000000000000..ca0d440ddc1e
--- /dev/null
+++ b/hw/9pfs/9p-util.h
@@ -0,0 +1,48 @@
+/*
+ * 9p utilities
+ *
+ * Copyright IBM, Corp. 2017
+ *
+ * Authors:
+ * Greg Kurz <groug@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.
+ */
+
+#ifndef QEMU_9P_UTIL_H
+#define QEMU_9P_UTIL_H
+
+static inline void close_preserve_errno(int fd)
+{
+ int serrno = errno;
+ close(fd);
+ errno = serrno;
+}
+
+static inline int openat_dir(int dirfd, const char *name)
+{
+ return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH);
+}
+
+static inline int openat_file(int dirfd, const char *name, int flags,
+ mode_t mode)
+{
+ int fd, serrno;
+
+ fd = openat(dirfd, name, flags | O_NOFOLLOW | O_NOCTTY | O_NONBLOCK,
+ mode);
+ if (fd == -1) {
+ return -1;
+ }
+
+ serrno = errno;
+ /* O_NONBLOCK was only needed to open the file. Let's drop it. */
+ assert(!fcntl(fd, F_SETFL, flags));
+ errno = serrno;
+ return fd;
+}
+
+int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode);
+
+#endif
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index da0ae0cfdbae..32197e6671dd 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-y = 9p.o
+common-obj-y = 9p.o 9p-util.o
common-obj-y += 9p-local.o 9p-xattr.o
common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
common-obj-y += coth.o cofs.o codir.o cofile.o
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 believe guests can pass in paths with consecutive slashes, so the
function must cope with them.
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 '/'.
On Mon, Feb 27, 2017 at 03:31:47PM +0100, Greg Kurz wrote:
> 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 '/'.
I see, thanks for explaining.
Stefan
On 02/26/2017 04:42 PM, Greg Kurz wrote:
> When using the passthrough security mode, symbolic links created by the
> guest are actual symbolic links on the host file system.
>
>
> diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> new file mode 100644
> index 000000000000..62fd7a76212a
> --- /dev/null
> +int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode)
> +{
> + int fd;
> +
> + fd = dup(dirfd);
> + if (fd == -1) {
> + return -1;
> + }
> +
Do you want to assert that the caller's path does not start with '/'?
This function ignores dirfd in that case, which may not be what you want.
> + while (*path) {
> + const char *c;
> + int next_fd;
> + char *head;
> +
> + head = g_strdup(path);
> + c = strchr(path, '/');
So if the caller passes path="a//b", then the first iteration sets
head="a", but the second iteration sets head="".
> + if (c) {
> + head[c - path] = 0;
> + next_fd = openat_dir(fd, head);
The second iteration will then fail (openat_dir on "" should fail with
ENOENT, right?). Oops.
> + } 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;
I think the fix is that you should skip past all consecutive '/' here,
rather than assuming there is just one. Or can you assert that all
callers are well-behaved, and that *path is not '/' at this point?
> + }
> +static inline int openat_file(int dirfd, const char *name, int flags,
> + mode_t mode)
> +{
> + int fd, serrno;
> +
> + fd = openat(dirfd, name, flags | O_NOFOLLOW | O_NOCTTY | O_NONBLOCK,
> + mode);
> + if (fd == -1) {
> + return -1;
> + }
> +
> + serrno = errno;
> + /* O_NONBLOCK was only needed to open the file. Let's drop it. */
> + assert(!fcntl(fd, F_SETFL, flags));
Ouch - side effect inside an assertion. We don't support use of NDEBUG,
but this is poor practice.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
On Mon, 27 Feb 2017 17:28:33 -0600
Eric Blake <eblake@redhat.com> wrote:
> On 02/26/2017 04:42 PM, Greg Kurz wrote:
> > When using the passthrough security mode, symbolic links created by the
> > guest are actual symbolic links on the host file system.
> >
>
> >
> > diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> > new file mode 100644
> > index 000000000000..62fd7a76212a
> > --- /dev/null
>
> > +int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode)
> > +{
> > + int fd;
> > +
> > + fd = dup(dirfd);
> > + if (fd == -1) {
> > + return -1;
> > + }
> > +
>
> Do you want to assert that the caller's path does not start with '/'?
Yes, I've added this for the pull request.
> This function ignores dirfd in that case, which may not be what you want.
>
Indeed, it really needs the path to be relative.
> > + while (*path) {
> > + const char *c;
> > + int next_fd;
> > + char *head;
> > +
> > + head = g_strdup(path);
> > + c = strchr(path, '/');
>
> So if the caller passes path="a//b", then the first iteration sets
> head="a", but the second iteration sets head="".
>
This doesn't happen with the current code, but you're right, we should
assert here also. We only wany a/b/c/d
>
> > + if (c) {
> > + head[c - path] = 0;
> > + next_fd = openat_dir(fd, head);
>
> The second iteration will then fail (openat_dir on "" should fail with
> ENOENT, right?). Oops.
>
> > + } 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;
>
> I think the fix is that you should skip past all consecutive '/' here,
> rather than assuming there is just one. Or can you assert that all
> callers are well-behaved, and that *path is not '/' at this point?
>
Again you're right :-\
> > + }
>
> > +static inline int openat_file(int dirfd, const char *name, int flags,
> > + mode_t mode)
> > +{
> > + int fd, serrno;
> > +
> > + fd = openat(dirfd, name, flags | O_NOFOLLOW | O_NOCTTY | O_NONBLOCK,
> > + mode);
> > + if (fd == -1) {
> > + return -1;
> > + }
> > +
> > + serrno = errno;
> > + /* O_NONBLOCK was only needed to open the file. Let's drop it. */
> > + assert(!fcntl(fd, F_SETFL, flags));
>
> Ouch - side effect inside an assertion. We don't support use of NDEBUG,
> but this is poor practice.
>
And I now remember you already made a similar comment in the past... I hope
I will remember this time.
Thanks!
© 2016 - 2025 Red Hat, Inc.