diff mbox

Expat regression fix for master branch

Message ID 20160912213515.GA15911@jasmine
State New
Headers show

Commit Message

Leo Famulari Sept. 12, 2016, 9:35 p.m. UTC
This patch applies an upstream patch for a regression caused by the fix 
for CVE-2016-0718.

Apparently, the bug only manifests when building with -DXML_UNICODE,
which I don't think our package does.

Bug report:
https://sourceforge.net/p/expat/bugs/539/

Source of patch:
https://sourceforge.net/p/expat/code_git/ci/af507cef2c93cb8d40062a0abe43a4f4e9158fb2
From 14abba35d1b8c455ce597b5c0eed4bcf87499e6a Mon Sep 17 00:00:00 2001
From: Leo Famulari <leo@famulari.name>
Date: Mon, 12 Sep 2016 16:54:45 -0400
Subject: [PATCH] gnu: expat: Fix regression caused by fix for CVE-2016-0718.

* gnu/packages/xml.scm (expat)[replacement]: New field.
(expat/fixed): New variable.
* gnu/packages/patches/expat-CVE-2016-0718-fix-regression.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
---
 gnu/local.mk                                       |  1 +
 .../expat-CVE-2016-0718-fix-regression.patch       | 35 ++++++++++++++++++++++
 gnu/packages/xml.scm                               | 12 ++++++++
 3 files changed, 48 insertions(+)
 create mode 100644 gnu/packages/patches/expat-CVE-2016-0718-fix-regression.patch

Comments

Ludovic Courtès Sept. 13, 2016, 9:54 p.m. UTC | #1
Hello,

Leo Famulari <leo@famulari.name> skribis:

> This patch applies an upstream patch for a regression caused by the fix 
> for CVE-2016-0718.
>
> Apparently, the bug only manifests when building with -DXML_UNICODE,
> which I don't think our package does.

I rebuilt it with --check and can confirm that XML_UNICODE is not
defined.

‘README’ mentions it:

--8<---------------cut here---------------start------------->8---
If you are interested in building Expat to provide document
information in UTF-16 encoding rather than the default UTF-8, follow
these instructions (after having run "make distclean"):

        1. For UTF-16 output as unsigned short (and version/error
           strings as char), run:

               ./configure CPPFLAGS=-DXML_UNICODE
--8<---------------cut here---------------end--------------->8---

It’s enough to have the patch in ‘core-updates’ only, but I could go
either way.  WDYT?

Ludo’.
Leo Famulari Sept. 14, 2016, 6:14 p.m. UTC | #2
On Tue, Sep 13, 2016 at 11:54:09PM +0200, Ludovic Courtès wrote:
> It’s enough to have the patch in ‘core-updates’ only, but I could go
> either way.  WDYT?

I could go either way, too. It depends on how much we want to support
use of Guix outside of our package tree.

At least we have made the issue public here on the mailing list, and
provided a patch for whoever needs it.

Does anyone else have an opinion about this?
Leo Famulari Sept. 25, 2016, 11:18 p.m. UTC | #3
On Mon, Sep 12, 2016 at 05:35:15PM -0400, Leo Famulari wrote:
> This patch applies an upstream patch for a regression caused by the fix 
> for CVE-2016-0718.
> 
> Apparently, the bug only manifests when building with -DXML_UNICODE,
> which I don't think our package does.

Sebastian Pipping (the Expat maintainer) contacted me to recommend that
we apply the patch on the master branch.

He says that the faulty code path can be reached even when XML_UNICODE
is not defined. Apparently, building with -DXML_UNICODE merely makes it
easier to reach the faulty code.

I think we should take Sebastian's advice. What does everyone think?
Efraim Flashner Sept. 26, 2016, 8:21 a.m. UTC | #4
On Sun, Sep 25, 2016 at 07:18:11PM -0400, Leo Famulari wrote:
> On Mon, Sep 12, 2016 at 05:35:15PM -0400, Leo Famulari wrote:
> > This patch applies an upstream patch for a regression caused by the fix 
> > for CVE-2016-0718.
> > 
> > Apparently, the bug only manifests when building with -DXML_UNICODE,
> > which I don't think our package does.
> 
> Sebastian Pipping (the Expat maintainer) contacted me to recommend that
> we apply the patch on the master branch.
> 
> He says that the faulty code path can be reached even when XML_UNICODE
> is not defined. Apparently, building with -DXML_UNICODE merely makes it
> easier to reach the faulty code.
> 
> I think we should take Sebastian's advice. What does everyone think?


