[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] 9pfs: prevent opening special files (CVE-2023-2861)
From: |
Greg Kurz |
Subject: |
Re: [PATCH v3] 9pfs: prevent opening special files (CVE-2023-2861) |
Date: |
Wed, 7 Jun 2023 16:50:59 +0200 |
On Wed, 7 Jun 2023 15:50:01 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> The 9p protocol does not specifically define how server shall behave when
> client tries to open a special file, however from security POV it does
> make sense for 9p server to prohibit opening any special file on host side
> in general. A sane Linux 9p client for instance would never attempt to
> open a special file on host side, it would always handle those exclusively
> on its guest side. A malicious client however could potentially escape
> from the exported 9p tree by creating and opening a device file on host
> side.
>
> With QEMU this could only be exploited in the following unsafe setups:
>
> - Running QEMU binary as root AND 9p 'local' fs driver AND 'passthrough'
> security model.
>
> or
>
> - Using 9p 'proxy' fs driver (which is running its helper daemon as
> root).
>
> These setups were already discouraged for safety reasons before,
> however for obvious reasons we are now tightening behaviour on this.
>
> Fixes: CVE-2023-2861
> Reported-by: Yanwu Shen <ywsPlz@gmail.com>
> Reported-by: Jietao Xiao <shawtao1125@gmail.com>
> Reported-by: Jinku Li <jkli@xidian.edu.cn>
> Reported-by: Wenbo Shen <shenwenbo@zju.edu.cn>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
> v2 -> v3:
> - Drop O_CREAT check and its comment.
> - Eliminate code duplication.
>
> fsdev/virtfs-proxy-helper.c | 26 ++++++++++++++++++++++++--
> hw/9pfs/9p-util.h | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index 5cafcd7703..256d7bfcec 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -26,6 +26,7 @@
> #include "qemu/xattr.h"
> #include "9p-iov-marshal.h"
> #include "hw/9pfs/9p-proxy.h"
> +#include "hw/9pfs/9p-util.h"
> #include "fsdev/9p-iov-marshal.h"
>
> #define PROGNAME "virtfs-proxy-helper"
> @@ -338,6 +339,27 @@ static void resetugid(int suid, int sgid)
> }
> }
>
> +/*
> + * Open regular file or directory. Attempts to open any special file are
> + * rejected.
> + *
> + * returns file descriptor or -1 on error
> + */
> +static int open_regular(const char *pathname, int flags, mode_t mode) {
> + int fd;
> +
> + fd = open(pathname, flags, mode);
> + if (fd < 0) {
> + return fd;
> + }
> +
> + if (check_is_regular_file_or_dir(fd) < 0) {
> + return -1;
> + }
> +
> + return fd;
> +}
> +
> /*
> * send response in two parts
> * 1) ProxyHeader
> @@ -682,7 +704,7 @@ static int do_create(struct iovec *iovec)
> if (ret < 0) {
> goto unmarshal_err_out;
> }
> - ret = open(path.data, flags, mode);
> + ret = open_regular(path.data, flags, mode);
> if (ret < 0) {
> ret = -errno;
> }
> @@ -707,7 +729,7 @@ static int do_open(struct iovec *iovec)
> if (ret < 0) {
> goto err_out;
> }
> - ret = open(path.data, flags);
> + ret = open_regular(path.data, flags, 0);
> if (ret < 0) {
> ret = -errno;
> }
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index c314cf381d..9b0a9e5878 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -13,6 +13,8 @@
> #ifndef QEMU_9P_UTIL_H
> #define QEMU_9P_UTIL_H
>
> +#include "qemu/error-report.h"
> +
> #ifdef O_PATH
> #define O_PATH_9P_UTIL O_PATH
> #else
> @@ -95,6 +97,7 @@ static inline int errno_to_dotl(int err) {
> #endif
>
> #define qemu_openat openat
> +#define qemu_fstat fstat
> #define qemu_fstatat fstatat
> #define qemu_mkdirat mkdirat
> #define qemu_renameat renameat
> @@ -108,6 +111,32 @@ static inline void close_preserve_errno(int fd)
> errno = serrno;
> }
>
> +/* CVE-2023-2861: Prohibit opening any special file directly on host
> + * (especially device files), as a compromised client could potentially gain
> + * access outside exported tree under certain, unsafe setups. We expect
> + * client to handle I/O on special files exclusively on guest side.
> + */
> +static inline int check_is_regular_file_or_dir(int fd)
> +{
> + struct stat stbuf;
> +
> + if (qemu_fstat(fd, &stbuf) < 0) {
> + close_preserve_errno(fd);
Maybe worth to mention somewhere that this function not only
checks but also closes the fd if it doesn't point to a regular
file or directory. Or maybe change the name, e.g.
filter_out_special_files() ?
Anyway the fix is fine enough to address the CVE.
Reviewed-by: Greg Kurz <groug@kaod.org>
> + return -1;
> + }
> + if (!S_ISREG(stbuf.st_mode) && !S_ISDIR(stbuf.st_mode)) {
> + error_report_once(
> + "9p: broken or compromised client detected; attempt to open "
> + "special file (i.e. neither regular file, nor directory)"
> + );
> + close(fd);
> + errno = ENXIO;
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> static inline int openat_dir(int dirfd, const char *name)
> {
> return qemu_openat(dirfd, name,
> @@ -142,6 +171,10 @@ again:
> return -1;
> }
>
> + if (check_is_regular_file_or_dir(fd) < 0) {
> + return -1;
> + }
> +
> serrno = errno;
> /* O_NONBLOCK was only needed to open the file. Let's drop it. We don't
> * do that with O_PATH since fcntl(F_SETFL) isn't supported, and openat()
--
Greg