[hcaaOlXR] Protect from Billion Laughs (#205)

- Owasp recommendation https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#xmlinputfactory-a-stax-parser - Leftshift recommendation https://www.shiftleft.io/blog/preventing-xxe-in-java-applications-02-12-2021/#xmlinputfactory - Disable Document Type Definitions (DTD) entirely.
neo4j · Oct 5, 2022 · cde5341 · cde5341
1 parent d22a145
commit cde5341
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 12 deletions.
diff --git a/core/src/main/java/apoc/load/Xml.java b/core/src/main/java/apoc/load/Xml.java
@@ -26,6 +26,7 @@
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
 import org.xml.sax.InputSource;
+import org.xml.sax.SAXParseException;
 
 import javax.xml.namespace.QName;
 import javax.xml.parsers.DocumentBuilder;
@@ -69,6 +70,7 @@ public class Xml {
     private static final XMLInputFactory FACTORY = XMLInputFactory.newFactory();
     static {
         FACTORY.setProperty(XMLInputFactory.IS_COALESCING, true);
+        FACTORY.setProperty(XMLInputFactory.SUPPORT_DTD, false);
         FACTORY.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
     }
 
@@ -148,6 +150,8 @@ private Stream<MapResult> parse(InputStream data, boolean simpleMode, String pat
         catch (Exception e){
             if(!failOnError)
                 return Stream.of(new MapResult(Collections.emptyMap()));
+            else if (e instanceof SAXParseException && e.getMessage().contains("DOCTYPE is disallowed"))
+                throw generateXmlDoctypeException();
             else
                 throw e;
         }
@@ -501,6 +505,9 @@ public Stream<NodeResult> importToGraph(@Name("urlOrBinary") Object urlOrBinary,
             xml.next();
 
             switch (xml.getEventType()) {
+                case XMLStreamConstants.DTD:
+                    throw generateXmlDoctypeException();
+
                 case XMLStreamConstants.START_DOCUMENT:
                     // xmlsteamreader starts off by definition at START_DOCUMENT prior to call next() - so ignore this one
                     break;
@@ -604,4 +611,7 @@ private void setPropertyIfNotNull(org.neo4j.graphdb.Node root, String propertyKe
         }
     }
 
+    private RuntimeException generateXmlDoctypeException() {
+        throw new RuntimeException("XML documents with a DOCTYPE are not allowed.");
+    }
 }
diff --git a/core/src/test/java/apoc/load/XmlTest.java b/core/src/test/java/apoc/load/XmlTest.java
@@ -15,7 +15,6 @@
 import org.neo4j.graphdb.ResourceIterator;
 import org.neo4j.test.rule.DbmsRule;
 import org.neo4j.test.rule.ImpermanentDbmsRule;
-import org.xml.sax.SAXParseException;
 
 import java.io.File;
 import java.nio.charset.Charset;
@@ -33,8 +32,6 @@
 import static apoc.util.MapUtil.map;
 import static apoc.util.TestUtil.*;
 import static java.util.Arrays.asList;
-import static org.hamcrest.CoreMatchers.containsString;
-import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -458,10 +455,25 @@ public void testLoadXmlPreventXXEVulnerabilityThrowsQueryExecutionException() {
                 r.next();
                 r.close();
             });
-        } catch (Exception e) {
+        } catch (QueryExecutionException e) {
             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());
+            assertTrue(except instanceof RuntimeException);
+            assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
+            throw e;
+        }
+    }
+
+    @Test(expected = QueryExecutionException.class)
+    public void testLoadXmlPreventBillionLaughVulnerabilityThrowsQueryExecutionException() {
+        try {
+            testResult(db, "CALL apoc.load.xml('" + TestUtil.getUrlFileName("xml/billion_laughs.xml") + "')", (r) -> {
+                r.next();
+                r.close();
+            });
+        } catch (QueryExecutionException e) {
+            Throwable except = ExceptionUtils.getRootCause(e);
+            assertTrue(except instanceof RuntimeException);
+            assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
             throw e;
         }
     }
@@ -474,10 +486,26 @@ public void testXmlParsePreventXXEVulnerabilityThrowsQueryExecutionException() {
                 r.next();
                 r.close();
             });
-        } catch (Exception e) {
+        } catch (QueryExecutionException e) {
             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());
+            assertTrue(except instanceof RuntimeException);
+            assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
+            throw e;
+        }
+    }
+
+    @Test(expected = QueryExecutionException.class)
+    public void testXmlParsePreventBillionLaughsVulnerabilityThrowsQueryExecutionException() {
+        try {
+            final var xml = "<?xml version=\"1.0\"?><!DOCTYPE lolz [<!ENTITY lol \"lol\"><!ENTITY lol1 \"&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;\">]><foo>&lol1;</foo>";
+            testResult(db, "RETURN apoc.xml.parse('" + xml + "')", (r) -> {
+                r.next();
+                r.close();
+            });
+        } catch (QueryExecutionException e) {
+            Throwable except = ExceptionUtils.getRootCause(e);
+            assertTrue(except instanceof RuntimeException);
+            assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
             throw e;
         }
     }
@@ -489,10 +517,25 @@ public void testImportXmlPreventXXEVulnerabilityThrowsQueryExecutionException()
                 r.next();
                 r.close();
             });
-        } catch (Exception e) {
+        } catch (QueryExecutionException e) {
+            Throwable except = ExceptionUtils.getRootCause(e);
+            assertTrue(except instanceof RuntimeException);
+            assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
+            throw e;
+        }
+    }
+
+    @Test(expected = QueryExecutionException.class)
+    public void testImportXmlPreventBillionLaughsVulnerabilityThrowsQueryExecutionException() {
+        try {
+            testResult(db, "CALL apoc.import.xml('" + TestUtil.getUrlFileName("xml/billion_laughs.xml") + "')", (r) -> {
+                r.next();
+                r.close();
+            });
+        } catch (QueryExecutionException e) {
             Throwable except = ExceptionUtils.getRootCause(e);
-            assertTrue(except instanceof com.ctc.wstx.exc.WstxParsingException);
-            assertThat(except.getMessage(), containsString("Encountered a reference to external entity \"xxe\""));
+            assertTrue(except instanceof RuntimeException);
+            assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
             throw e;
         }
     }

diff --git a/core/src/test/resources/xml/billion_laughs.xml b/core/src/test/resources/xml/billion_laughs.xml
@@ -0,0 +1,15 @@
+<?xml version="1.0"?>
+<!DOCTYPE lolz [
+        <!ENTITY lol "lol">
+        <!ELEMENT lolz (#PCDATA)>
+        <!ENTITY lol1 "&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;">
+        <!ENTITY lol2 "&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;">
+        <!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
+        <!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
+        <!ENTITY lol5 "&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;">
+        <!ENTITY lol6 "&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;">
+        <!ENTITY lol7 "&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;">
+        <!ENTITY lol8 "&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;">
+        <!ENTITY lol9 "&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;">
+        ]>
+<lolz>&lol9;</lolz>