From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A825CC433EF for ; Fri, 3 Sep 2021 16:17:09 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2EA0E60E77 for ; Fri, 3 Sep 2021 16:17:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 2EA0E60E77 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:49424 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mMBsK-0002qk-Ct for qemu-devel@archiver.kernel.org; Fri, 03 Sep 2021 12:17:08 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:59888) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mMBfw-00008V-P1; Fri, 03 Sep 2021 12:04:23 -0400 Received: from mail-ej1-x633.google.com ([2a00:1450:4864:20::633]:35579) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mMBfu-0007HH-EN; Fri, 03 Sep 2021 12:04:20 -0400 Received: by mail-ej1-x633.google.com with SMTP id i21so13065084ejd.2; Fri, 03 Sep 2021 09:04:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=hIGTA3O92tKzZmj48Bt5cff3kfhjrrVO2XocxUD/QaY=; b=dK2AZsZ8d6Aq2AZn8+rosYyKOQKUlAayh+/YLBcdxVP3yNiLCnkU3UGQF+18+zyUZ5 EwBzFAvYv7dGfRPacoYOhlQqyqPuQeDvpK9EceKTCD9mxYC2dAaXtXARkvFE6KJpfGEG iV6NVGIhdmUeuc5XBJYuLHr5GzmIlfXq6oHN4YUbnHcbCXiudfeLg5ql7tXed9AHROVL Lqg4Dw2kYOXLzAbXvMVtbDzjkTibKrIsIUw349b9d3i6RZgGN3SdOja3E5H0GGlsBkyu ZKHsOX14gAswAZlSzMqqx3tDYqpoJCG1SuITzgLhX8qIYEKlY75cz17dlSt1h9rYf5Fj +AOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=hIGTA3O92tKzZmj48Bt5cff3kfhjrrVO2XocxUD/QaY=; b=qEoMD+UCXM93ormZuCL9OTOBk/dFbeSbg9WBdgdOZ8zCOTdxXQk0hwSsTESHRg7MS/ 2JZB3fAZ7pAaF7+b53vHL2blrXRDoHOHEkQV0V6eyTHDnnX6vWQuynU171zdzKV78p2K KqkzI8DR0+NHq8lHxzd1sEy8/SWDeSAg1KuHZmUrJLzZG+/S/j1gIYJCboZK9BVoqJKC d0SZtLTzZ3qJ3couPVKFU779ur9OjzqtNVmNIradqUgwycnO57Q9gR65NJA7HAAbIx/l xn6MnuztPduXFqDFMh0fQDLNnJlBTcmFaGv+9LoMP5MsO/O1mvlVgYLR26UM7iWhufrp Ybkg== X-Gm-Message-State: AOAM531dybXIjOLOSrC+5ih4vpuCtNf/K1XHp0a9qfgs5OY4pXhlDmGO gmgFRctjUdZMmcTgvUGzlXTwN8k2/Zb8c3Dok8s= X-Google-Smtp-Source: ABdhPJyzLILnOgCsO6ON3yz2qeuqwSDYArDBLbm7N+d4I2JjOKZvYbDF0XhQI6Ex9ZK5+pD8yRFL/PLy3eA6lPQBWbE= X-Received: by 2002:a17:906:4fd6:: with SMTP id i22mr5099678ejw.92.1630685055533; Fri, 03 Sep 2021 09:04:15 -0700 (PDT) MIME-Version: 1.0 References: <20210901131624.46171-1-mjt@msgid.tls.msk.ru> In-Reply-To: <20210901131624.46171-1-mjt@msgid.tls.msk.ru> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Fri, 3 Sep 2021 20:04:03 +0400 Message-ID: Subject: Re: [PATCH v3] qemu-sockets: fix unix socket path copy (again) To: Michael Tokarev Content-Type: multipart/alternative; boundary="00000000000079c92105cb197323" Received-SPF: pass client-ip=2a00:1450:4864:20::633; envelope-from=marcandre.lureau@gmail.com; helo=mail-ej1-x633.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , =?UTF-8?Q?Daniel_P_=2E_Berrang=C3=A9?= , QEMU , qemu-stable Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000079c92105cb197323 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Wed, Sep 1, 2021 at 5:22 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. > > Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f > Fixes: http://bugs.debian.org/993145 > Signed-off-by: Michael Tokarev > 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 =3D (struct sockaddr_un *)sa; > > - assert(salen >=3D sizeof(su->sun_family) + 1 && > - salen <=3D sizeof(struct sockaddr_un)); > - > addr =3D g_new0(SocketAddress, 1); > addr->type =3D SOCKET_ADDRESS_TYPE_UNIX; > + salen -=3D 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 =3D g_strndup(su->sun_path + 1, > - salen - sizeof(su->sun_family) - > 1); > + addr->u.q_unix.path =3D g_strndup(su->sun_path + 1, salen - 1); > addr->u.q_unix.has_abstract =3D true; > addr->u.q_unix.abstract =3D true; > addr->u.q_unix.has_tight =3D true; > - addr->u.q_unix.tight =3D salen < sizeof(*su); > + addr->u.q_unix.tight =3D salen < sizeof(su->sun_path); > return addr; > } > #endif > > - addr->u.q_unix.path =3D g_strndup(su->sun_path, sizeof(su->sun_path)= ); > + addr->u.q_unix.path =3D g_strndup(su->sun_path, salen); > return addr; > } > #endif /* WIN32 */ > -- > 2.30.2 > > > --=20 Marc-Andr=C3=A9 Lureau --00000000000079c92105cb197323 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Wed, Sep 1, 2021 at 5:22 PM Mich= ael Tokarev <mjt@tls.msk.ru> wr= ote:
Commit 4cfd= 970ec188558daa6214f26203fe553fb1e01f 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 someo= ne else queued this already?

