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

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

[Michael Tokarev]

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
---
 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

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

[Daniel P. Berrangé]

On Wed, Sep 01, 2021 at 04:16:24PM +0300, 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.
> 
> 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Marc-André Lureau]

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

On Wed, Sep 1, 2021 at 5:16 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
> ---
>  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
>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

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

[Marc-André Lureau]

[-- 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 --]

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

[Michael Tokarev]

03.09.2021 19:04, Marc-André Lureau wrote:
[qemu-sockets.c unix path copy fix]

> Daniel or Michael, or someone else queued this already?

Nope, at least not me. I can send a pull request with a
single fix. Is it okay?

/mjt

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

[Philippe Mathieu-Daudé]

On 9/6/21 1:25 PM, Michael Tokarev wrote:
> 03.09.2021 19:04, Marc-André Lureau wrote:
> [qemu-sockets.c unix path copy fix]
> 
>> Daniel or Michael, or someone else queued this already?
> 
> Nope, at least not me. I can send a pull request with a
> single fix. Is it okay?

Certainly, but you could also pick the latest patches
sent to qemu-trivial@ already reviewed ;)

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

[Michael Tokarev]

06.09.2021 14:34, Philippe Mathieu-Daudé wrote:

> Certainly, but you could also pick the latest patches
> sent to qemu-trivial@ already reviewed ;)

I haven't done this in years..

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

[Philippe Mathieu-Daudé]

On 9/6/21 1:39 PM, Michael Tokarev wrote:
> 06.09.2021 14:34, Philippe Mathieu-Daudé wrote:
> 
>> Certainly, but you could also pick the latest patches
>> sent to qemu-trivial@ already reviewed ;)
> 
> I haven't done this in years..

Not sure what that means... you are still listed as
maintainer:

Trivial patches
---------------
Trivial patches
M: Michael Tokarev <mjt@tls.msk.ru>
M: Laurent Vivier <laurent@vivier.eu>
S: Maintained
L: qemu-trivial@nongnu.org

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