| Message ID | E1q6XIJ-0005RX-AW@lizzy.crudebyte.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] 9pfs: prevent opening special files (CVE-2023-2861) | expand |
Hi Christian, On Tue, 06 Jun 2023 15:57:50 +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> > --- > 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()
06.06.2023 16:57, Christian Schoenebeck wrote: .. > +++ b/fsdev/virtfs-proxy-helper.c > +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)) { > + 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; > +} > +++ b/hw/9pfs/9p-util.h > @@ -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. > + */ > + 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; > + } > + can't we re-use this same code used in two places, placing it into an inline function, such as is_file_regular_or_dir(fd) ? It smells like a very good candidate for implementing it in a single place.. Thanks, /mjt
On Tuesday, June 6, 2023 6:00:28 PM CEST Greg Kurz wrote: > Hi Christian, > > On Tue, 06 Jun 2023 15:57:50 +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> > > --- > > 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. I felt it wouldn't make it any better, as it might otherwise confuse those who already got the previous two patch emails. > > 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 You are right about Tlcreate, but for Tcreate 9p2000.u specifies, quote: "In addition to creating directories with DMDIR, 9P2000.u allows the creation of symlinks (DMSYMLINK), devices (DMDEVICE), named pipes (DMNAMEPIPE), and sockets (DMSOCKET)." http://ericvh.github.io/9p-rfc/rfc9p2000.u.html#anchor17 So I just remove mentioning Tlcreate in the comment? > > > + 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, I'll definitely take the time for another (v3) round in this case. Thanks! > -- > 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() > >
On Tuesday, June 6, 2023 8:48:49 PM CEST Michael Tokarev wrote: > 06.06.2023 16:57, Christian Schoenebeck wrote: [...] > > + /* 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)) { > > + 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; > > + } > > + > > can't we re-use this same code used in two places, placing it into an inline > function, such as is_file_regular_or_dir(fd) ? It smells like a very good > candidate for implementing it in a single place.. Yeah, my plan was to officially deprecate 9p proxy subsequently, so I didn't care too much about code duplication, but I guess you are right, it is simple enough to do it right. Best regards, Christian Schoenebeck
On Wed, 07 Jun 2023 13:02:17 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Tuesday, June 6, 2023 6:00:28 PM CEST Greg Kurz wrote: > > Hi Christian, > > > > On Tue, 06 Jun 2023 15:57:50 +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> > > > --- > > > 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. > > I felt it wouldn't make it any better, as it might otherwise confuse those who > already got the previous two patch emails. > No big deal. > > > 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 > > You are right about Tlcreate, but for Tcreate 9p2000.u specifies, quote: > > "In addition to creating directories with DMDIR, 9P2000.u allows the creation > of symlinks (DMSYMLINK), devices (DMDEVICE), named pipes (DMNAMEPIPE), and > sockets (DMSOCKET)." > > http://ericvh.github.io/9p-rfc/rfc9p2000.u.html#anchor17 > > So I just remove mentioning Tlcreate in the comment? > Well, Tcreate indeed allows to create other file types but in the end the only ones that result in a call to open() are directories and regular files. Regular files are created and open atomically with v9fs_co_open2() just like with Tlcreate. Directories are created with v9fs_co_mkdir() and open with v9fs_co_opendir(), hence two distinct thread-pool jobs. This means that open() isn't passed O_CREAT in this case, but we still have to check since rogue code could have replaced the directory with something else (the current patch wouldn't detect that a directory was replaced with a regular file BTW). > > > > > + 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, > > I'll definitely take the time for another (v3) round in this case. Thanks! > > > -- > > 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() > > > > > >
On Wednesday, June 7, 2023 1:02:17 PM CEST Christian Schoenebeck wrote: > On Tuesday, June 6, 2023 6:00:28 PM CEST Greg Kurz wrote: > > Hi Christian, > > > > On Tue, 06 Jun 2023 15:57:50 +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> > > > --- > > > 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. > > I felt it wouldn't make it any better, as it might otherwise confuse those who > already got the previous two patch emails. > > > > 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 > > You are right about Tlcreate, but for Tcreate 9p2000.u specifies, quote: > > "In addition to creating directories with DMDIR, 9P2000.u allows the creation > of symlinks (DMSYMLINK), devices (DMDEVICE), named pipes (DMNAMEPIPE), and > sockets (DMSOCKET)." > > http://ericvh.github.io/9p-rfc/rfc9p2000.u.html#anchor17 > > So I just remove mentioning Tlcreate in the comment? Scratch that. In case of 9P2000.u file types are distinguished in v9fs_create() and mknod() called accordingly instead. So the check and comment can safely be removed as suggested by you. I'll prepare v3 now. > > > > > > + 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, > > I'll definitely take the time for another (v3) round in this case. Thanks! > > > -- > > 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() > > > > > > > >
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)) { + 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. + */ + 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()
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> --- v1 -> v2: - Add equivalent fix for 'proxy' fs driver. - Minor adjustments on commit log. fsdev/virtfs-proxy-helper.c | 48 +++++++++++++++++++++++++++++++++++-- hw/9pfs/9p-util.h | 29 ++++++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-)