From patchwork Thu Jan 21 14:44:29 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Hajnoczi X-Patchwork-Id: 12036429 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.8 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MIME_BASE64_TEXT,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37D5EC433E0 for ; Thu, 21 Jan 2021 14:46:10 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8579622248 for ; Thu, 21 Jan 2021 14:46:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8579622248 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:46862 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1l2bDs-0001a2-KD for qemu-devel@archiver.kernel.org; Thu, 21 Jan 2021 09:46:08 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:41186) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1l2bCa-0000uE-F2 for qemu-devel@nongnu.org; Thu, 21 Jan 2021 09:44:48 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:49934) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.90_1) (envelope-from ) id 1l2bCX-00040P-5O for qemu-devel@nongnu.org; Thu, 21 Jan 2021 09:44:47 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611240283; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=9J/InfNPqwFuzr3M/NZ9MCR+nAkLgCHleGdJTnagcPo=; b=DbSjcVol1cE8rtWS9CghqHhJYSN3lN+kI4cETQoMjIQIWUkmJRVjYdAb9hWHHrNhLY99Ud Kf12uBvwRNc9Qn906xmwlyVkrDhIwQhPAKufHLcXEQ5Fm0GYEieUR55tp93cBzROvtW0tA XbF0akumX8y0itvphKiZQxsqCVrOBeE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-181-whaKCRs0O6yggLfwDCp-Sw-1; Thu, 21 Jan 2021 09:44:41 -0500 X-MC-Unique: whaKCRs0O6yggLfwDCp-Sw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 14B801800D41; Thu, 21 Jan 2021 14:44:40 +0000 (UTC) Received: from localhost (ovpn-115-60.ams2.redhat.com [10.36.115.60]) by smtp.corp.redhat.com (Postfix) with ESMTP id D99C539A60; Thu, 21 Jan 2021 14:44:30 +0000 (UTC) From: Stefan Hajnoczi To: qemu-devel@nongnu.org Subject: [PATCH] virtiofsd: prevent opening of special files (CVE-2020-35517) Date: Thu, 21 Jan 2021 14:44:29 +0000 Message-Id: <20210121144429.58885-1-stefanha@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=stefanha@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Received-SPF: pass client-ip=216.205.24.124; envelope-from=stefanha@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -11 X-Spam_score: -1.2 X-Spam_bar: - X-Spam_report: (-1.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.168, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, MIME_BASE64_TEXT=1.741, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mszeredi@redhat.com, slp@redhat.com, "Dr. David Alan Gilbert" , P J P , virtio-fs@redhat.com, Alex Xu , Stefan Hajnoczi , vgoyal@redhat.com Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" A well-behaved FUSE client does not attempt to open special files with FUSE_OPEN because they are handled on the client side (e.g. device nodes are handled by client-side device drivers). The check to prevent virtiofsd from opening special files is missing in a few cases, most notably FUSE_OPEN. A malicious client can cause virtiofsd to open a device node, potentially allowing the guest to escape. This can be exploited by a modified guest device driver. It is not exploitable from guest userspace since the guest kernel will handle special files inside the guest instead of sending FUSE requests. This patch adds the missing checks to virtiofsd. This is a short-term solution because it does not prevent a compromised virtiofsd process from opening device nodes on the host. Reported-by: Alex Xu Fixes: CVE-2020-35517 Signed-off-by: Stefan Hajnoczi Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Vivek Goyal --- This issue was diagnosed on public IRC and is therefore already known and not embargoed. A stronger fix, and the long-term solution, is for users to mount the shared directory and any sub-mounts with nodev, as well as nosuid and noexec. Unfortunately virtiofsd cannot do this automatically because bind mounts added by the user after virtiofsd has launched would not be detected. I suggest the following: 1. Modify libvirt and Kata Containers to explicitly set these mount options. 2. Then modify virtiofsd to check that the shared directory has the necessary options at startup. Refuse to start if the options are missing so that the user is aware of the security requirements. As a bonus this also increases the likelihood that other host processes besides virtiofsd will be protected by nosuid/noexec/nodev so that a malicious guest cannot drop these files in place and then arrange for a host process to come across them. Additionally, user namespaces have been discussed. They seem like a worthwhile addition as an unprivileged or privilege-separated mode although there are limitations with respect to security xattrs and the actual uid/gid stored on the host file system not corresponding to the guest uid/gid. --- tools/virtiofsd/passthrough_ll.c | 84 +++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 28 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 5fb36d9407..e08352d649 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -555,6 +555,29 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino) return fd; } +/* + * Open a file descriptor for an inode. Returns -EBADF if the inode is not a + * regular file or a directory. Use this helper function instead of raw + * openat(2) to prevent security issues when a malicious client opens special + * files such as block device nodes. + */ +static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode, + int open_flags) +{ + g_autofree char *fd_str = g_strdup_printf("%d", inode->fd); + int fd; + + if (!S_ISREG(inode->filetype) && !S_ISDIR(inode->filetype)) { + return -EBADF; + } + + fd = openat(lo->proc_self_fd, fd_str, open_flags); + if (fd < 0) { + return -errno; + } + return fd; +} + static void lo_init(void *userdata, struct fuse_conn_info *conn) { struct lo_data *lo = (struct lo_data *)userdata; @@ -684,8 +707,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr, if (fi) { truncfd = fd; } else { - sprintf(procname, "%i", ifd); - truncfd = openat(lo->proc_self_fd, procname, O_RDWR); + truncfd = lo_inode_open(lo, inode, O_RDWR); if (truncfd < 0) { goto out_err; } @@ -1725,7 +1747,6 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo, pid_t pid, int *err) { struct lo_inode_plock *plock; - char procname[64]; int fd; plock = @@ -1742,12 +1763,10 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo, } /* Open another instance of file which can be used for ofd locks. */ - sprintf(procname, "%i", inode->fd); - /* TODO: What if file is not writable? */ - fd = openat(lo->proc_self_fd, procname, O_RDWR); - if (fd == -1) { - *err = errno; + fd = lo_inode_open(lo, inode, O_RDWR); + if (fd < 0) { + *err = -fd; free(plock); return NULL; } @@ -1894,18 +1913,24 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) { int fd; ssize_t fh; - char buf[64]; struct lo_data *lo = lo_data(req); + struct lo_inode *inode = lo_inode(req, ino); fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino, fi->flags); + if (!inode) { + fuse_reply_err(req, EBADF); + return; + } + update_open_flags(lo->writeback, lo->allow_direct_io, fi); - sprintf(buf, "%i", lo_fd(req, ino)); - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW); - if (fd == -1) { - return (void)fuse_reply_err(req, errno); + fd = lo_inode_open(lo, inode, fi->flags & ~O_NOFOLLOW); + if (fd < 0) { + lo_inode_put(lo, &inode); + fuse_reply_err(req, -fd); + return; } pthread_mutex_lock(&lo->mutex); @@ -1913,6 +1938,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) pthread_mutex_unlock(&lo->mutex); if (fh == -1) { close(fd); + lo_inode_put(lo, &inode); fuse_reply_err(req, ENOMEM); return; } @@ -1923,6 +1949,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) } else if (lo->cache == CACHE_ALWAYS) { fi->keep_cache = 1; } + lo_inode_put(lo, &inode); fuse_reply_open(req, fi); } @@ -1982,39 +2009,40 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync, struct fuse_file_info *fi) { + struct lo_inode *inode = lo_inode(req, ino); + struct lo_data *lo = lo_data(req); int res; int fd; - char *buf; fuse_log(FUSE_LOG_DEBUG, "lo_fsync(ino=%" PRIu64 ", fi=0x%p)\n", ino, (void *)fi); + if (!inode) { + fuse_reply_err(req, EBADF); + return; + } + if (!fi) { - struct lo_data *lo = lo_data(req); - - res = asprintf(&buf, "%i", lo_fd(req, ino)); - if (res == -1) { - return (void)fuse_reply_err(req, errno); - } - - fd = openat(lo->proc_self_fd, buf, O_RDWR); - free(buf); - if (fd == -1) { - return (void)fuse_reply_err(req, errno); + fd = lo_inode_open(lo, inode, O_RDWR); + if (fd < 0) { + res = -fd; + goto out; } } else { fd = lo_fi_fd(req, fi); } if (datasync) { - res = fdatasync(fd); + res = fdatasync(fd) == -1 ? errno : 0; } else { - res = fsync(fd); + res = fsync(fd) == -1 ? errno : 0; } if (!fi) { close(fd); } - fuse_reply_err(req, res == -1 ? errno : 0); +out: + lo_inode_put(lo, &inode); + fuse_reply_err(req, res); } static void lo_read(fuse_req_t req, fuse_ino_t ino, size_t size, off_t offset,