Hi Christian, On Tue, 06 Jun 2023 15:57:50 +0200 Christian Schoenebeck <qemu_...@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 <yws...@gmail.com> > Reported-by: Jietao Xiao <shawtao1...@gmail.com> > Reported-by: Jinku Li <j...@xidian.edu.cn> > Reported-by: Wenbo Shen <shenwe...@zju.edu.cn> > Signed-off-by: Christian Schoenebeck <qemu_...@crudebyte.com> > --- > v1 -> v2: > - Add equivalent fix for 'proxy' fs driver. > - Minor adjustments on commit log. > Note that this might be a bit confusing for reviewers since v1 was never posted to qemu-devel. Technically, this should have been posted without the v2 tag. > fsdev/virtfs-proxy-helper.c | 48 +++++++++++++++++++++++++++++++++++-- > hw/9pfs/9p-util.h | 29 ++++++++++++++++++++++ > 2 files changed, 75 insertions(+), 2 deletions(-) > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 5cafcd7703..f311519fa3 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,49 @@ 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; > + struct stat stbuf; > + > + fd = open(pathname, flags, mode); > + if (fd < 0) { > + return fd; > + } > + > + /* 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. > + */ > + if (qemu_fstat(fd, &stbuf) < 0) { > + close_preserve_errno(fd); > + return -1; > + } > + if (!S_ISREG(stbuf.st_mode) && !S_ISDIR(stbuf.st_mode)) { > + /* Tcreate and Tlcreate 9p messages mandate to immediately open the > + * created file for I/O. So this is not (necessarily) due to a broken > + * client, and hence no error message is to be reported in this case. > + */ > + if (!(flags & O_CREAT)) { Tlcreate is explicitly about creating regular files only (see [1] and v9fs_lcreate()) and I don't quite see how open() could successfully create a regular file and the resulting fd is fstat'ed as something else. Tcreate seems to cover more types but again only regular files (with O_CREAT) or directories (without O_CREAT) are expected here (see v9fs_create()). Unless I'm missing something, it seems that the comment and the O_CREAT check should be removed. [1] https://github.com/chaos/diod/blob/master/protocol.md#lcreate----create-regular-file > + 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 fd; > +} > + > /* > * send response in two parts > * 1) ProxyHeader > @@ -682,7 +726,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 +751,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..9da1a0538d 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 > @@ -118,6 +121,7 @@ static inline int openat_file(int dirfd, const char > *name, int flags, > mode_t mode) > { > int fd, serrno, ret; > + struct stat stbuf; > > #ifndef CONFIG_DARWIN > again: > @@ -142,6 +146,31 @@ again: > return -1; > } > > + /* 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. > + */ > + if (qemu_fstat(fd, &stbuf) < 0) { > + close_preserve_errno(fd); > + return -1; > + } > + if (!S_ISREG(stbuf.st_mode) && !S_ISDIR(stbuf.st_mode)) { > + /* Tcreate and Tlcreate 9p messages mandate to immediately open the > + * created file for I/O. So this is not (necessarily) due to a broken > + * client, and hence no error message is to be reported in this case. > + */ Same remark as with the proxy helper. If you agree with my suggestions, feel free to add my R-b right away. Cheers, -- Greg > + if (!(flags & O_CREAT)) { > + 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; > + } > + > 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()