[hcaaOlXR] Protect from Billion Laughs #205
Conversation
| assertTrue(except instanceof SAXParseException); | ||
| assertEquals("DOCTYPE is disallowed when the feature \"http://apache.org/xml/features/disallow-doctype-decl\" set to true.", except.getMessage()); |
There was a problem hiding this comment.
Should we be surfacing errors in the underlying library?
| @@ -69,6 +69,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); | |||
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for looking into this!! Let's merge :)
- 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.
Protects fully against XXE and Billion Laugh attacks by following the following recommendations: