Re: [PATCH v2] 9pfs: prevent opening special files (CVE-2023-2861)

[Christian Schoenebeck]

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()
> 
>