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

[Greg Kurz]

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