From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leo Famulari Subject: Re: [PATCH 1/1] gnu: unrtf: Fix CVE-2016-10091. Date: Wed, 4 Jan 2017 02:13:25 -0500 Message-ID: <20170104071325.GA8103@jasmine> References: <049f6fc2d37899df14579e04092582e3382489d5.1483302566.git.leo@famulari.name> <8760lwqeau.fsf@kirby.i-did-not-set--mail-host-address--so-tickle-me> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="EVF5PPMfhYS0aIcm" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:32979) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cOflM-0005YC-UI for guix-devel@gnu.org; Wed, 04 Jan 2017 02:13:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cOflJ-0003R6-Mi for guix-devel@gnu.org; Wed, 04 Jan 2017 02:13:32 -0500 Received: from out2-smtp.messagingengine.com ([66.111.4.26]:48977) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cOflJ-0003QS-2a for guix-devel@gnu.org; Wed, 04 Jan 2017 02:13:29 -0500 Content-Disposition: inline In-Reply-To: <8760lwqeau.fsf@kirby.i-did-not-set--mail-host-address--so-tickle-me> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Marius Bakke Cc: guix-devel@gnu.org --EVF5PPMfhYS0aIcm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jan 03, 2017 at 05:49:29PM +0100, Marius Bakke wrote: > Leo Famulari writes: > > +Patch copied from Debian: > > + > > +https://anonscm.debian.org/cgit/collab-maint/unrtf.git/commit/?h=3Djes= sie&id=3D7500a48fb0fbad3ab963fb17560b2f90a8a485c8 > > + > > +The Debian patch adapts this upstream commit so that it can be applied > > +to the 0.21.9 release tarball: > > + > > +http://hg.savannah.gnu.org/hgweb/unrtf/rev/3b16893a6406 >=20 > Isn't the Debian patch the same as this upstream commit? I can't spot > the difference with a cursory glance. The upstream patch mostly fails to apply: ------ can't find file to patch at input line 12 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- | |# HG changeset patch |# User Jean-Francois Dockes |# Date 1483180555 -3600 |# Node ID 3b16893a6406b548ff9c3a5966b47e11773eb671 |# Parent f37dd4dea9c1b97e2563759b1164573d42d679dd |Replace all instances of sprintf with snprintf and adjust size of integer = field in some cases | |diff --git a/Windows/unrtf_w.c b/Windows/unrtf_w.c |--- a/Windows/unrtf_w.c |+++ b/Windows/unrtf_w.c -------------------------- No file to patch. Skipping patch. 1 out of 1 hunk ignored patching file src/attr.c Hunk #1 succeeded at 746 (offset -218 lines). Hunk #2 FAILED at 986. 1 out of 2 hunks FAILED -- saving rejects to file src/attr.c.rej patching file src/convert.c Hunk #1 succeeded at 472 with fuzz 2 (offset -83 lines). Hunk #2 FAILED at 1383. Hunk #3 FAILED at 1412. Hunk #4 FAILED at 1441. Hunk #5 FAILED at 1630. Hunk #6 FAILED at 1687. Hunk #7 FAILED at 1855. Hunk #8 FAILED at 1882. Hunk #9 succeeded at 1403 (offset -488 lines). Hunk #10 FAILED at 1908. Hunk #11 succeeded at 1976 with fuzz 2 (offset -585 lines). Hunk #12 FAILED at 2590. Hunk #13 FAILED at 4214. 10 out of 13 hunks FAILED -- saving rejects to file src/convert.c.rej patching file src/output.c Hunk #1 FAILED at 402. Hunk #2 FAILED at 629. 2 out of 2 hunks FAILED -- saving rejects to file src/output.c.rej source is under 'unrtf-0.21.9' ------ Based on my comparison of the respective patches from UnRTF and Debian, I think there have been widespread stylistic changes in the upstream code. For example, the upstream patch has this hunk in 'src/attr.c', which fails to apply to the tarball: @@ -986,7 +986,7 @@ if (string[i] !=3D '\0') { - sprintf(tmp, "%d", nr); + snprintf(tmp, 20, "%d", nr); strcpy(&s[j], tmp); j =3D j + strlen(tmp); } Whereas Debian's hunk is like this: @@ -762,7 +762,7 @@ assemble_string(char *string, int nr) } if (string[i] !=3D '\0') { - sprintf(tmp, "%d", nr); + snprintf(tmp, 20, "%d", nr); strcpy(&s[j], tmp); j =3D j + strlen(tmp); } The conditional's opening bracket has been moved. There are lots of these issues in the upstream patch. It's not really a "bugfix patch" but merely the upstream commit that happens to fix this bug. > > +diff --git a/debian/patches/series b/debian/patches/series > > +new file mode 100644 > > +index 0000000..7868249 > > +--- /dev/null > > ++++ b/debian/patches/series > > +@@ -0,0 +1 @@ > > ++0001-Replace-all-instances-of-sprintf-with-snprintf-and-a.patch >=20 > This part we surely don't need ;-) Oops! > Unless the Debian patch fixes other issues than upstream revision > 3b16893a6406 I would just pick and link to that, skipping the Debian > step. WDYT? I think we should use Debian's patch. --EVF5PPMfhYS0aIcm Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEsFFZSPHn08G5gDigJkb6MLrKfwgFAlhsoI8ACgkQJkb6MLrK fwiCxRAA53r8YRTOXCwQ/75XORtuolDsRo19ODrGmn5FVFNRA/2t7/P8D2jRgEaQ BuLjqJTZ5o0mX2f+eW/xooh8eAmasccgnYPA6EjZdmZqtfRwA6EuCzWYWXagnOQa OJngUTcwvOPlZbA/e5Nv5h6BECTNkbNcUWi4AOD2C+Tr68HvH+YdvuF/BEp8D1+z jQA5/KsV3vgcpYacuPOE5+fHBCMTemjXP9wS/RIHCk88dwLXDQqHp5owC62O7QIk p7hDNRrREjUd+X//2AkWu2E9MFKyDWfUoER9IGpMczMkhqutW2pdur1xLGuXS+YA XRIcvce9mZOghXngWQ3YiTNPKKgtEXrzc4iZ/g2j312VmRO8dARB/+ZqTDV70TNn +aAeQznD+05RZld96TKCJbpDHspaFKlzTc7874+P1umVt3PxxAcjx1nnA6ktsv0P oaVa70Cpjda73CAlp+RN/4XKlMvG5Ve/Ph8Nrh8UgQwXuVLGIZssNt2fyE/cgGRm +8w7+Ixhksnf3faztZ7Z0T/ZfMexwOpxHaDlEXLChKwgDgL5cMm9RNlYy7f1EGsh VqVUkDyLc/NFgEY1OUipz7BIxAkoRG0ZbVUsVZz8x1vuvFzlobfuaLdoCPyzW2rD m4x74zXyhNEa/asm5ypWPPPkIEpYOtQJ9cW7IirYobKqDdkjivo= =/NxI -----END PGP SIGNATURE----- --EVF5PPMfhYS0aIcm--