diff mbox

[0/3] Expat and libxslt changes for core-updates

Message ID 20160609231935.GA14894@jasmine
State New
Headers show

Commit Message

Leo Famulari June 9, 2016, 11:19 p.m. UTC
On Thu, Jun 09, 2016 at 12:43:17PM -0400, Leo Famulari wrote:
> On Wed, Jun 08, 2016 at 01:10:16PM +0300, Efraim Flashner wrote:
> > FWIW debian's expat-2.1.1(-3) still has the cve-2015-1283 applied.
> 
> I looked at the expat Git repo and the original fix for CVE-2015-1283
> was part of 2.1.1. The improvement to the fix must be backported. I will
> take the upstream commit that applies the improvement.

We adopt Debian's patch for CVE-2012-6702 and CVE-2016-5300 (already
sent for review for the master branch).

We also adapt the CVE-2015-1283 "re-fix" patch to apply to upstream's
fix for CVE-2015-1283. The Debian "re-fix" patch had some context
(comments) that did not exist in the upstream 2.1.1 release.

And as before, we patch for CVE-2016-0718.

It's not possible for me test this on core-updates (too much to build).
On master, I made a new expat-2.1.1 package that inherited from expat
and built that with the patches.

The merge will probably be messy...

Off-topic: A regular package and a grafted package on master, and an
updated version of the package on core-updates... this is getting very
complicated and we should try our best to avoid such tangled situations
in the future.

Comments

Ludovic Courtès June 10, 2016, 12:59 p.m. UTC | #1
Leo Famulari <leo@famulari.name> skribis:

> On Thu, Jun 09, 2016 at 12:43:17PM -0400, Leo Famulari wrote:
>> On Wed, Jun 08, 2016 at 01:10:16PM +0300, Efraim Flashner wrote:
>> > FWIW debian's expat-2.1.1(-3) still has the cve-2015-1283 applied.
>> 
>> I looked at the expat Git repo and the original fix for CVE-2015-1283
>> was part of 2.1.1. The improvement to the fix must be backported. I will
>> take the upstream commit that applies the improvement.
>
> We adopt Debian's patch for CVE-2012-6702 and CVE-2016-5300 (already
> sent for review for the master branch).
>
> We also adapt the CVE-2015-1283 "re-fix" patch to apply to upstream's
> fix for CVE-2015-1283. The Debian "re-fix" patch had some context
> (comments) that did not exist in the upstream 2.1.1 release.
>
> And as before, we patch for CVE-2016-0718.
>
> It's not possible for me test this on core-updates (too much to build).
> On master, I made a new expat-2.1.1 package that inherited from expat
> and built that with the patches.
>
> The merge will probably be messy...

We should leave it to you, to minimize breakage.

> Off-topic: A regular package and a grafted package on master, and an
> updated version of the package on core-updates... this is getting very
> complicated and we should try our best to avoid such tangled situations
> in the future.

Do you think it would help to delay such upgrades in ‘core-updates’
until the time where ‘core-updates’ is getting ready for merge?

> From a4a3a09b40c5f98b2c2a3d15458ab086ce867c3d Mon Sep 17 00:00:00 2001
> From: Leo Famulari <leo@famulari.name>
> Date: Tue, 7 Jun 2016 20:26:41 -0400
> Subject: [v2 1/2] gnu: expat: Fix CVE-2012-6702, CVE-2016-0718, and
>  CVE-2016-5300.
>
> * gnu/packages/patches/expat-CVE-2012-6702-and-CVE-2016-5300.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.
> * gnu/packages/patches/expat-CVE-2015-1283-refix.patch: Adapt to upstream
> changes.
> * gnu/packages/xml.scm (expat)[source]: Use patches.

LGTM, thank you!

Ludo’.
Leo Famulari June 11, 2016, 12:49 a.m. UTC | #2
On Fri, Jun 10, 2016 at 02:59:49PM +0200, Ludovic Courtès wrote:
> Leo Famulari <leo@famulari.name> skribis:
> > The merge will probably be messy...
> 
> We should leave it to you, to minimize breakage.

