We test whenever the path of unix-domain socket
address is non-empty and strictly-less than
the length of the path buffer. Both these
conditions are wrong: the socket can be unnamed,
with empty path, or socket can have pathname
null-terminated _after_ the sun_path buffer,
since we provided more room when asking kernel.

So on one side, allow empty, unnamed sockets
(and adjust the test for abstract socket too -
only do that if the socket is not unnamed),
and on another side, allow path length to be
up to our own maximum, - we have up to size
of sockaddr_storage there.

While at it, fix the duplication of regular
pathname socket to not require trailing \0
(since it can be missing for unnamed sockets).

Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
Fixes: http://bugs.debian.org/993145
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Cc: qemu-stable@nongnu.org
--
Two questions.
1. Why do we store the name of the socket to start with?
2. The code in the abstract socket case should not use
   g_strndup but g_memdup instead, since the whole thing
   is a blob of the given length, not a \0-terminated string.
---
 util/qemu-sockets.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index f2f3676d1f..7c83d81792 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1345,13 +1345,20 @@ 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));
+    /* there's a corner case when trailing \0 does not fit into
+     * sockaddr_un. Compare length with sizeof(sockaddr_storage),
+     * not with sizeof(sockaddr_un), since this is what we actually
+     * provide, to ensure we had no truncation and a room for
+     * the trailing \0 which we add below.
+     * When salen == sizeof(sun_family) it is unnamed socket,
+     * and when first byte of sun_path is \0, it is abstract. */
+    assert(salen >= sizeof(su->sun_family) &&
+           salen <= sizeof(struct sockaddr_storage));
 
     addr = g_new0(SocketAddress, 1);
     addr->type = SOCKET_ADDRESS_TYPE_UNIX;
 #ifdef CONFIG_LINUX
-    if (!su->sun_path[0]) {
+    if (salen > sizeof(su->sun_family) && !su->sun_path[0]) {
         /* Linux abstract socket */
         addr->u.q_unix.path = g_strndup(su->sun_path + 1,
                                         salen - sizeof(su->sun_family) - 1);
@@ -1363,7 +1370,7 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
     }
 #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 - sizeof(su->sun_family));
     return addr;
 }
 #endif /* WIN32 */
-- 
2.30.2

Hi

On Tue, Aug 31, 2021 at 10:26 PM Michael Tokarev <mjt@tls.msk.ru> wrote:

> We test whenever the path of unix-domain socket
> address is non-empty and strictly-less than
> the length of the path buffer. Both these
> conditions are wrong: the socket can be unnamed,
> with empty path, or socket can have pathname
> null-terminated _after_ the sun_path buffer,
> since we provided more room when asking kernel.
>
> So on one side, allow empty, unnamed sockets
> (and adjust the test for abstract socket too -
> only do that if the socket is not unnamed),
> and on another side, allow path length to be
> up to our own maximum, - we have up to size
> of sockaddr_storage there.
>
> While at it, fix the duplication of regular
> pathname socket to not require trailing \0
> (since it can be missing for unnamed sockets).
>
> Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> Fixes: http://bugs.debian.org/993145
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> Cc: qemu-stable@nongnu.org
> --
> Two questions.
> 1. Why do we store the name of the socket to start with?
> 2. The code in the abstract socket case should not use
>    g_strndup but g_memdup instead, since the whole thing
>    is a blob of the given length, not a \0-terminated string.
> ---
>  util/qemu-sockets.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index f2f3676d1f..7c83d81792 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,13 +1345,20 @@ 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));
> +    /* there's a corner case when trailing \0 does not fit into
> +     * sockaddr_un. Compare length with sizeof(sockaddr_storage),
> +     * not with sizeof(sockaddr_un), since this is what we actually
> +     * provide, to ensure we had no truncation and a room for
> +     * the trailing \0 which we add below.
> +     * When salen == sizeof(sun_family) it is unnamed socket,
> +     * and when first byte of sun_path is \0, it is abstract. */
> +    assert(salen >= sizeof(su->sun_family) &&
> +           salen <= sizeof(struct sockaddr_storage));
>

Seems right to me, however there are some notes in libc bits/socket.h
/* Structure large enough to hold any socket address (with the historical
   exception of AF_UNIX).  */

And also this
https://idea.popcount.org/2019-12-06-addressing/#fn:sockaddr_storage

I must say I feel confused by those comments :) Is it large enough or not??


>      addr = g_new0(SocketAddress, 1);
>      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>  #ifdef CONFIG_LINUX
> -    if (!su->sun_path[0]) {
> +    if (salen > sizeof(su->sun_family) && !su->sun_path[0]) {
>          /* Linux abstract socket */
>          addr->u.q_unix.path = g_strndup(su->sun_path + 1,
>                                          salen - sizeof(su->sun_family) -
> 1);
> @@ -1363,7 +1370,7 @@ socket_sockaddr_to_address_unix(struct
> sockaddr_storage *sa,
>      }
>  #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 -
> sizeof(su->sun_family));
>

looks good to me otherwise

     return addr;
>  }
>  #endif /* WIN32 */
> --
> 2.30.2
>
>

