| Message ID | 148814893846.28146.10539730675852394601.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
On Sun, Feb 26, 2017 at 11:42:18PM +0100, Greg Kurz wrote: > @@ -48,6 +49,24 @@ typedef struct { > int mountfd; > } LocalData; > > +int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, > + mode_t mode) > +{ > + LocalData *data = fs_ctx->private; > + > + /* All paths are relative to the path data->mountfd points to */ > + while (*path == '/') { > + path++; > + } > + > + return openat_nofollow(data->mountfd, path, flags, mode); What about all the other openat_nofollow() users? They don't explicitly strip leading slashes. Perhaps this should be part of a renamed relative_openat_nofollow() function. > +} > + > +int local_opendir_nofollow(FsContext *fs_ctx, const char *path) > +{ > + return local_open_nofollow(fs_ctx, path, O_DIRECTORY | O_RDONLY, 0); Why not strip slashes here?
On Mon, 27 Feb 2017 12:49:01 +0000 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Sun, Feb 26, 2017 at 11:42:18PM +0100, Greg Kurz wrote: > > @@ -48,6 +49,24 @@ typedef struct { > > int mountfd; > > } LocalData; > > > > +int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, > > + mode_t mode) > > +{ > > + LocalData *data = fs_ctx->private; > > + > > + /* All paths are relative to the path data->mountfd points to */ > > + while (*path == '/') { > > + path++; > > + } > > + > > + return openat_nofollow(data->mountfd, path, flags, mode); > > What about all the other openat_nofollow() users? They don't explicitly $ git grep openat_nofollow hw/9pfs/9p-local.c: return openat_nofollow(data->mountfd, path, flags, mode); hw/9pfs/9p-util.c:int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode) hw/9pfs/9p-util.h:int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode); > strip leading slashes. Perhaps this should be part of a renamed > relative_openat_nofollow() function. > > > +} > > + > > +int local_opendir_nofollow(FsContext *fs_ctx, const char *path) > > +{ > > + return local_open_nofollow(fs_ctx, path, O_DIRECTORY | O_RDONLY, 0); > > Why not strip slashes here? Because this one calls the other one above.
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index be6be615149b..74b921e65316 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -13,6 +13,7 @@ #include "qemu/osdep.h" #include "9p.h" +#include "9p-local.h" #include "9p-xattr.h" #include "9p-util.h" #include "fsdev/qemu-fsdev.h" /* local_ops */ @@ -48,6 +49,24 @@ typedef struct { int mountfd; } LocalData; +int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, + mode_t mode) +{ + LocalData *data = fs_ctx->private; + + /* All paths are relative to the path data->mountfd points to */ + while (*path == '/') { + path++; + } + + return openat_nofollow(data->mountfd, path, flags, mode); +} + +int local_opendir_nofollow(FsContext *fs_ctx, const char *path) +{ + return local_open_nofollow(fs_ctx, path, O_DIRECTORY | O_RDONLY, 0); +} + #define VIRTFS_META_DIR ".virtfs_metadata" static char *local_mapped_attr_path(FsContext *ctx, const char *path) @@ -359,13 +378,9 @@ static int local_closedir(FsContext *ctx, V9fsFidOpenState *fs) static int local_open(FsContext *ctx, V9fsPath *fs_path, int flags, V9fsFidOpenState *fs) { - char *buffer; - char *path = fs_path->data; int fd; - buffer = rpath(ctx, path); - fd = open(buffer, flags | O_NOFOLLOW); - g_free(buffer); + fd = local_open_nofollow(ctx, fs_path->data, flags, 0); if (fd == -1) { return -1; } @@ -376,13 +391,15 @@ static int local_open(FsContext *ctx, V9fsPath *fs_path, static int local_opendir(FsContext *ctx, V9fsPath *fs_path, V9fsFidOpenState *fs) { - char *buffer; - char *path = fs_path->data; + int dirfd; DIR *stream; - buffer = rpath(ctx, path); - stream = opendir(buffer); - g_free(buffer); + dirfd = local_opendir_nofollow(ctx, fs_path->data); + if (dirfd == -1) { + return -1; + } + + stream = fdopendir(dirfd); if (!stream) { return -1; } diff --git a/hw/9pfs/9p-local.h b/hw/9pfs/9p-local.h new file mode 100644 index 000000000000..32c72749d9df --- /dev/null +++ b/hw/9pfs/9p-local.h @@ -0,0 +1,20 @@ +/* + * 9p local backend 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_LOCAL_H +#define QEMU_9P_LOCAL_H + +int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, + mode_t mode); +int local_opendir_nofollow(FsContext *fs_ctx, const char *path); + +#endif
The local_open() and local_opendir() callbacks are vulnerable to symlink attacks because they call: (1) open(O_NOFOLLOW) which follows symbolic links in all path elements but the rightmost one (2) opendir() which follows symbolic links in all path elements This patch converts both callbacks to use new helpers based on openat_nofollow() to only open files and directories if they are below the virtfs shared folder This partly fixes CVE-2016-9602. Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- v2: - use LocalData type - strip leading '/' characters in local_open_nofollow() --- hw/9pfs/9p-local.c | 37 +++++++++++++++++++++++++++---------- hw/9pfs/9p-local.h | 20 ++++++++++++++++++++ 2 files changed, 47 insertions(+), 10 deletions(-) create mode 100644 hw/9pfs/9p-local.h