thanks

=

---
=C2=A0util/qemu-sockets.c | 13 +++++--------
=C2=A01 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_sto= rage *sa,
=C2=A0 =C2=A0 =C2=A0SocketAddress *addr;
=C2=A0 =C2=A0 =C2=A0struct sockaddr_un *su =3D (struct sockaddr_un *)sa;
-=C2=A0 =C2=A0 assert(salen >=3D sizeof(su->sun_family) + 1 &&= ;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0salen <=3D sizeof(struct socka= ddr_un));
-
=C2=A0 =C2=A0 =C2=A0addr =3D g_new0(SocketAddress, 1);
=C2=A0 =C2=A0 =C2=A0addr->type =3D SOCKET_ADDRESS_TYPE_UNIX;
+=C2=A0 =C2=A0 salen -=3D offsetof(struct sockaddr_un, sun_path);
=C2=A0#ifdef CONFIG_LINUX
-=C2=A0 =C2=A0 if (!su->sun_path[0]) {
+=C2=A0 =C2=A0 if (salen > 0 && !su->sun_path[0]) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Linux abstract socket */
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 addr->u.q_unix.path =3D g_strndup(su->su= n_path + 1,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 salen - = sizeof(su->sun_family) - 1);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 addr->u.q_unix.path =3D g_strndup(su->su= n_path + 1, salen - 1);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0addr->u.q_unix.has_abstract =3D true;<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0addr->u.q_unix.abstract =3D true;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0addr->u.q_unix.has_tight =3D true;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 addr->u.q_unix.tight =3D salen < sizeof(= *su);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 addr->u.q_unix.tight =3D salen < sizeof(= su->sun_path);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return addr;
=C2=A0 =C2=A0 =C2=A0}
=C2=A0#endif

-=C2=A0 =C2=A0 addr->u.q_unix.path =3D g_strndup(su->sun_path, sizeof= (su->sun_path));
+=C2=A0 =C2=A0 addr->u.q_unix.path =3D g_strndup(su->sun_path, salen)= ;
=C2=A0 =C2=A0 =C2=A0return addr;
=C2=A0}
=C2=A0#endif /* WIN32 */
--
2.30.2




--
Marc-Andr=C3=A9 Lureau
--00000000000079c92105cb197323--