31.08.2021 22:21, Marc-André Lureau wrote:
...

> Seems right to me, however there are some notes in libc bits/socket.h
> /* Structure large enough to hold any socket address (with the historical
>     exception of AF_UNIX).  */
> 
> And also this
> https://idea.popcount.org/2019-12-06-addressing/#fn:sockaddr_storage <https://idea.popcount.org/2019-12-06-addressing/#fn:sockaddr_storage>
> 
> I must say I feel confused by those comments :) Is it large enough or not??

It was my first thought too when I first saw the prototype
of this very function we're patching here:

   socket_sockaddr_to_address_unix(struct sockaddr_storage *sa, ...)

it uses sockaddr_storage and I swear I always thought sockaddr_storage
is for sockaddr_in and sockaddr_in6 but NOT for sockaddr_un.

If this is the case we're in trouble actually and all this stuff
needs serious review.

However by fact sockaddr_storage is actually LARGER than sockaddr_un.
I dunno how universal it is.

/mjt

On Tue, Aug 31, 2021 at 11:21:43PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 31, 2021 at 10:26 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> > We test whenever the path of unix-domain socket
> > address is non-empty and strictly-less than
> > the length of the path buffer. Both these
> > conditions are wrong: the socket can be unnamed,
> > with empty path, or socket can have pathname
> > null-terminated _after_ the sun_path buffer,
> > since we provided more room when asking kernel.
> >
> > So on one side, allow empty, unnamed sockets
> > (and adjust the test for abstract socket too -
> > only do that if the socket is not unnamed),
> > and on another side, allow path length to be
> > up to our own maximum, - we have up to size
> > of sockaddr_storage there.
> >
> > While at it, fix the duplication of regular
> > pathname socket to not require trailing \0
> > (since it can be missing for unnamed sockets).
> >
> > Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> > Fixes: http://bugs.debian.org/993145
> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > Cc: qemu-stable@nongnu.org
> > --
> > Two questions.
> > 1. Why do we store the name of the socket to start with?
> > 2. The code in the abstract socket case should not use
> >    g_strndup but g_memdup instead, since the whole thing
> >    is a blob of the given length, not a \0-terminated string.
> > ---
> >  util/qemu-sockets.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index f2f3676d1f..7c83d81792 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -1345,13 +1345,20 @@ 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));
> > +    /* there's a corner case when trailing \0 does not fit into
> > +     * sockaddr_un. Compare length with sizeof(sockaddr_storage),
> > +     * not with sizeof(sockaddr_un), since this is what we actually
> > +     * provide, to ensure we had no truncation and a room for
> > +     * the trailing \0 which we add below.
> > +     * When salen == sizeof(sun_family) it is unnamed socket,
> > +     * and when first byte of sun_path is \0, it is abstract. */
> > +    assert(salen >= sizeof(su->sun_family) &&
> > +           salen <= sizeof(struct sockaddr_storage));
> >
> 
> Seems right to me, however there are some notes in libc bits/socket.h
> /* Structure large enough to hold any socket address (with the historical
>    exception of AF_UNIX).  */
> 
> And also this
> https://idea.popcount.org/2019-12-06-addressing/#fn:sockaddr_storage
> 
> I must say I feel confused by those comments :) Is it large enough or not??

From 'man unix(7)'

       When  binding a socket to an address, Linux is one of the im‐
       plementations that appends a null terminator if none is  sup‐
       plied in sun_path.  In most cases this is unproblematic: when
       the socket address is retrieved, it will be one  byte  longer
       than that supplied when the socket was bound.  However, there
       is one case where confusing behavior can result: if 108  non-
       null  bytes are supplied when a socket is bound, then the ad‐
       dition of the null terminator takes the length of  the  path‐
       name  beyond sizeof(sun_path).  Consequently, when retrieving
       the socket address (for example, via accept(2)), if the input
       addrlen  argument  for  the  retrieving  call is specified as
       sizeof(struct sockaddr_un), then the returned address  struc‐
       ture won't have a null terminator in sun_path.

Essentially this is saying that if the kernel allocates the
sockaddr_un buffer, it will have intentionally *over-allocated*
by 1 byte.  So you can't assume salen <= sizeof(sockaddr_un)
nor  salen <= sizeof(sockadr_storage) unless *you* have allocated
the sockaddr buffer.

In this context of QEMU, I don't know whether the addr buffer
we dealing with was allocated by QEMU or the kernel, if the
former, then we're safe in terms of size, but cannot assume
NUL terminator.


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

