Reduce chance of XXE

[billoneil]

In FasterXML/jackson-dataformat-xml#190 it seems like the following are disabled by default

        // as per [dataformat-xml#190], disable external entity expansion by default
        xmlIn.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, Boolean.FALSE);
        // and ditto wrt [dataformat-xml#211], SUPPORT_DTD
        xmlIn.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.FALSE);

https://github.com/FasterXML/jackson-dataformat-xml/blob/c626be64b87394804ce516a9d31b575270472118/src/main/java/com/fasterxml/jackson/dataformat/xml/XmlFactoryBuilder.java#L102-L105

However, in woodstox this appears to be enabled by default.

https://github.com/FasterXML/woodstox/blob/master/src/main/java/com/ctc/wstx/api/ReaderConfig.java#L182-L183

Should these be disabled by default as well and where would one do that?

I am able to fix this in code with the following.

WstxInputFactory inputFactory = new WstxInputFactory();
inputFactory.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, Boolean.TRUE);
inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.FALSE);

Should these be off by default here as well?

[cowtowncoder]

Default settings for these properties are mandated by Stax API (javax.xml.stream) specification, as far as I remember.
So I would not want to change these unless specification either mandates such settings or leaves them up to implementation.

In latter case (unspecified, left up to impl) there would then be the question of backwards incompatibility so change would at least have to go in a minor update, or perhaps even major. This change could easily break existing systems where resolution of external parsed entities is required, and that do not explicitly enable it.

So: if you find Stax API notes to allow this I can consider it. From security perspective this could be beneficial.

[billoneil]

@cowtowncoder I had similar concerns. Is there anywhere else in Jackson where passing in a WstxInputFactory might make sense to override / default the property instead of changing the default here?

We don't hit the block in XmlFactory because the InputFactory is not null.

If not I think this is reasonable to close.

[cowtowncoder]

If there are code paths where additional overrides are needed, it'd be good to know -- I am not aware of any.
But right now I do not have time to go hunting for those (my OSS time is spread across all Jackson things and limited overall) so it's up to user reports unfortunately.

As to this issue: as I said, I would consider change if it was allowed but last I remember Stax specification (combination of JSR-173, javax.xml.stream Javadocs) did specify settings that Woodstox uses. So I'd need to see some documentation to change my understanding, if someone has time to go dig up the latest state of things.

[billoneil]

Sounds good, closing this.