9pfs: local: unlinkat: don't follow symlinks
The local_unlinkat() callback is vulnerable to symlink attacks because it
calls remove() which follows symbolic links in all path elements but the
rightmost one.
This patch converts local_unlinkat() to rely on opendir_nofollow() and
unlinkat() instead.
Most of the code is moved to a separate local_unlinkat_common() helper
which will be reused in a subsequent patch to fix the same issue in
local_remove().
This partly fixes CVE-2016-9602.
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 2c491af..04de511 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -969,6 +969,56 @@
return ret;
}
+static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
+ int flags)
+{
+ int ret = -1;
+
+ if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+ int map_dirfd;
+
+ if (flags == AT_REMOVEDIR) {
+ int fd;
+
+ fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH);
+ if (fd == -1) {
+ goto err_out;
+ }
+ /*
+ * If directory remove .virtfs_metadata contained in the
+ * directory
+ */
+ ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR);
+ close_preserve_errno(fd);
+ if (ret < 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.
+ */
+ map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR);
+ ret = unlinkat(map_dirfd, name, 0);
+ close_preserve_errno(map_dirfd);
+ if (ret < 0 && errno != ENOENT) {
+ /*
+ * We didn't had the .virtfs_metadata file. May be file created
+ * in non-mapped mode ?. Ignore ENOENT.
+ */
+ goto err_out;
+ }
+ }
+
+ ret = unlinkat(dirfd, name, flags);
+err_out:
+ return ret;
+}
+
static int local_remove(FsContext *ctx, const char *path)
{
int err;
@@ -1118,52 +1168,15 @@
const char *name, int flags)
{
int ret;
- V9fsString fullname;
- char *buffer;
+ int dirfd;
- v9fs_string_init(&fullname);
-
- v9fs_string_sprintf(&fullname, "%s/%s", dir->data, name);
- if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
- if (flags == AT_REMOVEDIR) {
- /*
- * If directory remove .virtfs_metadata contained in the
- * directory
- */
- buffer = g_strdup_printf("%s/%s/%s", ctx->fs_root,
- fullname.data, VIRTFS_META_DIR);
- ret = remove(buffer);
- g_free(buffer);
- if (ret < 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, fullname.data);
- ret = remove(buffer);
- g_free(buffer);
- if (ret < 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, dir->data);
+ if (dirfd == -1) {
+ return -1;
}
- /* Remove the name finally */
- buffer = rpath(ctx, fullname.data);
- ret = remove(buffer);
- g_free(buffer);
-err_out:
- v9fs_string_free(&fullname);
+ ret = local_unlinkat_common(ctx, dirfd, name, flags);
+ close_preserve_errno(dirfd);
return ret;
}
