On Wed, Sep 1, 2021 at 5:16 PM Michael Tokarev <m...@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 <m...@tls.msk.ru> > CC: qemu-sta...@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.lur...@redhat.com>
