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

[Michael Tokarev]

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