Okay, should I do it today or is core-updates frozen?

> > Off-topic: A regular package and a grafted package on master, and an
> > updated version of the package on core-updates... this is getting very
> > complicated and we should try our best to avoid such tangled situations
> > in the future.
> 
> Do you think it would help to delay such upgrades in ‘core-updates’
> until the time where ‘core-updates’ is getting ready for merge?

I don't know if there is a great solution; I think this is a really
perverse case.

I think of core-updates as the place to put these sorts of changes. If
we were to decide to delay the changes, we'd all have our own private
core-updates forks, and then we'd probably duplicate work and forget
about things. What do you think?

If anybody else feels burdened by this as I do, your ideas are very
welcome :)
Ludovic Courtès June 12, 2016, 8:26 p.m. UTC | #3
Leo Famulari <leo@famulari.name> skribis:

> On Fri, Jun 10, 2016 at 02:59:49PM +0200, Ludovic Courtès wrote:
>> Leo Famulari <leo@famulari.name> skribis:
>> > The merge will probably be messy...
>> 
>> We should leave it to you, to minimize breakage.
>
> Okay, should I do it today or is core-updates frozen?

It’s OK if you do it today.  :-)

The branch is being built but it’s just the beginning.

>> > Off-topic: A regular package and a grafted package on master, and an
>> > updated version of the package on core-updates... this is getting very
>> > complicated and we should try our best to avoid such tangled situations
>> > in the future.
>> 
>> Do you think it would help to delay such upgrades in ‘core-updates’
>> until the time where ‘core-updates’ is getting ready for merge?
>
> I don't know if there is a great solution; I think this is a really
> perverse case.
>
> I think of core-updates as the place to put these sorts of changes. If
> we were to decide to delay the changes, we'd all have our own private
> core-updates forks, and then we'd probably duplicate work and forget
> about things. What do you think?

Sorry, I explained myself poorly.  Here, we (1) grafted Expat in master,
(2) upgraded Expat in core-updates, and (3) only after that did we merge
master in core-updates, making the merge more troublesome, right?

My question was whether we should swap (2) and (3).

Thoughts?

Ludo’.
Leo Famulari June 13, 2016, 2:20 a.m. UTC | #4
On Sun, Jun 12, 2016 at 10:26:53PM +0200, Ludovic Courtès wrote:
> Leo Famulari <leo@famulari.name> skribis:
> 
> > On Fri, Jun 10, 2016 at 02:59:49PM +0200, Ludovic Courtès wrote:
> >> Leo Famulari <leo@famulari.name> skribis:
> >> > The merge will probably be messy...
> >> 
> >> We should leave it to you, to minimize breakage.
> >
> > Okay, should I do it today or is core-updates frozen?
> 
> It’s OK if you do it today.  :-)
> 
> The branch is being built but it’s just the beginning.

Okay, I've done it. I spent so much time thinking about this merge that
I am confident it's correct, but I'm nervous because I can't feasibly
build it as a test.

`make` succeeded.

> Sorry, I explained myself poorly.  Here, we (1) grafted Expat in master,
> (2) upgraded Expat in core-updates, and (3) only after that did we merge
> master in core-updates, making the merge more troublesome, right?
> 
> My question was whether we should swap (2) and (3).

Yes, I think we should do (3) before (2), if we notice (1).

Although (2) preceded (1) chronologically, in this case.

(2) happened circa Mar 25 and (1) happened circa May 18, according to
the dates on the commits.

So, swapping (2) and (3) wouldn't have helped this case.
Ludovic Courtès June 13, 2016, 3:05 p.m. UTC | #5
Leo Famulari <leo@famulari.name> skribis:

> On Sun, Jun 12, 2016 at 10:26:53PM +0200, Ludovic Courtès wrote:

[...]

>> Sorry, I explained myself poorly.  Here, we (1) grafted Expat in master,
>> (2) upgraded Expat in core-updates, and (3) only after that did we merge
>> master in core-updates, making the merge more troublesome, right?
>> 
>> My question was whether we should swap (2) and (3).
>
> Yes, I think we should do (3) before (2), if we notice (1).
>
> Although (2) preceded (1) chronologically, in this case.
>
> (2) happened circa Mar 25 and (1) happened circa May 18, according to
> the dates on the commits.
>
> So, swapping (2) and (3) wouldn't have helped this case.

