Re: [PULL 1/1] Add a feature for mapping a host unix socket to a guest tcp socket

[Peter Maydell <peter.maydell@linaro.org>]

On Sun, 5 Oct 2025 at 20:48, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> From: Viktor Kurilko <murlockkinght@gmail.com>
>
> This patch adds the ability to map a host unix socket to a guest tcp socket when
> using the slirp backend. This feature was added in libslirp version 4.7.0.
>
> A new syntax for unix socket: -hostfwd=unix:hostpath-[guestaddr]:guestport
>
> Signed-off-by: Viktor Kurilko <murlockkinght@gmail.com>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Message-ID: <20250808143904.363907-1-murlockkinght@gmail.com>
> ---

Coverity worries here about a possible time-of-check-time-of-use
bug (CID 1641394). This is a heuristic that tends to fire even
when there's no interesting attack possible, but I don't
know what this code is doing so I raise it here:

> +#if !defined(WIN32) && SLIRP_CHECK_VERSION(4, 7, 0)
> +    if (is_unix) {
> +        if (get_str_sep(buf, sizeof(buf), &p, '-') < 0) {
> +            fail_reason = "Missing - separator";
> +            goto fail_syntax;
> +        }
> +        if (buf[0] == '\0') {
> +            fail_reason = "Missing unix socket path";
> +            goto fail_syntax;
> +        }
> +        if (buf[0] != '/') {
> +            fail_reason = "unix socket path must be absolute";
> +            goto fail_syntax;
> +        }
> +
> +        size_t path_len = strlen(buf);
> +        if (path_len > sizeof(host_addr.un.sun_path) - 1) {
> +            fail_reason = "Unix socket path is too long";
> +            goto fail_syntax;
> +        }
> +
> +        struct stat st;
> +        if (stat(buf, &st) == 0) {

Coverity notes that we do a check on the filename here
with stat()...

> +            if (!S_ISSOCK(st.st_mode)) {
> +                fail_reason = "file exists and it's not unix socket";
> +                goto fail_syntax;
> +            }
> +
> +            if (unlink(buf) < 0) {

...and then later we do an unlink() if it's a unix socket.
But Coverity points out that an attacker could change what
the filename points to between the stat and the unlink,
causing us to unlink some non-socket file.

Do we care ?

> +                error_setg_errno(errp, errno, "Failed to unlink '%s'", buf);
> +                goto fail_syntax;
> +            }
> +        }
> +        host_addr.un.sun_family = AF_UNIX;
> +        memcpy(host_addr.un.sun_path, buf, path_len);
> +        host_addr_size = sizeof(host_addr.un);
> +    } else

thanks
-- PMM