Skip to content

Commit cde5341

Browse files
AzuObsAzuObs
authored
[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.
1 parent d22a145 commit cde5341

File tree

3 files changed

+80
-12
lines changed

3 files changed

+80
-12
lines changed

‎core/src/main/java/apoc/load/Xml.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.w3c.dom.Node;
2727
import org.w3c.dom.NodeList;
2828
import org.xml.sax.InputSource;
29+
import org.xml.sax.SAXParseException;
2930

3031
import javax.xml.namespace.QName;
3132
import javax.xml.parsers.DocumentBuilder;
@@ -69,6 +70,7 @@ public class Xml {
6970
private static final XMLInputFactory FACTORY = XMLInputFactory.newFactory();
7071
static {
7172
FACTORY.setProperty(XMLInputFactory.IS_COALESCING, true);
73+
FACTORY.setProperty(XMLInputFactory.SUPPORT_DTD, false);
7274
FACTORY.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
7375
}
7476

@@ -148,6 +150,8 @@ private Stream<MapResult> parse(InputStream data, boolean simpleMode, String pat
148150
catch (Exception e){
149151
if(!failOnError)
150152
return Stream.of(new MapResult(Collections.emptyMap()));
153+
else if (e instanceof SAXParseException && e.getMessage().contains("DOCTYPE is disallowed"))
154+
throw generateXmlDoctypeException();
151155
else
152156
throw e;
153157
}
@@ -501,6 +505,9 @@ public Stream<NodeResult> importToGraph(@Name("urlOrBinary") Object urlOrBinary,
501505
xml.next();
502506

503507
switch (xml.getEventType()) {
508+
case XMLStreamConstants.DTD:
509+
throw generateXmlDoctypeException();
510+
504511
case XMLStreamConstants.START_DOCUMENT:
505512
// xmlsteamreader starts off by definition at START_DOCUMENT prior to call next() - so ignore this one
506513
break;
@@ -604,4 +611,7 @@ private void setPropertyIfNotNull(org.neo4j.graphdb.Node root, String propertyKe
604611
}
605612
}
606613

614+
private RuntimeException generateXmlDoctypeException() {
615+
throw new RuntimeException("XML documents with a DOCTYPE are not allowed.");
616+
}
607617
}

‎core/src/test/java/apoc/load/XmlTest.java‎

Lines changed: 55 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import org.neo4j.graphdb.ResourceIterator;
1616
import org.neo4j.test.rule.DbmsRule;
1717
import org.neo4j.test.rule.ImpermanentDbmsRule;
18-
import org.xml.sax.SAXParseException;
1918

2019
import java.io.File;
2120
import java.nio.charset.Charset;
@@ -33,8 +32,6 @@
3332
import static apoc.util.MapUtil.map;
3433
import static apoc.util.TestUtil.*;
3534
import static java.util.Arrays.asList;
36-
import static org.hamcrest.CoreMatchers.containsString;
37-
import static org.hamcrest.MatcherAssert.assertThat;
3835
import static org.junit.Assert.assertEquals;
3936
import static org.junit.Assert.assertFalse;
4037
import static org.junit.Assert.assertNotNull;
@@ -458,10 +455,25 @@ public void testLoadXmlPreventXXEVulnerabilityThrowsQueryExecutionException() {
458455
r.next();
459456
r.close();
460457
});
461-
} catch (Exception e) {
458+
} catch (QueryExecutionException e) {
462459
Throwable except = ExceptionUtils.getRootCause(e);
463-
assertTrue(except instanceof SAXParseException);
464-
assertEquals("DOCTYPE is disallowed when the feature \"http://apache.org/xml/features/disallow-doctype-decl\" set to true.", except.getMessage());
460+
assertTrue(except instanceof RuntimeException);
461+
assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
462+
throw e;
463+
}
464+
}
465+
466+
@Test(expected = QueryExecutionException.class)
467+
public void testLoadXmlPreventBillionLaughVulnerabilityThrowsQueryExecutionException() {
468+
try {
469+
testResult(db, "CALL apoc.load.xml('" + TestUtil.getUrlFileName("xml/billion_laughs.xml") + "')", (r) -> {
470+
r.next();
471+
r.close();
472+
});
473+
} catch (QueryExecutionException e) {
474+
Throwable except = ExceptionUtils.getRootCause(e);
475+
assertTrue(except instanceof RuntimeException);
476+
assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
465477
throw e;
466478
}
467479
}
@@ -474,10 +486,26 @@ public void testXmlParsePreventXXEVulnerabilityThrowsQueryExecutionException() {
474486
r.next();
475487
r.close();
476488
});
477-
} catch (Exception e) {
489+
} catch (QueryExecutionException e) {
478490
Throwable except = ExceptionUtils.getRootCause(e);
479-
assertTrue(except instanceof SAXParseException);
480-
assertEquals("DOCTYPE is disallowed when the feature \"http://apache.org/xml/features/disallow-doctype-decl\" set to true.", except.getMessage());
491+
assertTrue(except instanceof RuntimeException);
492+
assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
493+
throw e;
494+
}
495+
}
496+
497+
@Test(expected = QueryExecutionException.class)
498+
public void testXmlParsePreventBillionLaughsVulnerabilityThrowsQueryExecutionException() {
499+
try {
500+
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>";
501+
testResult(db, "RETURN apoc.xml.parse('" + xml + "')", (r) -> {
502+
r.next();
503+
r.close();
504+
});
505+
} catch (QueryExecutionException e) {
506+
Throwable except = ExceptionUtils.getRootCause(e);
507+
assertTrue(except instanceof RuntimeException);
508+
assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
481509
throw e;
482510
}
483511
}
@@ -489,10 +517,25 @@ public void testImportXmlPreventXXEVulnerabilityThrowsQueryExecutionException()
489517
r.next();
490518
r.close();
491519
});
492-
} catch (Exception e) {
520+
} catch (QueryExecutionException e) {
521+
Throwable except = ExceptionUtils.getRootCause(e);
522+
assertTrue(except instanceof RuntimeException);
523+
assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
524+
throw e;
525+
}
526+
}
527+
528+
@Test(expected = QueryExecutionException.class)
529+
public void testImportXmlPreventBillionLaughsVulnerabilityThrowsQueryExecutionException() {
530+
try {
531+
testResult(db, "CALL apoc.import.xml('" + TestUtil.getUrlFileName("xml/billion_laughs.xml") + "')", (r) -> {
532+
r.next();
533+
r.close();
534+
});
535+
} catch (QueryExecutionException e) {
493536
Throwable except = ExceptionUtils.getRootCause(e);
494-
assertTrue(except instanceof com.ctc.wstx.exc.WstxParsingException);
495-
assertThat(except.getMessage(), containsString("Encountered a reference to external entity \"xxe\""));
537+
assertTrue(except instanceof RuntimeException);
538+
assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed.");
496539
throw e;
497540
}
498541
}

‎core/src/test/resources/xml/billion_laughs.xml‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?xml version="1.0"?>
2+
<!DOCTYPE lolz [
3+
<!ENTITY lol "lol">
4+
<!ELEMENT lolz (#PCDATA)>
5+
<!ENTITY lol1 "&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;">
6+
<!ENTITY lol2 "&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;">
7+
<!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
8+
<!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
9+
<!ENTITY lol5 "&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;">
10+
<!ENTITY lol6 "&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;">
11+
<!ENTITY lol7 "&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;">
12+
<!ENTITY lol8 "&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;">
13+
<!ENTITY lol9 "&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;">
14+
]>
15+
<lolz>&lol9;</lolz>

0 commit comments

Comments
 (0)