Inkscape: A Vector Drawing Tool

Merge lp:~doctormo/inkscape/fix166371 into lp:inkscape

Proposed by Martin Owens on 2013-09-11
Status: Merged
Merged at revision: 12510
Proposed branch: lp:~doctormo/inkscape/fix166371
Merge into: lp:inkscape
Diff against target: 128 lines (+59/-16) 1 file modified
To merge this branch: bzr merge lp:~doctormo/inkscape/fix166371
Reviewer Review Type Date Requested Status
Inkscape Developers 2013-09-11 Pending
Review via email: mp+185064@code.launchpad.net

Description of the Change

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.

To post a comment you must log in.
lp:~doctormo/inkscape/fix166371 updated on 2013-09-12
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.

Preview Diff

1=== modified file 'src/xml/repr-io.cpp'
2--- src/xml/repr-io.cpp 2013-08-05 21:07:35 +0000
3+++ src/xml/repr-io.cpp 2013-09-12 21:07:18 +0000
4@@ -100,6 +100,9 @@
5
6 int setFile( char const * filename );
7
8+ xmlDocPtr readXml();
9+ bool SystemCheck; // Checks for SYSTEM Entities
10+
11 static int readCb( void * context, char * buffer, int len );
12 static int closeCb( void * context );
13
14@@ -121,6 +124,7 @@
15 {
16 int retVal = -1;
17
18+ this->SystemCheck = false;
19 this->filename = filename;
20
21 fp = Inkscape::IO::fopen_utf8name(filename, "r");
22@@ -178,13 +182,53 @@
23 return retVal;
24 }
25
26+xmlDocPtr XmlSource::readXml()
27+{
28+ int parse_options = XML_PARSE_HUGE | XML_PARSE_RECOVER;
29+
30+ Inkscape::Preferences *prefs = Inkscape::Preferences::get();
31+ bool allowNetAccess = prefs->getBool("/options/externalresources/xml/allow_net_access", false);
32+ if (!allowNetAccess) parse_options |= XML_PARSE_NONET;
33+
34+ // Allow NOENT only if we're filtering out SYSTEM and PUBLIC entities
35+ if (SystemCheck) parse_options |= XML_PARSE_NOENT;
36+
37+ return xmlReadIO( readCb, closeCb, this,
38+ filename, getEncoding(), parse_options);
39+}
40
41 int XmlSource::readCb( void * context, char * buffer, int len )
42 {
43 int retVal = -1;
44+
45 if ( context ) {
46 XmlSource* self = static_cast<XmlSource*>(context);
47 retVal = self->read( buffer, len );
48+
49+ if(self->SystemCheck) {
50+ GMatchInfo *info;
51+ gint start, end;
52+
53+ GRegex *regex = g_regex_new(
54+ "<!ENTITY\\s+[^>\\s]+\\s+(SYSTEM|PUBLIC\\s+\"[^>\"]+\")\\s+\"[^>\"]+\"\\s*>",
55+ G_REGEX_CASELESS, G_REGEX_MATCH_NEWLINE_ANY, NULL);
56+
57+ // Check for SYSTEM or PUBLIC entities and kill them with spaces
58+ // Note: g_regex_replace does not modify buffer in place, this
59+ // logic is used instead because we can just blank out the offending
60+ // charicters in the right place without hurting the length.
61+ g_regex_match (regex, buffer, G_REGEX_MATCH_NEWLINE_ANY, &info);
62+
63+ while (g_match_info_matches (info)) {
64+ if (g_match_info_fetch_pos (info, 1, &start, &end)) {
65+ for (int x=start; x<end; x++)
66+ buffer[x] = 0x20;
67+ }
68+ g_match_info_next (info, NULL);
69+ }
70+ g_match_info_unref(info);
71+ g_regex_unref(regex);
72+ }
73 }
74 return retVal;
75 }
76@@ -299,22 +343,21 @@
77 XmlSource src;
78
79 if ( (src.setFile(filename) == 0) ) {
80- int parse_options = XML_PARSE_HUGE; // do not use XML_PARSE_NOENT ! see bug lp:1025185
81- Inkscape::Preferences *prefs = Inkscape::Preferences::get();
82- bool allowNetAccess = prefs->getBool("/options/externalresources/xml/allow_net_access", false);
83- if (!allowNetAccess) {
84- parse_options |= XML_PARSE_NONET;
85+ doc = src.readXml();
86+ rdoc = sp_repr_do_read( doc, default_ns );
87+ // For some reason, failed ns loading results in this
88+ // We try a system check version of load with NOENT for adobe
89+ if(rdoc && strcmp(rdoc->root()->name(), "ns:svg") == 0) {
90+ xmlFreeDoc( doc );
91+ src.setFile(filename);
92+ src.SystemCheck = true;
93+ doc = src.readXml();
94+ rdoc = sp_repr_do_read( doc, default_ns );
95 }
96- doc = xmlReadIO( XmlSource::readCb,
97- XmlSource::closeCb,
98- &src,
99- localFilename,
100- src.getEncoding(),
101- parse_options);
102 }
103 }
104
105- rdoc = sp_repr_do_read( doc, default_ns );
106+
107 if ( doc ) {
108 xmlFreeDoc( doc );
109 }
110@@ -947,15 +990,15 @@
111 // THIS DOESN'T APPEAR TO DO ANYTHING. Can it be commented out or deleted?
112 {
113 GQuark const href_key = g_quark_from_static_string("xlink:href");
114- GQuark const absref_key = g_quark_from_static_string("sodipodi:absref");
115+ //GQuark const absref_key = g_quark_from_static_string("sodipodi:absref");
116
117 gchar const *xxHref = 0;
118- gchar const *xxAbsref = 0;
119+ //gchar const *xxAbsref = 0;
120 for ( List<AttributeRecord const> ai(attributes); ai; ++ai ) {
121 if ( ai->key == href_key ) {
122 xxHref = ai->value;
123- } else if ( ai->key == absref_key ) {
124- xxAbsref = ai->value;
125+ //} else if ( ai->key == absref_key ) {
126+ //xxAbsref = ai->value;
127 }
128 }
129