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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3188 bytes --]

Hi

On Wed, Sep 1, 2021 at 5:22 PM Michael Tokarev <mjt@tls.msk.ru> 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.
>
> Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f
> Fixes: http://bugs.debian.org/993145
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> CC: qemu-stable@nongnu.org


Daniel or Michael, or someone else queued this already?

thanks


> ---
>  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 */
> --
> 2.30.2
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4278 bytes --]