allow loading of xml with DTD references, fixes #1185 (#1208)

taseroth · jexp · commit 5cf5f95acecf · 2019-05-29T14:16:35.000+02:00
loading of xml files was previously forbidden (#931). With this change, an dummy entity resolver is registered that does nothing, e.g. does not load external files.
diff --git a/src/main/java/apoc/load/Xml.java b/src/main/java/apoc/load/Xml.java
@@ -1,8 +1,8 @@
 package apoc.load;
 
-import apoc.util.FileUtils;
 import apoc.result.MapResult;
 import apoc.result.NodeResult;
+import apoc.util.FileUtils;
 import apoc.util.Util;
 import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -13,6 +13,7 @@
 import org.neo4j.procedure.*;
 import org.w3c.dom.CharacterData;
 import org.w3c.dom.*;
+import org.xml.sax.InputSource;
 
 import javax.xml.namespace.QName;
 import javax.xml.parsers.DocumentBuilder;
@@ -28,6 +29,7 @@
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.StringReader;
 import java.net.URL;
 import java.net.URLConnection;
 import java.util.*;
@@ -40,7 +42,7 @@
 
 public class Xml {
 
-    public static final XMLInputFactory FACTORY = XMLInputFactory.newFactory();
+    private static final XMLInputFactory FACTORY = XMLInputFactory.newFactory();
 
     @Context
     public GraphDatabaseService db;
@@ -69,8 +71,8 @@ private Stream<MapResult> xmlXpathToMapResult(@Name("url") String url, boolean s
             DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
             documentBuilderFactory.setNamespaceAware(true);
             documentBuilderFactory.setIgnoringElementContentWhitespace(true);
-            documentBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
             DocumentBuilder documentBuilder = documentBuilderFactory.newDocumentBuilder();
+            documentBuilder.setEntityResolver((publicId, systemId) -> new InputSource(new StringReader("")));
 
             FileUtils.checkReadAllowed(url);
             url = FileUtils.changeFileUrlIfImportDirectoryConstrained(url);
diff --git a/src/test/java/apoc/load/XmlTest.java b/src/test/java/apoc/load/XmlTest.java
@@ -1,15 +1,12 @@
 package apoc.load;
 
 import apoc.util.TestUtil;
-import org.apache.commons.lang.exception.ExceptionUtils;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.neo4j.graphdb.GraphDatabaseService;
-import org.neo4j.graphdb.QueryExecutionException;
 import org.neo4j.helpers.collection.Iterators;
 import org.neo4j.test.TestGraphDatabaseFactory;
-import org.xml.sax.SAXParseException;
 
 import java.util.Collections;
 import java.util.List;
@@ -22,29 +19,29 @@
 
 public class XmlTest {
 
-    public static final String XML_AS_NESTED_MAP =
+    private static final String XML_AS_NESTED_MAP =
             "{_type=parent, name=databases, " +
                     "_children=[" +
                     "{_type=child, name=Neo4j, _text=Neo4j is a graph database}, " +
                     "{_type=child, name=relational, _children=[" +
                     "{_type=grandchild, name=MySQL, _text=MySQL is a database & relational}, " +
                     "{_type=grandchild, name=Postgres, _text=Postgres is a relational database}]}]}";
-    public static final String XML_AS_NESTED_SIMPLE_MAP =
+    private static final String XML_AS_NESTED_SIMPLE_MAP =
             "{_type=parent, name=databases, " +
                     "_child=[" +
                     "{_type=child, name=Neo4j, _text=Neo4j is a graph database}, " +
                     "{_type=child, name=relational, _grandchild=[" +
                     "{_type=grandchild, name=MySQL, _text=MySQL is a database & relational}, " +
                     "{_type=grandchild, name=Postgres, _text=Postgres is a relational database}]}]}";
-    public static final String XML_XPATH_AS_NESTED_MAP =
+    static final String XML_XPATH_AS_NESTED_MAP =
             "[{_type=book, id=bk103, _children=[{_type=author, _text=Corets, Eva}, {_type=title, _text=Maeve Ascendant}, {_type=genre, _text=Fantasy}, {_type=price, _text=5.95}, {_type=publish_date, _text=2000-11-17}, {_type=description, _text=After the collapse of a nanotechnology " +
                     "society in England, the young survivors lay the " +
                     "foundation for a new society.}]}]";
 
-    public static final String XML_AS_SINGLE_LINE =
+    private static final String XML_AS_SINGLE_LINE =
             "{_type=table, _children=[{_type=tr, _children=[{_type=td, _children=[{_type=img, src=pix/logo-tl.gif}]}]}]}";
 
-    public static final String XML_AS_SINGLE_LINE_SIMPLE =
+    private static final String XML_AS_SINGLE_LINE_SIMPLE =
             "{_type=table, _table=[{_type=tr, _tr=[{_type=td, _td=[{_type=img, src=pix/logo-tl.gif}]}]}]}";
 
     private GraphDatabaseService db;
@@ -64,7 +61,7 @@ public void tearDown() {
     }
 
     @Test
-    public void testLoadXml() throws Exception {
+    public void testLoadXml() {
         testCall(db, "CALL apoc.load.xml('file:databases.xml')", //  YIELD value RETURN value
                 (row) -> {
                     Object value = row.get("value");
@@ -73,7 +70,7 @@ public void testLoadXml() throws Exception {
     }
 
     @Test
-    public void testLoadXmlSimple() throws Exception {
+    public void testLoadXmlSimple() {
         testCall(db, "CALL apoc.load.xmlSimple('file:databases.xml')", //  YIELD value RETURN value
                 (row) -> {
                     Object value = row.get("value");
@@ -247,10 +244,10 @@ public void testLoadXmlWithNextWordRels() {
                 row -> assertNotNull(row.get("node")));
         testResult(db, "match (n) return labels(n)[0] as label, count(*) as count", result -> {
             final Map<String, Long> resultMap = result.stream().collect(Collectors.toMap(o -> (String)o.get("label"), o -> (Long)o.get("count")));
-            assertEquals(2l, (long)resultMap.get("XmlProcessingInstruction"));
-            assertEquals(1l, (long)resultMap.get("XmlDocument"));
-            assertEquals(3263l, (long)resultMap.get("XmlWord"));
-            assertEquals(454l, (long)resultMap.get("XmlTag"));
+            assertEquals(2L, (long)resultMap.get("XmlProcessingInstruction"));
+            assertEquals(1L, (long)resultMap.get("XmlDocument"));
+            assertEquals(3263L, (long)resultMap.get("XmlWord"));
+            assertEquals(454L, (long)resultMap.get("XmlTag"));
         });
 
         // no node more than one NEXT/NEXT_SIBLING
@@ -265,7 +262,7 @@ public void testLoadXmlWithNextWordRels() {
         testResult(db, "match p=(:XmlDocument)-[:NEXT_WORD*]->(e:XmlWord) where not (e)-[:NEXT_WORD]->() return length(p) as len",
                 result -> {
                     Map<String, Object> r = Iterators.single(result);
-                    assertEquals(3263l, r.get("len"));
+                    assertEquals(3263L, r.get("len"));
                 });
     }
 
@@ -276,10 +273,10 @@ public void testLoadXmlWithNextEntityRels() {
                 row -> assertNotNull(row.get("node")));
         testResult(db, "match (n) return labels(n)[0] as label, count(*) as count", result -> {
             final Map<String, Long> resultMap = result.stream().collect(Collectors.toMap(o -> (String)o.get("label"), o -> (Long)o.get("count")));
-            assertEquals(2l, (long)resultMap.get("XmlProcessingInstruction"));
-            assertEquals(1l, (long)resultMap.get("XmlDocument"));
-            assertEquals(3263l, (long)resultMap.get("XmlCharacters"));
-            assertEquals(454l, (long)resultMap.get("XmlTag"));
+            assertEquals(2L, (long)resultMap.get("XmlProcessingInstruction"));
+            assertEquals(1L, (long)resultMap.get("XmlDocument"));
+            assertEquals(3263L, (long)resultMap.get("XmlCharacters"));
+            assertEquals(454L, (long)resultMap.get("XmlTag"));
         });
 
         // no node more than one NEXT/NEXT_SIBLING
@@ -294,7 +291,7 @@ public void testLoadXmlWithNextEntityRels() {
         testResult(db, "match p=(:XmlDocument)-[:NE*]->(e:XmlCharacters) where not (e)-[:NE]->() return length(p) as len",
                 result -> {
                     Map<String, Object> r = Iterators.single(result);
-                    assertEquals(3263l, r.get("len"));
+                    assertEquals(3263L, r.get("len"));
                 });
     }
 
@@ -378,21 +375,17 @@ public void testLoadXmlFromTgzByUrl() {
         });
     }
 
-    @Test(expected = QueryExecutionException.class)
-    public void testLoadXmlPreventXXEVulnerabilityThrowsQueryExecutionException() {
-        try {
-            testResult(db, "CALL apoc.load.xml('file:src/test/resources/xml/xxe.xml', '/catalog/book[genre=\"Computer\"]') yield value as result", (r) -> {});
-        } catch (QueryExecutionException e) {
-            // We want test that the cause of the exception is SAXParseException with the correct cause message
-            Throwable except = ExceptionUtils.getRootCause(e);
-            assertTrue(except instanceof SAXParseException);
-            assertEquals("DOCTYPE is disallowed when the feature \"http://apache.org/xml/features/disallow-doctype-decl\" set to true.", except.getMessage());
-            throw e;
-        }
+    @Test
+    public void testExternalDTDschouldNotBeLoaded() {
+        testCall(db, "CALL apoc.load.xml('file:src/test/resources/xml/missingExternalDTD.xml', '/', null, true)",
+                (row) -> {
+                    Object value = row.get("value");
+                    assertEquals("{_type=document, _document=[null, {_type=title, _text=dtd 404}]}", value.toString());
+                });
     }
 
     @Test
-    public void testLoadXmlSingleLineSimple() throws Exception {
+    public void testLoadXmlSingleLineSimple() {
         testCall(db, "CALL apoc.load.xml('file:src/test/resources/xml/singleLine.xml', '/', null, true)", //  YIELD value RETURN value
                 (row) -> {
                     Object value = row.get("value");
@@ -401,7 +394,7 @@ public void testLoadXmlSingleLineSimple() throws Exception {
     }
 
     @Test
-    public void testLoadXmlSingleLine() throws Exception {
+    public void testLoadXmlSingleLine() {
         testCall(db, "CALL apoc.load.xml('file:src/test/resources/xml/singleLine.xml')", //  YIELD value RETURN value
                 (row) -> {
                     Object value = row.get("value");
diff --git a/src/test/resources/xml/missingExternalDTD.xml b/src/test/resources/xml/missingExternalDTD.xml
@@ -0,0 +1,10 @@
+<?xml version="1.0" ?>
+
+<!DOCTYPE document SYSTEM "file://should-not-exist.dtd">
+<document>
+    <!-- The refenced external DTD should point to a non-exiting file.
+        Since we do not want to load external DTDs (XXE attack), the parser
+        should be configured in a way not to load external DTD and hence not fail.
+    -->
+    <title>dtd 404</title>
+</document>
diff --git a/src/test/resources/xml/xxe.xml b/src/test/resources/xml/xxe.xml
