Re: [PATCH v3] 9pfs: prevent opening special files (CVE-2023-2861)
07.06.2023 17:50, Greg Kurz wrote:
On Wed, 7 Jun 2023 15:50:01 +0200
..
+static inline int check_is_regular_file_or_dir(int fd)
+{
+struct stat stbuf;
+
+if (qemu_fstat(fd, ) < 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() ?
I realized this after sent initial comment, - my suggestion for
the name was awful. It is either check_is_regular() and close
after it failed, or it is ensure_regular_or_close().. But I
didn't sent a correction, hoping it's easy to spot the awful
suggestion.. :)
I don't like it when such a simple thing, especially when
reviewed without good care like in my case, generates so
much ping-pong.. :(
Anyway the fix is fine enough to address the CVE.
Yeah.
/mjt
Re: [PATCH v3] 9pfs: prevent opening special files (CVE-2023-2861)
On Wed, 7 Jun 2023 15:50:01 +0200
Christian Schoenebeck 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
> Reported-by: Jietao Xiao
> Reported-by: Jinku Li
> Reported-by: Wenbo Shen
> Signed-off-by: Christian Schoenebeck
> ---
> 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_fstatatfstatat
> #define qemu_mkdiratmkdirat
> #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, ) < 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
> +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;
>
[PATCH v3] 9pfs: prevent opening special files (CVE-2023-2861)
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
Reported-by: Jietao Xiao
Reported-by: Jinku Li
Reported-by: Wenbo Shen
Signed-off-by: Christian Schoenebeck
---
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_fstatatfstatat
#define qemu_mkdiratmkdirat
#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, ) < 0) {
+close_preserve_errno(fd);
+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()
--
2.30.2