Seems to me that as the Expat maintainer he would know best.
Leo Famulari Sept. 27, 2016, 5:50 p.m. UTC | #5
On Mon, Sep 26, 2016 at 11:21:07AM +0300, Efraim Flashner wrote:
> On Sun, Sep 25, 2016 at 07:18:11PM -0400, Leo Famulari wrote:
> > On Mon, Sep 12, 2016 at 05:35:15PM -0400, Leo Famulari wrote:
> > > This patch applies an upstream patch for a regression caused by the fix 
> > > for CVE-2016-0718.
> > > 
> > > Apparently, the bug only manifests when building with -DXML_UNICODE,
> > > which I don't think our package does.
> > 
> > Sebastian Pipping (the Expat maintainer) contacted me to recommend that
> > we apply the patch on the master branch.
> > 
> > He says that the faulty code path can be reached even when XML_UNICODE
> > is not defined. Apparently, building with -DXML_UNICODE merely makes it
> > easier to reach the faulty code.
> > 
> > I think we should take Sebastian's advice. What does everyone think?
> 
> 
> Seems to me that as the Expat maintainer he would know best.

I pushed the commit as b9bc6e842066b066ebdf9eaf75d41753598d75b5
diff mbox

Patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 5714009..6e1c97c 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -501,6 +501,7 @@  dist_patch_DATA =						\
   %D%/packages/patches/expat-CVE-2012-6702-and-CVE-2016-5300.patch	\
   %D%/packages/patches/expat-CVE-2015-1283-refix.patch		\
   %D%/packages/patches/expat-CVE-2016-0718.patch		\
+  %D%/packages/patches/expat-CVE-2016-0718-fix-regression.patch	\
   %D%/packages/patches/fastcap-mulGlobal.patch			\
   %D%/packages/patches/fastcap-mulSetup.patch			\
   %D%/packages/patches/fasthenry-spAllocate.patch		\
diff --git a/gnu/packages/patches/expat-CVE-2016-0718-fix-regression.patch b/gnu/packages/patches/expat-CVE-2016-0718-fix-regression.patch
new file mode 100644
index 0000000..b489401
--- /dev/null
+++ b/gnu/packages/patches/expat-CVE-2016-0718-fix-regression.patch
@@ -0,0 +1,35 @@ 
+Fix regression caused by fix for CVE-2016-0718 when building with -DXML_UNICODE.
+
+Discussion:
+
+https://sourceforge.net/p/expat/bugs/539/
+
+Patch copied from upstream source repository:
+
+https://sourceforge.net/p/expat/code_git/ci/af507cef2c93cb8d40062a0abe43a4f4e9158fb2/
+
+From af507cef2c93cb8d40062a0abe43a4f4e9158fb2 Mon Sep 17 00:00:00 2001
+From: Sebastian Pipping <sebastian@pipping.org>
+Date: Sun, 17 Jul 2016 20:22:29 +0200
+Subject: [PATCH 1/2] Fix regression bug #539 (needs -DXML_UNICODE)
+
+Thanks to Andy Wang and Karl Waclawek!
+---
+ expat/lib/xmlparse.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/expat/lib/xmlparse.c b/expat/lib/xmlparse.c
+index b308e67..0d5dd7b 100644
+--- a/lib/xmlparse.c
++++ b/lib/xmlparse.c
+@@ -2468,7 +2468,7 @@ doContent(XML_Parser parser,
+                        &fromPtr, rawNameEnd,
+                        (ICHAR **)&toPtr, (ICHAR *)tag->bufEnd - 1);
+             convLen = (int)(toPtr - (XML_Char *)tag->buf);
+-            if ((convert_res == XML_CONVERT_COMPLETED) || (convert_res == XML_CONVERT_INPUT_INCOMPLETE)) {
++            if ((fromPtr >= rawNameEnd) || (convert_res == XML_CONVERT_INPUT_INCOMPLETE)) {
+               tag->name.strLen = convLen;
+               break;
+             }
+-- 
+2.10.0
diff --git a/gnu/packages/xml.scm b/gnu/packages/xml.scm
index 8e0fe01..9dcedee 100644
--- a/gnu/packages/xml.scm
+++ b/gnu/packages/xml.scm
@@ -50,6 +50,7 @@ 
 (define-public expat
   (package
     (name "expat")
+    (replacement expat/fixed)
     (version "2.1.1")
     (source (origin
              (method url-fetch)
@@ -70,6 +71,17 @@  stream-oriented parser in which an application registers handlers for
 things the parser might find in the XML document (like start tags).")
     (license license:expat)))
 
+(define expat/fixed
+  (package
+    (inherit expat)
+    (source (origin
+              (inherit (package-source expat))
+              (patches (search-patches
+                         "expat-CVE-2012-6702-and-CVE-2016-5300.patch"
+                         "expat-CVE-2015-1283-refix.patch"
+                         "expat-CVE-2016-0718.patch"
+                         "expat-CVE-2016-0718-fix-regression.patch"))))))
+
 (define-public libxml2
   (package
     (name "libxml2")