On 01.09.2021 12:12, Daniel P. Berrangé wrote:
>> Seems right to me, however there are some notes in libc bits/socket.h
>> /* Structure large enough to hold any socket address (with the historical
>>     exception of AF_UNIX).  */
>>
>> And also this
>> https://idea.popcount.org/2019-12-06-addressing/#fn:sockaddr_storage
>>
>> I must say I feel confused by those comments :) Is it large enough or not??
> 
>  From 'man unix(7)'

I quoted this very place in other thread yesterday. And also included
the kernel code which actually does this.

But the question here is about different issue: is sockaddr_un larger
than sockaddr_storage?

/mjt

On Tue, 31 Aug 2021 at 19:34, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> We test whenever the path of unix-domain socket
> address is non-empty and strictly-less than
> the length of the path buffer. Both these
> conditions are wrong: the socket can be unnamed,
> with empty path, or socket can have pathname
> null-terminated _after_ the sun_path buffer,
> since we provided more room when asking kernel.
>
> So on one side, allow empty, unnamed sockets
> (and adjust the test for abstract socket too -
> only do that if the socket is not unnamed),
> and on another side, allow path length to be
> up to our own maximum, - we have up to size
> of sockaddr_storage there.
>
> While at it, fix the duplication of regular
> pathname socket to not require trailing \0
> (since it can be missing for unnamed sockets).
>
> Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> Fixes: http://bugs.debian.org/993145
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> Cc: qemu-stable@nongnu.org
> --
> Two questions.
> 1. Why do we store the name of the socket to start with?
> 2. The code in the abstract socket case should not use
>    g_strndup but g_memdup instead, since the whole thing
>    is a blob of the given length, not a \0-terminated string.
> ---
>  util/qemu-sockets.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index f2f3676d1f..7c83d81792 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,13 +1345,20 @@ 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));
> +    /* there's a corner case when trailing \0 does not fit into
> +     * sockaddr_un. Compare length with sizeof(sockaddr_storage),
> +     * not with sizeof(sockaddr_un), since this is what we actually
> +     * provide, to ensure we had no truncation and a room for
> +     * the trailing \0 which we add below.
> +     * When salen == sizeof(sun_family) it is unnamed socket,
> +     * and when first byte of sun_path is \0, it is abstract. */
> +    assert(salen >= sizeof(su->sun_family) &&
> +           salen <= sizeof(struct sockaddr_storage));

Again, why are we asserting an upper bound? We don't care here:
the representation in the SocketAddress structure has no length
limit on the path. (Conversely, we do care about the max length
when we convert from a SocketAddress to a sockaddr_un: we do this
in eg unix_connect_saddr().)

>      addr = g_new0(SocketAddress, 1);
>      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>  #ifdef CONFIG_LINUX
> -    if (!su->sun_path[0]) {
> +    if (salen > sizeof(su->sun_family) && !su->sun_path[0]) {
>          /* Linux abstract socket */
>          addr->u.q_unix.path = g_strndup(su->sun_path + 1,
>                                          salen - sizeof(su->sun_family) - 1);
> @@ -1363,7 +1370,7 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>      }
>  #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 - sizeof(su->sun_family));
>      return addr;
>  }

-- PMM

On 31.08.2021 22:47, Peter Maydell wrote:
> On Tue, 31 Aug 2021 at 19:34, Michael Tokarev <mjt@tls.msk.ru> wrote:
..
>> -    assert(salen >= sizeof(su->sun_family) + 1 &&
>> -           salen <= sizeof(struct sockaddr_un));
>> +    /* there's a corner case when trailing \0 does not fit into
>> +     * sockaddr_un. Compare length with sizeof(sockaddr_storage),
>> +     * not with sizeof(sockaddr_un), since this is what we actually
>> +     * provide, to ensure we had no truncation and a room for
>> +     * the trailing \0 which we add below.
>> +     * When salen == sizeof(sun_family) it is unnamed socket,
>> +     * and when first byte of sun_path is \0, it is abstract. */
>> +    assert(salen >= sizeof(su->sun_family) &&
>> +           salen <= sizeof(struct sockaddr_storage));
> 
> Again, why are we asserting an upper bound? We don't care here:
> the representation in the SocketAddress structure has no length
> limit on the path. (Conversely, we do care about the max length
> when we convert from a SocketAddress to a sockaddr_un: we do this
> in eg unix_connect_saddr().)

We have sizeof(sockaddr_storage) space there. If the kernel returned
salen greather than that, this means we received only partial address
and can't rely on it. It is like snprintf() returning more bytes than
available in the buffer - it says how much bytes NEEDED.

/mjt

