Re: [PATCH v3] qemu-sockets: fix unix socket path copy (again)

[Stefan Reiter]

Thanks for the patch, ran into the same issue here and was
about to send my own ;)

On 9/1/21 3:16 PM, Michael Tokarev wrote:
> Commit 4cfd970ec188558daa6214f26203fe553fb1e01f added an
> assert which ensures the path within an address of a unix
> socket returned from the kernel is at least one byte and
> does not exceed sun_path buffer. Both of this constraints
> are wrong:
>
> A unix socket can be unnamed, in this case the path is
> completely empty (not even \0)
>
> And some implementations (notable linux) can add extra
> trailing byte (\0) _after_ the sun_path buffer if we
> passed buffer larger than it (and we do).
>
> So remove the assertion (since it causes real-life breakage)
> but at the same time fix the usage of sun_path. Namely,
> we should not access sun_path[0] if kernel did not return
> it at all (this is the case for unnamed sockets),
> and use the returned salen when copyig actual path as an
> upper constraint for the amount of bytes to copy - this
> will ensure we wont exceed the information provided by
> the kernel, regardless whenever there is a trailing \0
> or not. This also helps with unnamed sockets.
>
> Note the case of abstract socket, the sun_path is actually
> a blob and can contain \0 characters, - it should not be
> passed to g_strndup and the like, it should be accessed by
> memcpy-like functions.

I know this is already applied now, but I noticed that you
don't actually follow through on that part - g_strndup is
still used in both cases for sun_path.

Haven't run into a bug with this, just noting that the
commit message is a bit confusing paired with the patch.

Might be a bit tricky though, as all callers would need to
be careful too...

> Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f
> Fixes: http://bugs.debian.org/993145
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> CC: qemu-stable@nongnu.org
> ---
>   util/qemu-sockets.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index f2f3676d1f..c5043999e9 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,25 +1345,22 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage 
> *sa,
>       SocketAddress *addr;
>       struct sockaddr_un *su = (struct sockaddr_un *)sa;
>   
> -    assert(salen >= sizeof(su->sun_family) + 1 &&
> -           salen <= sizeof(struct sockaddr_un));
> -
>       addr = g_new0(SocketAddress, 1);
>       addr->type = SOCKET_ADDRESS_TYPE_UNIX;
> +    salen -= offsetof(struct sockaddr_un, sun_path);
>   #ifdef CONFIG_LINUX
> -    if (!su->sun_path[0]) {
> +    if (salen > 0 && !su->sun_path[0]) {
>           /* Linux abstract socket */
> -        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> -                                        salen - sizeof(su->sun_family) - 1);
> +        addr->u.q_unix.path = g_strndup(su->sun_path + 1, salen - 1);
>           addr->u.q_unix.has_abstract = true;
>           addr->u.q_unix.abstract = true;
>           addr->u.q_unix.has_tight = true;
> -        addr->u.q_unix.tight = salen < sizeof(*su);
> +        addr->u.q_unix.tight = salen < sizeof(su->sun_path);
>           return addr;
>       }
>   #endif
>   
> -    addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
> +    addr->u.q_unix.path = g_strndup(su->sun_path, salen);
>       return addr;
>   }
>   #endif /* WIN32 */