XXE DoS within SVG Parsing

[prodigysml]

Issue

androidsvg is vulnerable to XXE attacks as some dangerous features are not disabled. This leads to a confirmed denial of service scenario (https://en.wikipedia.org/wiki/Billion_laughs_attack) and may lead to execution of commands on the server.

This issue occurs in the SVG parse section of the code:

androidsvg/androidsvg/src/main/java/com/caverock/androidsvg/SVGParser.java

Line 576 in 1f53713

SVG parse(InputStream is) throws SVGParseException

Prior to parsing the XML, features like entities are not disabled. These should not be required at all within an SVG file.

androidsvg/androidsvg/src/main/java/com/caverock/androidsvg/SVGParser.java

Line 606 in 1f53713

xr.parse(new InputSource(is));

Remediation

Implementing something down the lines of the following:

SAXParserFactory spf = SAXParserFactory.newInstance();
spf.setFeature("http://xml.org/sax/features/external-general-entities", false);
spf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
spf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);

This ensures that the entities are no longer parsed and external dtd files are not either.

[BigBadaboom]

Thanks for the report.

Entities do have their uses. The SVG spec itself contains examples that use entity expansion. I am reluctant to completely disable them. Especially considering none of the browsers have either.

But it does make sense to either:
(a) check whether the Android SAX parser has an entity expansion limit. If it does, check that it is set appropriately, or
(b) if we can't set a limit, then disable entity expansion by default, but allow the user to enable it if necessary.

FMR: https://docs.oracle.com/javase/tutorial/jaxp/limits/using.html

[prodigysml]

Fair enough! Thanks for that! Didn't actually know svgs use entities. Both approaches seem like a good idea! Thanks for being so prompt!

[BigBadaboom]

Investigations have shown that the expat parser built into Android (which is used by the SAXParser which AndroidSVG uses) does not currently support any limits on entity expansion. It is still an open bug.

libexpat/libexpat#34

Although there is a way to turn off external entity expansion, there doesn't seem to be a way to turn off the expansion of internal entities. Internal entities are those defined in the XML file itself. Expat doesn't seem to support the startEntity(), endEntity(), internalEntityDecl(), and the other entity-related SAX methods either. So there doesn't even seem to be a way to intercept them, before expansion happens.

It might mean that I either:
(a) have to move to the XmlPullParser, which I believe doesn't support entities at all, or
(b) Add a security warning to the documentation, reminding users to not trust external SVG files

[prodigysml]

To be honest, considering there isn't much easily doable, adding a security warning seems reasonable. (Bonus if it's in red :) )

[BigBadaboom]

Fixed. This change will be in the 1.3 release.

I have implemented a dual XML parser approach. The SVG parser now starts out using the faster XMLPullParser (which doesn't support entities). If it detects entity declarations, it will reset the input stream and switch to the SAX parser (which does support entities).

If you want to disable entity parsing, for security reasons, you can now call the new method setInternalEntitiesEnabled(false)

[TWiStErRob]

@BigBadaboom Is there a way to always use the one that supports entity parsing? I use it very extensively and don't want to pay the price of resetting the stream. My image sources are secure, I hand-write them.

[BigBadaboom]

@TWiStErRob Not at the moment. If you are finding that this change has had a significant detrimental impact on your app's performance, please open a new issue.

[TWiStErRob]

Haven't tested yet, maybe just being paranoid; I'll keep an eye out. Thank you!