On Wed, 1 Sept 2021 at 09:29, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> On 31.08.2021 22:47, Peter Maydell wrote:
> > On Tue, 31 Aug 2021 at 19:34, Michael Tokarev <mjt@tls.msk.ru> wrote:
> ..
> >> -    assert(salen >= sizeof(su->sun_family) + 1 &&
> >> -           salen <= sizeof(struct sockaddr_un));
> >> +    /* there's a corner case when trailing \0 does not fit into
> >> +     * sockaddr_un. Compare length with sizeof(sockaddr_storage),
> >> +     * not with sizeof(sockaddr_un), since this is what we actually
> >> +     * provide, to ensure we had no truncation and a room for
> >> +     * the trailing \0 which we add below.
> >> +     * When salen == sizeof(sun_family) it is unnamed socket,
> >> +     * and when first byte of sun_path is \0, it is abstract. */
> >> +    assert(salen >= sizeof(su->sun_family) &&
> >> +           salen <= sizeof(struct sockaddr_storage));
> >
> > Again, why are we asserting an upper bound? We don't care here:
> > the representation in the SocketAddress structure has no length
> > limit on the path. (Conversely, we do care about the max length
> > when we convert from a SocketAddress to a sockaddr_un: we do this
> > in eg unix_connect_saddr().)
>
> We have sizeof(sockaddr_storage) space there. If the kernel returned
> salen greather than that, this means we received only partial address
> and can't rely on it. It is like snprintf() returning more bytes than
> available in the buffer - it says how much bytes NEEDED.

I think that if we need to check that we should be checking that
at the point where we make the accept() or whatever other call
filled in the sockaddr. That is the point at which the code
either (a) knows that the buffer is guaranteed big enough and
can assert that if it likes or (b) may have some reasonable way
to deal with the failure, eg allocate a bigger buffer and retry,
and (c) it is also the point where the code knows how big the actual
buffer it passed to the kernel is and so can validly determine if
it was truncated.

We don't check that the address is not truncated in any of the
other sockaddr-type-to-SocketAddress conversion functions...

-- PMM

On 01.09.2021 12:52, Peter Maydell wrote:
> On Wed, 1 Sept 2021 at 09:29, Michael Tokarev <mjt@tls.msk.ru> wrote:
...
>> We have sizeof(sockaddr_storage) space there. If the kernel returned
>> salen greather than that, this means we received only partial address
>> and can't rely on it. It is like snprintf() returning more bytes than
>> available in the buffer - it says how much bytes NEEDED.
> 
> I think that if we need to check that we should be checking that
> at the point where we make the accept() or whatever other call
> filled in the sockaddr. That is the point at which the code
> either (a) knows that the buffer is guaranteed big enough and
> can assert that if it likes or (b) may have some reasonable way
> to deal with the failure, eg allocate a bigger buffer and retry,
> and (c) it is also the point where the code knows how big the actual
> buffer it passed to the kernel is and so can validly determine if
> it was truncated.

I don't care where it is done, as long as what is done does not
break in real-life scenario.

Originally I asked another question: WHY we ask for the socket name
in the first place, why do we need it, where do we use it?

Maybe we should answer to this one first and maybe remove whole
thing completely instead of fixing something which isn't used?

> We don't check that the address is not truncated in any of the
> other sockaddr-type-to-SocketAddress conversion functions...

Because other addresses have fixed length. If anything there does
not fit or is not equal to the expected length, the hell will break
loose for every application out there, qemu will note be the first
to look for..

/mjt

On Wed, Sep 01, 2021 at 02:45:55PM +0300, Michael Tokarev wrote:
> On 01.09.2021 12:52, Peter Maydell wrote:
> > On Wed, 1 Sept 2021 at 09:29, Michael Tokarev <mjt@tls.msk.ru> wrote:
> ...
> > > We have sizeof(sockaddr_storage) space there. If the kernel returned
> > > salen greather than that, this means we received only partial address
> > > and can't rely on it. It is like snprintf() returning more bytes than
> > > available in the buffer - it says how much bytes NEEDED.
> > 
> > I think that if we need to check that we should be checking that
> > at the point where we make the accept() or whatever other call
> > filled in the sockaddr. That is the point at which the code
> > either (a) knows that the buffer is guaranteed big enough and
> > can assert that if it likes or (b) may have some reasonable way
> > to deal with the failure, eg allocate a bigger buffer and retry,
> > and (c) it is also the point where the code knows how big the actual
> > buffer it passed to the kernel is and so can validly determine if
> > it was truncated.
> 
> I don't care where it is done, as long as what is done does not
> break in real-life scenario.
> 
> Originally I asked another question: WHY we ask for the socket name
> in the first place, why do we need it, where do we use it?

We report the address information in the monitor in several places.
We also use the address information when running SASL auth in VNC.


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