OK.  In an ideal world, ‘core-updates’ cycles would be shorter, so we
wouldn’t have had to worry about (3) in the first place.

Ludo’.
diff mbox

Patch

From a4a3a09b40c5f98b2c2a3d15458ab086ce867c3d Mon Sep 17 00:00:00 2001
From: Leo Famulari <leo@famulari.name>
Date: Tue, 7 Jun 2016 20:26:41 -0400
Subject: [v2 1/2] gnu: expat: Fix CVE-2012-6702, CVE-2016-0718, and
 CVE-2016-5300.

* gnu/packages/patches/expat-CVE-2012-6702-and-CVE-2016-5300.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/patches/expat-CVE-2015-1283-refix.patch: Adapt to upstream
changes.
* gnu/packages/xml.scm (expat)[source]: Use patches.
---
 gnu/local.mk                                       |   1 +
 .../expat-CVE-2012-6702-and-CVE-2016-5300.patch    | 142 +++++++++++++++++++++
 .../patches/expat-CVE-2015-1283-refix.patch        |  27 ++--
 gnu/packages/xml.scm                               |   4 +
 4 files changed, 159 insertions(+), 15 deletions(-)
 create mode 100644 gnu/packages/patches/expat-CVE-2012-6702-and-CVE-2016-5300.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index ef7b4df..4ace667 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -480,6 +480,7 @@  dist_patch_DATA =						\
   %D%/packages/patches/emacs-source-date-epoch.patch		\
   %D%/packages/patches/eudev-rules-directory.patch		\
   %D%/packages/patches/evilwm-lost-focus-bug.patch		\
