[AJmycukR] vuln-fix: Securing XML parser against XXE (CVE-2023-23926)

Fixing a XML External Entity (XXE) vulnerability, that was impacting the apoc.import.graphml procedure.
neo4j-contrib · Feb 13, 2023 · c3e2a29 · c3e2a29
1 parent 1b60237
commit c3e2a29
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 0 deletions.
diff --git a/core/src/main/java/apoc/export/graphml/XmlGraphMLReader.java b/core/src/main/java/apoc/export/graphml/XmlGraphMLReader.java
@@ -206,6 +206,8 @@ public long parseXML(Reader input) throws XMLStreamException {
         XMLInputFactory inputFactory = XMLInputFactory.newInstance();
         inputFactory.setProperty("javax.xml.stream.isCoalescing", true);
         inputFactory.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, true);
+        inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
+        inputFactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
         XMLEventReader reader = inputFactory.createXMLEventReader(input);
         Entity last = null;
         Map<String, Key> nodeKeys = new HashMap<>();
@@ -218,10 +220,17 @@ public long parseXML(Reader input) throws XMLStreamException {
                 XMLEvent event;
                 try {
                     event = (XMLEvent) reader.next();
+                    if ( event.getEventType() == XMLStreamConstants.DTD)
+                    {
+                        generateXmlDoctypeException();
+                    }
                 } catch (Exception e) {
                     // in case of unicode invalid chars we skip the event, or we exit in case of EOF
                     if (e.getMessage().contains("Unexpected EOF")) {
                         break;
+                    } else if (e.getMessage().contains("DOCTYPE"))
+                    {
+                        throw e;
                     }
                     continue;
                 }
@@ -416,4 +425,8 @@ private String getAttribute(StartElement element, QName qname) {
         Attribute attribute = element.getAttributeByName(qname);
         return attribute != null ? attribute.getValue() : null;
     }
+
+    private RuntimeException generateXmlDoctypeException() {
+        throw new RuntimeException("XML documents with a DOCTYPE are not allowed.");
+    }
 }
diff --git a/core/src/test/java/apoc/export/graphml/ExportGraphMLTest.java b/core/src/test/java/apoc/export/graphml/ExportGraphMLTest.java
@@ -49,10 +49,12 @@
 import static apoc.util.BinaryTestUtil.fileToBinary;
 import static apoc.util.MapUtil.map;
 import static apoc.util.TestUtil.isRunningInCI;
+import static apoc.util.TestUtil.testResult;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeFalse;
 import static org.neo4j.configuration.GraphDatabaseSettings.TransactionStateMemoryAllocation.OFF_HEAP;
@@ -714,6 +716,34 @@ public void testExportGraphmlQueryWithStringCaptionCamelCase() {
         assertXMLEquals(output, EXPECTED_TYPES_PATH_CAMEL_CASE);
     }
 
+    @Test
+    public void testImportGraphmlPreventXXEVulnerabilityThrowsQueryExecutionException() {
+        QueryExecutionException e = assertThrows(QueryExecutionException.class,
+                () -> testResult(db, "CALL apoc.import.graphml('" + TestUtil.getUrlFileName("xml/xxe.xml") + "', {})", (r) -> {
+                    r.next();
+                    r.close();
+                })
+        );
+
+        Throwable except = ExceptionUtils.getRootCause(e);
+        assertTrue(except instanceof RuntimeException);
+        assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
+    }
+
+    @Test
+    public void testImportGraphmlPreventBillionLaughVulnerabilityThrowsQueryExecutionException() {
+        QueryExecutionException e = assertThrows(QueryExecutionException.class,
+                () -> testResult(db, "CALL apoc.import.graphml('" + TestUtil.getUrlFileName("xml/billion_laughs.xml") + "', {})", (r) -> {
+                    r.next();
+                    r.close();
+                })
+        );
+
+        Throwable except = ExceptionUtils.getRootCause(e);
+        assertTrue(except instanceof RuntimeException);
+        assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
+    }
+
     private void assertResults(File output, Map<String, Object> r, final String source) {
         assertCommons(r);
         assertEquals(output.getAbsolutePath(), r.get("file"));