[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH 26/29] 9pfs: local: mknod: don't follow symlinks
From: |
Greg Kurz |
Subject: |
[Qemu-devel] [PATCH 26/29] 9pfs: local: mknod: don't follow symlinks |
Date: |
Mon, 20 Feb 2017 15:42:42 +0100 |
User-agent: |
StGit/0.17.1-20-gc0b1b-dirty |
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 fchmod()
is introduced as a replacement to local_post_create_passthrough() to fix (4).
No effort is made to factor out code because local_post_create_passthrough()
will be dropped when all users have been converted to call the new helper.
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>
---
hw/9pfs/9p-local.c | 82 +++++++++++++++++++++++++++++++---------------------
1 file changed, 49 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 9c4faae0b3a2..89357b9486e3 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -509,6 +509,37 @@ err:
return -1;
}
+static int local_set_cred_passthrough(FsContext *fs_ctx, int dirfd,
+ const char *name, FsCred *credp)
+{
+ int fd, err = -1;
+
+ 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;
+ }
+ }
+
+ fd = openat_nofollow(dirfd, name, O_RDONLY, 0);
+ if (fd == -1) {
+ return -1;
+ }
+
+ if (fchmod(fd, credp->fc_mode & 07777) < 0) {
+ goto out;
+ }
+ err = 0;
+
+out:
+ close_preserve_errno(fd);
+ return err;
+}
+
static ssize_t local_readlink(FsContext *fs_ctx, V9fsPath *fs_path,
char *buf, size_t bufsz)
{
@@ -712,61 +743,46 @@ static int local_chmod(FsContext *fs_ctx, V9fsPath
*fs_path, FsCred *credp)
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;
}
- [Qemu-devel] [PATCH 21/29] 9pfs: local: improve error handling in link op, (continued)
- [Qemu-devel] [PATCH 21/29] 9pfs: local: improve error handling in link op, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 22/29] 9pfs: local: link: don't follow symlinks, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 24/29] 9pfs: local: chown: don't follow symlinks, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 25/29] 9pfs: local: symlink: don't follow symlinks, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 26/29] 9pfs: local: mknod: don't follow symlinks,
Greg Kurz <=
- [Qemu-devel] [PATCH 27/29] 9pfs: local: mkdir: don't follow symlinks, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 28/29] 9pfs: local: open2: don't follow symlinks, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 29/29] 9pfs: local: drop unused code, Greg Kurz, 2017/02/20