+  %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/fastcap-mulGlobal.patch			\
diff --git a/gnu/packages/patches/expat-CVE-2012-6702-and-CVE-2016-5300.patch b/gnu/packages/patches/expat-CVE-2012-6702-and-CVE-2016-5300.patch
new file mode 100644
index 0000000..edc43f8
--- /dev/null
+++ b/gnu/packages/patches/expat-CVE-2012-6702-and-CVE-2016-5300.patch
@@ -0,0 +1,142 @@ 
+Fix CVE-2012-6702 and CVE-2016-5300.
+
+https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2012-6702
+https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-5300
+
+Patch copied from:
+https://sources.debian.net/src/expat/2.1.0-6%2Bdeb8u3/debian/patches/cve-2012-6702-plus-cve-2016-5300-v1.patch/
+
+From cb31522769d11a375078a073cba94e7176cb48a4 Mon Sep 17 00:00:00 2001
+From: Sebastian Pipping <sebastian@pipping.org>
+Date: Wed, 16 Mar 2016 15:30:12 +0100
+Subject: [PATCH] Resolve call to srand, use more entropy (patch version 1.0)
+
+Squashed backport against vanilla Expat 2.1.1, addressing:
+* CVE-2012-6702 -- unanticipated internal calls to srand
+* CVE-2016-5300 -- use of too little entropy
+
+Since commit e3e81a6d9f0885ea02d3979151c358f314bf3d6d
+(released with Expat 2.1.0) Expat called srand by itself
+from inside generate_hash_secret_salt for an instance
+of XML_Parser if XML_SetHashSalt was either (a) not called
+for that instance or if (b) salt 0 was passed to XML_SetHashSalt
+prior to parsing.  That call to srand passed (rather litle)
+entropy extracted from the current time as a seed for srand.
+
+That call to srand (1) broke repeatability for code calling
+srand with a non-random seed prior to parsing with Expat,
+and (2) resulted in a rather small set of hashing salts in
+Expat in total.
+
+For a short- to mid-term fix, the new approach avoids calling
+srand altogether, extracts more entropy out of the clock and
+other sources, too.
+
+For a long term fix, we may want to read sizeof(long) bytes
+from a source like getrandom(..) on Linux, and from similar
+sources on other supported architectures.
+
+https://bugzilla.redhat.com/show_bug.cgi?id=1197087
+---
+ CMakeLists.txt |  3 +++
+ lib/xmlparse.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
+ 2 files changed, 44 insertions(+), 7 deletions(-)
+
+diff --git a/CMakeLists.txt b/CMakeLists.txt
+index 353627e..524d514 100755
+--- a/CMakeLists.txt
++++ b/CMakeLists.txt
+@@ -41,6 +41,9 @@ include_directories(${CMAKE_BINARY_DIR} ${CMAKE_SOURCE_DIR}/lib)
+ if(MSVC)
+     add_definitions(-D_CRT_SECURE_NO_WARNINGS -wd4996)
+ endif(MSVC)
++if(WIN32)
++    add_definitions(-DCOMPILED_FROM_DSP)
++endif(WIN32)
+ 
+ set(expat_SRCS
+     lib/xmlparse.c
+diff --git a/lib/xmlparse.c b/lib/xmlparse.c
+index e308c79..c5f942f 100644
+--- a/lib/xmlparse.c
++++ b/lib/xmlparse.c
+@@ -6,7 +6,14 @@
+ #include <string.h>                     /* memset(), memcpy() */
+ #include <assert.h>
+ #include <limits.h>                     /* UINT_MAX */
+-#include <time.h>                       /* time() */
++
++#ifdef COMPILED_FROM_DSP
++#define getpid GetCurrentProcessId
++#else
++#include <sys/time.h>                   /* gettimeofday() */
++#include <sys/types.h>                  /* getpid() */
++#include <unistd.h>                     /* getpid() */
++#endif
+ 
+ #define XML_BUILDING_EXPAT 1
+ 
+@@ -432,7 +439,7 @@ static ELEMENT_TYPE *
+ getElementType(XML_Parser parser, const ENCODING *enc,
+                const char *ptr, const char *end);
+ 
+-static unsigned long generate_hash_secret_salt(void);
++static unsigned long generate_hash_secret_salt(XML_Parser parser);
+ static XML_Bool startParsing(XML_Parser parser);
+ 
+ static XML_Parser
+@@ -691,11 +698,38 @@ static const XML_Char implicitContext[] = {
+ };
+ 
+ static unsigned long
+-generate_hash_secret_salt(void)
++gather_time_entropy(void)
+ {
+-  unsigned int seed = time(NULL) % UINT_MAX;
+-  srand(seed);
+-  return rand();
++#ifdef COMPILED_FROM_DSP
++  FILETIME ft;
++  GetSystemTimeAsFileTime(&ft); /* never fails */
++  return ft.dwHighDateTime ^ ft.dwLowDateTime;
++#else
++  struct timeval tv;
++  int gettimeofday_res;
++
++  gettimeofday_res = gettimeofday(&tv, NULL);
++  assert (gettimeofday_res == 0);
++
++  /* Microseconds time is <20 bits entropy */
++  return tv.tv_usec;
++#endif
++}
++
++static unsigned long
++generate_hash_secret_salt(XML_Parser parser)
++{
++  /* Process ID is 0 bits entropy if attacker has local access
++   * XML_Parser address is few bits of entropy if attacker has local access */
++  const unsigned long entropy =
++      gather_time_entropy() ^ getpid() ^ (unsigned long)parser;
++
++  /* Factors are 2^31-1 and 2^61-1 (Mersenne primes M31 and M61) */
++  if (sizeof(unsigned long) == 4) {
++    return entropy * 2147483647;
++  } else {
++    return entropy * 2305843009213693951;
++  }
+ }
+ 
+ static XML_Bool  /* only valid for root parser */
+@@ -703,7 +737,7 @@ startParsing(XML_Parser parser)
+ {
+     /* hash functions must be initialized before setContext() is called */
+     if (hash_secret_salt == 0)
+-      hash_secret_salt = generate_hash_secret_salt();
++      hash_secret_salt = generate_hash_secret_salt(parser);
+     if (ns) {
+       /* implicit context only set for root parser, since child
+          parsers (i.e. external entity parsers) will inherit it
+-- 
+2.8.2
+
diff --git a/gnu/packages/patches/expat-CVE-2015-1283-refix.patch b/gnu/packages/patches/expat-CVE-2015-1283-refix.patch
index af5e3bc..fc8d629 100644
--- a/gnu/packages/patches/expat-CVE-2015-1283-refix.patch
+++ b/gnu/packages/patches/expat-CVE-2015-1283-refix.patch
@@ -1,42 +1,39 @@ 
-Update previous fix for CVE-2015-1283 to not rely on undefined behavior.
+Follow-up upstream fix for CVE-2015-1283 to not rely on undefined
+behavior.
 
-Copied from Debian, as found in Debian package version 2.1.0-6+deb8u2.
+Adapted from a patch from Debian (found in Debian package version
+2.1.0-6+deb8u2) to apply to upstream code:
 
 https://sources.debian.net/src/expat/2.1.0-6%2Bdeb8u2/debian/patches/CVE-2015-1283-refix.patch/
 
-From 29a11774d8ebbafe8418b4a5ffb4cc1160b194a1 Mon Sep 17 00:00:00 2001
-From: Pascal Cuoq <cuoq@trust-in-soft.com>
-Date: Sun, 15 May 2016 09:05:46 +0200
-Subject: [PATCH] Avoid relying on undefined behavior in CVE-2015-1283 fix.
-
 ---
- expat/lib/xmlparse.c | 6 ++++--
+ lib/xmlparse.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)
 
 diff --git a/lib/xmlparse.c b/lib/xmlparse.c
-index 13e080d..cdb12ef 100644
+index 0f6f4cd..5c70c17 100644
 --- a/lib/xmlparse.c
 +++ b/lib/xmlparse.c
-@@ -1695,7 +1695,8 @@ XML_GetBuffer(XML_Parser parser, int len
+@@ -1727,7 +1727,8 @@ XML_GetBuffer(XML_Parser parser, int len)
    }
  
    if (len > bufferLim - bufferEnd) {
 -    int neededSize = len + (int)(bufferEnd - bufferPtr);
 +    /* Do not invoke signed arithmetic overflow: */
 +    int neededSize = (int) ((unsigned)len + (unsigned)(bufferEnd - bufferPtr));
- /* BEGIN MOZILLA CHANGE (sanity check neededSize) */
      if (neededSize < 0) {
        errorCode = XML_ERROR_NO_MEMORY;
-@@ -1729,7 +1730,8 @@ XML_GetBuffer(XML_Parser parser, int len
+       return NULL;
+@@ -1759,7 +1760,8 @@ XML_GetBuffer(XML_Parser parser, int len)
        if (bufferSize == 0)
          bufferSize = INIT_BUFFER_SIZE;
        do {
 -        bufferSize *= 2;
 +        /* Do not invoke signed arithmetic overflow: */
 +        bufferSize = (int) (2U * (unsigned) bufferSize);
- /* BEGIN MOZILLA CHANGE (prevent infinite loop on overflow) */
        } while (bufferSize < neededSize && bufferSize > 0);
- /* END MOZILLA CHANGE */
+       if (bufferSize <= 0) {
+         errorCode = XML_ERROR_NO_MEMORY;
 -- 
-2.8.2
+2.8.3
 
diff --git a/gnu/packages/xml.scm b/gnu/packages/xml.scm
index a860f98..f3bb7d8 100644
--- a/gnu/packages/xml.scm
+++ b/gnu/packages/xml.scm
@@ -51,6 +51,10 @@ 
              (method url-fetch)
              (uri (string-append "mirror://sourceforge/expat/expat/"
                                  version "/expat-" version ".tar.bz2"))
+             (patches (search-patches
+                        "expat-CVE-2012-6702-and-CVE-2016-5300.patch"
+                        "expat-CVE-2015-1283-refix.patch"
+                        "expat-CVE-2016-0718.patch"))
              (sha256
               (base32
                "0ryyjgvy7jq0qb7a9mhc1giy3bzn56aiwrs8dpydqngplbjq9xdg"))))
-- 
2.8.3