[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH 25/81] 9pfs: local: mknod: don't follow symlinks
From: |
Michael Roth |
Subject: |
[Qemu-devel] [PATCH 25/81] 9pfs: local: mknod: don't follow symlinks |
Date: |
Mon, 20 Mar 2017 18:07:49 -0500 |
From: Greg Kurz <address@hidden>
The local_mknod() callback is vulnerable to symlink attacks because it
calls:
(1) mknod() which follows symbolic links for all path elements but the
rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
mkdir(), both functions following symbolic links for all path
elements but the rightmost one
(4) local_post_create_passthrough() which calls in turn lchown() and
chmod(), both functions also following symbolic links
This patch converts local_mknod() to rely on opendir_nofollow() and
mknodat() to fix (1), as well as local_set_xattrat() and
local_set_mapped_file_attrat() to fix (2) and (3) respectively.
A new local_set_cred_passthrough() helper based on fchownat() and
fchmodat_nofollow() is introduced as a replacement to
local_post_create_passthrough() to fix (4).
The mapped and mapped-file security modes are supposed to be identical,
except for the place where credentials and file modes are stored. While
here, we also make that explicit by sharing the call to mknodat().
This partly fixes CVE-2016-9602.
Signed-off-by: Greg Kurz <address@hidden>
Reviewed-by: Stefan Hajnoczi <address@hidden>
(cherry picked from commit d815e7219036d6911fce12efe3e59906264c8536)
Signed-off-by: Greg Kurz <address@hidden>
Signed-off-by: Michael Roth <address@hidden>
---
hw/9pfs/9p-local.c | 68 ++++++++++++++++++++++++++++--------------------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 2bb77e2..f71cc11 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -543,6 +543,23 @@ err:
return -1;
}
+static int local_set_cred_passthrough(FsContext *fs_ctx, int dirfd,
+ const char *name, FsCred *credp)
+{
+ if (fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
+ AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH) < 0) {
+ /*
+ * If we fail to change ownership and if we are
+ * using security model none. Ignore the error
+ */
+ if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
+ return -1;
+ }
+ }
+
+ return fchmodat_nofollow(dirfd, name, credp->fc_mode & 07777);
+}
+
static ssize_t local_readlink(FsContext *fs_ctx, V9fsPath *fs_path,
char *buf, size_t bufsz)
{
@@ -737,61 +754,46 @@ out:
static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
const char *name, FsCred *credp)
{
- char *path;
int err = -1;
- int serrno = 0;
- V9fsString fullname;
- char *buffer = NULL;
+ int dirfd;
- v9fs_string_init(&fullname);
- v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
- path = fullname.data;
+ dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+ if (dirfd == -1) {
+ return -1;
+ }
- /* Determine the security model */
- if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
- buffer = rpath(fs_ctx, path);
- err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
+ if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
+ fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+ err = mknodat(dirfd, name, SM_LOCAL_MODE_BITS | S_IFREG, 0);
if (err == -1) {
goto out;
}
- err = local_set_xattr(buffer, credp);
- if (err == -1) {
- serrno = errno;
- goto err_end;
- }
- } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
- buffer = rpath(fs_ctx, path);
- err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
- if (err == -1) {
- goto out;
+ if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+ err = local_set_xattrat(dirfd, name, credp);
+ } else {
+ err = local_set_mapped_file_attrat(dirfd, name, credp);
}
- err = local_set_mapped_file_attr(fs_ctx, path, credp);
if (err == -1) {
- serrno = errno;
goto err_end;
}
- } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
- (fs_ctx->export_flags & V9FS_SM_NONE)) {
- buffer = rpath(fs_ctx, path);
- err = mknod(buffer, credp->fc_mode, credp->fc_rdev);
+ } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
+ fs_ctx->export_flags & V9FS_SM_NONE) {
+ err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
if (err == -1) {
goto out;
}
- err = local_post_create_passthrough(fs_ctx, path, credp);
+ err = local_set_cred_passthrough(fs_ctx, dirfd, name, credp);
if (err == -1) {
- serrno = errno;
goto err_end;
}
}
goto out;
err_end:
- remove(buffer);
- errno = serrno;
+ unlinkat_preserve_errno(dirfd, name, 0);
out:
- g_free(buffer);
- v9fs_string_free(&fullname);
+ close_preserve_errno(dirfd);
return err;
}
--
2.7.4
- [Qemu-devel] [PATCH 13/81] 9pfs: local: utimensat: don't follow symlinks, (continued)
- [Qemu-devel] [PATCH 13/81] 9pfs: local: utimensat: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 14/81] 9pfs: local: statfs: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 15/81] 9pfs: local: truncate: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 12/81] 9pfs: local: remove: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 19/81] 9pfs: local: rename: use renameat, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 17/81] 9pfs: local: lstat: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 16/81] 9pfs: local: readlink: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 21/81] 9pfs: local: link: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 18/81] 9pfs: local: renameat: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 29/81] 9pfs: fix bogus fd check in local_remove(), Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 25/81] 9pfs: local: mknod: don't follow symlinks,
Michael Roth <=
- [Qemu-devel] [PATCH 01/81] 9pfs: local: move xattr security ops to 9p-xattr.c, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 20/81] 9pfs: local: improve error handling in link op, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 23/81] 9pfs: local: chown: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 22/81] 9pfs: local: chmod: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 28/81] 9pfs: local: drop unused code, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 27/81] 9pfs: local: open2: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 26/81] 9pfs: local: mkdir: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 24/81] 9pfs: local: symlink: don't follow symlinks, Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 02/81] 9pfs: remove side-effects in local_init(), Michael Roth, 2017/03/20
- [Qemu-devel] [PATCH 31/81] 9pfs: fail local_statfs() earlier, Michael Roth, 2017/03/20