| Message ID | 148760166051.31154.9036980312875397552.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 20, 2017 at 03:41:00PM +0100, Greg Kurz wrote: > + dirfd = local_opendir_nofollow(ctx, dirpath); > + if (dirfd) { > + goto out; > } > > - buffer = rpath(ctx, path); > - err = remove(buffer); > - g_free(buffer); > + if (fstatat(dirfd, path, &stbuf, AT_SYMLINK_NOFOLLOW) < 0) { > + goto err_out; > + } > + > + if (S_ISDIR(stbuf.st_mode)) { > + flags |= AT_REMOVEDIR; > + } > + > + err = local_unlinkat_common(ctx, dirfd, name, flags); The alternative is optimistically skip fstat but then do: if (err == EISDIR) { err = local_unlinkat_common(ctx, dirfd, name, flags | AT_REMOVEDIR); } It might be faster. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Thu, 23 Feb 2017 14:23:15 +0000 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Mon, Feb 20, 2017 at 03:41:00PM +0100, Greg Kurz wrote: > > + dirfd = local_opendir_nofollow(ctx, dirpath); > > + if (dirfd) { > > + goto out; > > } > > > > - buffer = rpath(ctx, path); > > - err = remove(buffer); > > - g_free(buffer); > > + if (fstatat(dirfd, path, &stbuf, AT_SYMLINK_NOFOLLOW) < 0) { > > + goto err_out; > > + } > > + > > + if (S_ISDIR(stbuf.st_mode)) { > > + flags |= AT_REMOVEDIR; > > + } > > + > > + err = local_unlinkat_common(ctx, dirfd, name, flags); > > The alternative is optimistically skip fstat but then do: > > if (err == EISDIR) { It would be err == -1 && errno == EISDIR actually. > err = local_unlinkat_common(ctx, dirfd, name, flags | AT_REMOVEDIR); > } > > It might be faster. > This would work for passthrough and mapped modes indeed. For mapped-file mode, things are more complicated though. If path points to a directory and we call local_unlinkat_common() without AT_REMOVEDIR, it won't unlink the metadata directory and unlinkat() will fail with ENOENT because the directory isn't empty... I'd rather try to optimize in a followup patch later to avoid the extra complexity. > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 21522ca7de43..c6f4c8d95442 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1016,54 +1016,32 @@ err_out: static int local_remove(FsContext *ctx, const char *path) { - int err; struct stat stbuf; - char *buffer; + char *dirpath = g_path_get_dirname(path); + char *name = g_path_get_basename(path); + int flags = 0; + int dirfd; + int err = -1; - if (ctx->export_flags & V9FS_SM_MAPPED_FILE) { - buffer = rpath(ctx, path); - err = lstat(buffer, &stbuf); - g_free(buffer); - if (err) { - goto err_out; - } - /* - * If directory remove .virtfs_metadata contained in the - * directory - */ - if (S_ISDIR(stbuf.st_mode)) { - buffer = g_strdup_printf("%s/%s/%s", ctx->fs_root, - path, VIRTFS_META_DIR); - err = remove(buffer); - g_free(buffer); - if (err < 0 && errno != ENOENT) { - /* - * We didn't had the .virtfs_metadata file. May be file created - * in non-mapped mode ?. Ignore ENOENT. - */ - goto err_out; - } - } - /* - * Now remove the name from parent directory - * .virtfs_metadata directory - */ - buffer = local_mapped_attr_path(ctx, path); - err = remove(buffer); - g_free(buffer); - if (err < 0 && errno != ENOENT) { - /* - * We didn't had the .virtfs_metadata file. May be file created - * in non-mapped mode ?. Ignore ENOENT. - */ - goto err_out; - } + dirfd = local_opendir_nofollow(ctx, dirpath); + if (dirfd) { + goto out; } - buffer = rpath(ctx, path); - err = remove(buffer); - g_free(buffer); + if (fstatat(dirfd, path, &stbuf, AT_SYMLINK_NOFOLLOW) < 0) { + goto err_out; + } + + if (S_ISDIR(stbuf.st_mode)) { + flags |= AT_REMOVEDIR; + } + + err = local_unlinkat_common(ctx, dirfd, name, flags); err_out: + close_preserve_errno(dirfd); +out: + g_free(name); + g_free(dirpath); return err; }
The local_remove() callback is vulnerable to symlink attacks because it calls: (1) lstat() which follows symbolic links in all path elements but the rightmost one (2) remove() which follows symbolic links in all path elements but the rightmost one This patch converts local_remove() to rely on opendir_nofollow(), fstatat(AT_SYMLINK_NOFOLLOW) to fix (1) and unlinkat() to fix (2). This partly fixes CVE-2016-9602. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p-local.c | 64 +++++++++++++++++----------------------------------- 1 file changed, 21 insertions(+), 43 deletions(-)