Re: [PATCH 1/1] gnu: unrtf: Fix CVE-2016-10091.

[Leo Famulari <leo@famulari.name>]

[-- Attachment #1: Type: text/plain, Size: 3797 bytes --]

On Tue, Jan 03, 2017 at 05:49:29PM +0100, Marius Bakke wrote:
> Leo Famulari <leo@famulari.name> writes:
> > +Patch copied from Debian:
> > +
> > +https://anonscm.debian.org/cgit/collab-maint/unrtf.git/commit/?h=jessie&id=7500a48fb0fbad3ab963fb17560b2f90a8a485c8
> > +
> > +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
> 
> 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 <jf@dockes.org>
|# 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] != '\0')
                {
-                       sprintf(tmp, "%d", nr);
+                       snprintf(tmp, 20, "%d", nr);
                        strcpy(&s[j], tmp);
                        j = j + strlen(tmp);
                }

Whereas Debian's hunk is like this:

@@ -762,7 +762,7 @@ assemble_string(char *string, int nr)
                }

                if (string[i] != '\0') {
-                       sprintf(tmp, "%d", nr);
+                       snprintf(tmp, 20, "%d", nr);
                        strcpy(&s[j], tmp);
                        j = 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
> 
> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]