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

[Greg Kurz <groug@kaod.org>]

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