From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37864) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cif3x-0001Uy-OF for qemu-devel@nongnu.org; Tue, 28 Feb 2017 05:31:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cif3u-00084C-JL for qemu-devel@nongnu.org; Tue, 28 Feb 2017 05:31:21 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:40781) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cif3u-000841-8x for qemu-devel@nongnu.org; Tue, 28 Feb 2017 05:31:18 -0500 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v1SASdST051648 for ; Tue, 28 Feb 2017 05:31:17 -0500 Received: from e06smtp05.uk.ibm.com (e06smtp05.uk.ibm.com [195.75.94.101]) by mx0a-001b2d01.pphosted.com with ESMTP id 28w3d4jxrp-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 28 Feb 2017 05:31:16 -0500 Received: from localhost by e06smtp05.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 28 Feb 2017 10:31:14 -0000 From: Greg Kurz Date: Tue, 28 Feb 2017 11:30:38 +0100 In-Reply-To: <1488277840-18608-1-git-send-email-groug@kaod.org> References: <1488277840-18608-1-git-send-email-groug@kaod.org> Message-Id: <1488277840-18608-27-git-send-email-groug@kaod.org> Subject: [Qemu-devel] [PULL 26/28] 9pfs: local: mkdir: don't follow symlinks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Peter Maydell , "Aneesh Kumar K.V" , Greg Kurz The local_mkdir() callback is vulnerable to symlink attacks because it calls: (1) mkdir() 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_mkdir() to rely on opendir_nofollow() and mkdirat() to fix (1), as well as local_set_xattrat(), local_set_mapped_file_attrat() and local_set_cred_passthrough() to fix (2), (3) and (4) respectively. 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 mkdirat(). This partly fixes CVE-2016-9602. Signed-off-by: Greg Kurz Reviewed-by: Stefan Hajnoczi --- hw/9pfs/9p-local.c | 55 ++++++++++++++++++++---------------------------------- 1 file changed, 20 insertions(+), 35 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index db70c2daf498..33893d50113c 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -799,62 +799,47 @@ out: static int local_mkdir(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 = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS); + if (fs_ctx->export_flags & V9FS_SM_MAPPED || + fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { + err = mkdirat(dirfd, name, SM_LOCAL_DIR_MODE_BITS); if (err == -1) { goto out; } - credp->fc_mode = credp->fc_mode|S_IFDIR; - 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 = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS); - if (err == -1) { - goto out; + credp->fc_mode = credp->fc_mode | S_IFDIR; + + 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); } - credp->fc_mode = credp->fc_mode|S_IFDIR; - 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 = mkdir(buffer, credp->fc_mode); + } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || + fs_ctx->export_flags & V9FS_SM_NONE) { + err = mkdirat(dirfd, name, credp->fc_mode); 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, AT_REMOVEDIR); out: - g_free(buffer); - v9fs_string_free(&fullname); + close_preserve_errno(dirfd); return err; } -- 2.7.4