Merge lp:~doctormo/inkscape/fix166371 into lp:~inkscape.dev/inkscape/trunk

[Martin Owens]

This is a fix for our critical blocker regarding Adobe vs Hackers in bug 166371. I should explain some things:

The security vuln occurs when SYSTEM calls load data from the hard drive, using replacements with entities causes it to open a huge security hole where private data can be stolen. This can be closed by stopping the parsing of ENTITIES. libxml2's response to the vulnerability was very poor IMO, simply killing an entire feature of their library without sufficient protection when it's switched back on.

But Adobe files REQUIRE ENTITY replacements for their namespace attributes.

What this fix does, is firstly attempt a normal open, so most files will not be slowed down by the filtering. If it fails in the specific way Adobe files fail... which is to cause a valid document with no namespace i.e. ns:svg instead of svg:svg then it attempts to reload the document, this time it switches on NOENT /and/ the same switch strips SYSTEM out of the read buffer before it gets to the parser. Thus even an adobe file can't be used to steal data and there is no vuln.

The alternative fix, is to switch NOENT back off and manually replace each of the Adobe namespace entities, but this is longer winded and needs updating in any ways files might differ. Please advise as the code is similar and doesn't require much change between them.

[lp:~doctormo/inkscape/fix166371]

12507. By Martin Owens on 2013-09-12

Improve code with Kosiński's regex replacement

12508. By Martin Owens on 2013-09-12

Fix regex so it effects the buffer and ban PUBLIC entities too.

=== modified file 'src/xml/repr-io.cpp'
--- src/xml/repr-io.cpp	2013-08-05 21:07:35 +0000
+++ src/xml/repr-io.cpp	2013-09-12 21:07:18 +0000
@@ -100,6 +100,9 @@
 
     int setFile( char const * filename );
 
+    xmlDocPtr readXml();
+    bool SystemCheck; // Checks for SYSTEM Entities
+
     static int readCb( void * context, char * buffer, int len );
     static int closeCb( void * context );
 
@@ -121,6 +124,7 @@
 {
     int retVal = -1;
 
+    this->SystemCheck = false;
     this->filename = filename;
 
     fp = Inkscape::IO::fopen_utf8name(filename, "r");
@@ -178,13 +182,53 @@
     return retVal;
 }
 
+xmlDocPtr XmlSource::readXml()
+{
+    int parse_options = XML_PARSE_HUGE | XML_PARSE_RECOVER;
+
+    Inkscape::Preferences *prefs = Inkscape::Preferences::get();
+    bool allowNetAccess = prefs->getBool("/options/externalresources/xml/allow_net_access", false);
+    if (!allowNetAccess) parse_options |= XML_PARSE_NONET;
+
+    // Allow NOENT only if we're filtering out SYSTEM and PUBLIC entities
+    if (SystemCheck)     parse_options |= XML_PARSE_NOENT;
+
+    return xmlReadIO( readCb, closeCb, this,
+                      filename, getEncoding(), parse_options);
+}
 
 int XmlSource::readCb( void * context, char * buffer, int len )
 {
     int retVal = -1;
+
     if ( context ) {
         XmlSource* self = static_cast<XmlSource*>(context);
         retVal = self->read( buffer, len );
+
+        if(self->SystemCheck) {
+            GMatchInfo *info;
+            gint start, end;
+
+            GRegex *regex = g_regex_new(
+                "<!ENTITY\\s+[^>\\s]+\\s+(SYSTEM|PUBLIC\\s+\"[^>\"]+\")\\s+\"[^>\"]+\"\\s*>",
+                G_REGEX_CASELESS, G_REGEX_MATCH_NEWLINE_ANY, NULL);
+
+            // Check for SYSTEM or PUBLIC entities and kill them with spaces
+            // Note: g_regex_replace does not modify buffer in place, this
+            // logic is used instead because we can just blank out the offending
+            // charicters in the right place without hurting the length.
+            g_regex_match (regex, buffer, G_REGEX_MATCH_NEWLINE_ANY, &info);
+
+            while (g_match_info_matches (info)) {
+                if (g_match_info_fetch_pos (info, 1, &start, &end)) {
+                    for (int x=start; x<end; x++)
+                        buffer[x] = 0x20;
+                }
+                g_match_info_next (info, NULL);
+            }
+            g_match_info_unref(info);
+            g_regex_unref(regex);
+        }
     }
     return retVal;
 }
@@ -299,22 +343,21 @@
         XmlSource src;
 
         if ( (src.setFile(filename) == 0) ) {
-            int parse_options = XML_PARSE_HUGE; // do not use XML_PARSE_NOENT ! see bug lp:1025185
-            Inkscape::Preferences *prefs = Inkscape::Preferences::get();
-            bool allowNetAccess = prefs->getBool("/options/externalresources/xml/allow_net_access", false);
-            if (!allowNetAccess) {
-                parse_options |= XML_PARSE_NONET;
+            doc = src.readXml();
+            rdoc = sp_repr_do_read( doc, default_ns );
+            // For some reason, failed ns loading results in this
+            // We try a system check version of load with NOENT for adobe
+            if(rdoc && strcmp(rdoc->root()->name(), "ns:svg") == 0) {
+                xmlFreeDoc( doc );
+                src.setFile(filename);
+                src.SystemCheck = true;
+                doc = src.readXml();
+                rdoc = sp_repr_do_read( doc, default_ns );
             }
-            doc = xmlReadIO( XmlSource::readCb,
-                             XmlSource::closeCb,
-                             &src,
-                             localFilename,
-                             src.getEncoding(),
-                             parse_options);
         }
     }
 
-    rdoc = sp_repr_do_read( doc, default_ns );
+
     if ( doc ) {
         xmlFreeDoc( doc );
     }
@@ -947,15 +990,15 @@
     // THIS DOESN'T APPEAR TO DO ANYTHING. Can it be commented out or deleted?
     {
         GQuark const href_key = g_quark_from_static_string("xlink:href");
-        GQuark const absref_key = g_quark_from_static_string("sodipodi:absref");
+        //GQuark const absref_key = g_quark_from_static_string("sodipodi:absref");
 
         gchar const *xxHref = 0;
-        gchar const *xxAbsref = 0;
+        //gchar const *xxAbsref = 0;
         for ( List<AttributeRecord const> ai(attributes); ai; ++ai ) {
             if ( ai->key == href_key ) {
                 xxHref = ai->value;
-            } else if ( ai->key == absref_key ) {
-                xxAbsref = ai->value;
+            //} else if ( ai->key == absref_key ) {
+                //xxAbsref = ai->value;
             }
         }