From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53167) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adaiz-0003vY-Kc for qemu-devel@nongnu.org; Wed, 09 Mar 2016 04:48:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adaiu-0001j6-K3 for qemu-devel@nongnu.org; Wed, 09 Mar 2016 04:48:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59922) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adaiu-0001j0-CS for qemu-devel@nongnu.org; Wed, 09 Mar 2016 04:48:08 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id A9C7064D05 for ; Wed, 9 Mar 2016 09:48:07 +0000 (UTC) Date: Wed, 9 Mar 2016 10:48:04 +0100 From: Kevin Wolf Message-ID: <20160309094804.GA5205@noname.redhat.com> References: <1457420446-25276-1-git-send-email-peterx@redhat.com> <1457420446-25276-2-git-send-email-peterx@redhat.com> <87pov5e5w3.fsf@blackfin.pond.sub.org> <20160308101944.GD5807@noname.str.redhat.com> <20160309025738.GF2377@pxdev.xzpeter.org> <56DF92D2.9040905@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="GvXjxJ+pjyke8COw" Content-Disposition: inline In-Reply-To: <56DF92D2.9040905@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/8] qdict: fix unbounded stack for qdict_array_entries List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, Markus Armbruster , Peter Xu , Luiz Capitulino --GvXjxJ+pjyke8COw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 09.03.2016 um 04:04 hat Eric Blake geschrieben: > On 03/08/2016 07:57 PM, Peter Xu wrote: > > On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote: > >> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben: > >>> Same arguments as for PATCH 2, except here an argument on the maximum > >>> length of subqdict would probably be easier. > >> > >> Yes, these are constant string literals in all callers, including the > >> one non-test case in quorum. > >> > >> Let's simply assert a reasonable maximum for subqdict_length. The > >> minimum we need to allow with the existing callers is 9, and I expect > >> we'll never get keys longer than 16 characters. > >=20 > > Hi, Kevin, Markus, > >=20 > > The patch should be trying to do as mentioned above. To make it > > clearer, how about the following one: > >=20 > > diff --git a/qobject/qdict.c b/qobject/qdict.c > > index 9833bd0..dde99e0 100644 > > --- a/qobject/qdict.c > > +++ b/qobject/qdict.c > > @@ -704,17 +704,16 @@ int qdict_array_entries(QDict *src, const char *s= ubqdict) > > for (i =3D 0; i < INT_MAX; i++) { > > QObject *subqobj; > > int subqdict_entries; > > - size_t slen =3D 32 + subqdict_len; > > - char indexstr[slen], prefix[slen]; > > + char indexstr[128], prefix[128]; > > size_t snprintf_ret; > >=20 > > - snprintf_ret =3D snprintf(indexstr, slen, "%s%u", subqdict, i); > > - assert(snprintf_ret < slen); > > + snprintf_ret =3D snprintf(indexstr, ARRAY_SIZE(indexstr), "%s%= u", subqdict, i); > > + assert(snprintf_ret < ARRAY_SIZE(indexstr)); >=20 > sizeof(indexstr) works, and is a bit nicer than ARRAY_SIZE() when > dealing with char. >=20 > But I'm worried that this can trigger an abort() by someone hammering on > the command line. Just because we don't expect any QMP command to > validate with a key name longer than 128 doesn't mean that we don't have > to deal with a command line with a garbage key name that long. What's > wrong with just using g_strdup_printf() and heap-allocating the result, > avoiding snprintf() and fixed lengths altogether? I can only repeat myself, we're not dealing with user data here, but with constant literal strings. Put an assert(subqdict_len < 32); at the beginning of the function and be done with it. Any violation of it is not unexpected user input, but a caller bug. > Two assertions on the snprintf_ret should make sure we are safe, > right? No, asserting after the fact that you haven't just overflown a buffer is generally not a valid way of error handling (especially if you consider that compiling with NDEBUG would make the assert disappear). Of course, the strnlen() would already avoid this, so what the assertion would really catch is string truncation. In summary, the behaviour after your patch would still be correct, but it's pointless, it's less obvious what the reason for the array size is and it wastes memory on the stack. So I wouldn't do that. Kevin --GvXjxJ+pjyke8COw Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJW3/FUAAoJEH8JsnLIjy/W0hoP+wd4UgKgLlu4qEZJelEMHYs2 DdcP8vY5NfJxSiZbyoU88Gf3wVvZfkTD91/aN5xkDJEuIEijwmMwf2UuXUJMC2n/ lZh2hQJufnbH2hxz8Ha16sUNGNJELitOz5FcUyEJY+Be9sYLxWmsgSKPnatDjuYc iNe6lHp6n3LwS4fZNtCGGcMaTE9eaGwWN04MTVEHRxr7McWKjFw3XtOMScfBl6cJ 4hGcX38qziJ4tAuqvkP4Lj7pc2n581XLAsEwo3yvtAsDbfx9qzWeMHAuOc8B6Bpj Ktsx2DX8krmo4PCOvQPE8il0irtXq2cuLCHqAHyVWiaQX8dM+XBo7LqBGiZTOOFE WPxasoBqCvKRAwgTqG+krWAfNwzJUd+sLW3TeJoIV1L0nZ/yUipScKBLKse63qBC QDI7P5aXi8iXIOcysJ5gXO0duy/XF6UFJUzV97Nmb8/TrenkIxFttF6cw6idFLaL Xne2+unt7SgRqiL6y9v5PPO3q8RQCmrWmy6bvQ4J/SF32M8QAZ2OmwSxiLIJjMTN NyOo8OWwYPUB1oL3Uj37A7kfUSs2iefY6dHFWKcJ/E7zAWlNLV7J/gDw9a//kh2G TuOZOs1JQHDNpo2nysdJsQ/VbP6KKJ1fxMXC6tf7yEvYZxHcyjLwavhc19QmLhkj QAywYMgXZRIXX7tlxTS/ =W4Wo -----END PGP SIGNATURE----- --GvXjxJ+pjyke8COw--