Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hcaaOlXR] Protect from Billion Laughs #205

Merged
merged 1 commit into from
Oct 5, 2022
Merged

[hcaaOlXR] Protect from Billion Laughs #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions core/src/main/java/apoc/load/Xml.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a Java expert, but what is the difference between this implementation and what OWASP snippet looks like: xmlInputFactory.setProperty("javax.xml.stream.isSupportingExternalEntities", false);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also , what about this one:
// This causes XMLStreamException to be thrown if external DTDs are accessed. xmlInputFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");.
I guess it is not needed , because right above we set SUPPORT_DTD to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a Java expert, but what is the difference between this implementation and what OWASP snippet looks like: xmlInputFactory.setProperty("javax.xml.stream.isSupportingExternalEntities", false);

FACTORY.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); is the same as the line above. It's just using the Enum object rather than writing it out as a String.

also , what about this one:
// This causes XMLStreamException to be thrown if external DTDs are accessed.
xmlInputFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");.
I guess it is not needed , because right above we set SUPPORT_DTD to false?

I think the OWASP article was for an older version of Java perhaps, where the StAX parser returned was using this configuration. The default parser Java is giving us does not use the setting.

The setting is not entirely needed however as it simply throws an exception whenever the parser encounters a <!DOCTYPE>. Currently what our implementation is doing is that it simply ignores the <!DOCTYPE> and will throw an error if it encounters an unknown entity (eg one that was declared the in DOCTYPE)

This is not a bad thing really as it makes my change a bit more backwards compatible while still being safe.

It's also perfectly aligned with the LeftShift (eg competitor to Snyk and Veracode) recommendation which is to implement exactly what I've done. It's mostly aligned with the OWASP article but only because I suspect the OWASP article is quite old and doesn't fully work anymore.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this!! Let's merge :)

}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.");
}
}
67 changes: 55 additions & 12 deletions core/src/test/java/apoc/load/XmlTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.neo4j.internal.helpers.collection.Iterators;
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;
Expand All @@ -32,8 +31,6 @@
import static apoc.util.CompressionConfig.COMPRESSION;
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;
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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;
}
}
Expand All @@ -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;
}
}
Expand Down
15 changes: 15 additions & 0 deletions core/src/test/resources/xml/billion_laughs.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>