Harden XML parsing via copernik-xml-factory

[ppkarwasz]

Summary

Route every direct JAXP factory instantiation through the hardened factories from eu.copernik:copernik-xml-factory 0.1.1. These factories block external DTD and entity fetching and bound internal entity expansion, regardless of the JAXP implementation on the classpath.

Hardening the parsing of a configuration file is admittedly not necessary: configuration files are normally trusted. This limits the side-effects if a user (against advice) decides to parse untrusted configuration files.

Changes

• Add the copernik-xml-factory dependency.
• Route factory creation through XmlFactories (DocumentBuilderFactory, SAXParserFactory, TransformerFactory) in XMLConfiguration, XMLDocumentHelper, XMLPropertiesConfiguration and XMLPropertyListConfiguration, plus the affected tests.
• Harden the source passed to XMLDocumentHelper.transform.
• XMLConfiguration's entity resolver now throws a SAXException for unregistered entities instead of returning null. Returning null would let the parser fall back to fetching the external resource and would override the deny-all resolver the hardening factories install on some implementations. This is done in an anonymous DefaultEntityResolver subclass local to XMLConfiguration, leaving the public DefaultEntityResolver contract (return null for unknown entities) unchanged.

Testing

• mvn test for the affected XML test classes: 145 tests, all passing.
• mvn checkstyle:check: clean.

[ppkarwasz]

Converting this to draft, since some Java-version dependent problems arose.

[garydgregory]

-1: We've already discussed making an new Commons component for this code, let's do that please.

[ppkarwasz]

-1: We've already discussed making an new Commons component for this code, let's do that please.

Sorry, I was busy iterating on code and I forgot to mention that I created this PR only to test the capabilities (or missing capabilities) of copernik-xml-factory, before it gets included as a separate Commons component.

For a new component to be useful, it should be adopted by at least a couple of projects. Currently I have tested support for such a library:

• In Log4j (Make XInclude opt-in and rework XML configuration hardening logging-log4j2#4144), where it appears that the library should have separate self-contained public classes, so that code necessary by a single JAXP implementation can be shaded. This does not exclude the presence of an XmlFactories common utility,
• In Batik (BATIK-1395), where the idea of replacing stock JDK-specific hardenings with an external implementation-independent library didn't find good ground,
• Calcite (CALCITE-7609) is interested, so I will test how well adapted the library is to their code,
• In this PR I am exploring the applicability of the library to Configuration. I already found one bug (Custom EntityResolver returning null can undo hardening on non-JAXP-1.5 parsers (Xerces, Android) copernik-eu/copernik-xml-factory#28) and a problem with JDK 8 and schema validation.

[garydgregory]

Shading can cause more problems than it's worth as exemplified with the mess some projects make shading ASM. When downstream users blow up because they can't update ASM to test or support a new Java version since that code is now a hard copy 🤔

[ppkarwasz]

JDK 8 is affected by a bug, which denies access based on ACCESS_EXTERNAL_SCHEMA before consulting the resolver. This is why tests with xsi:schemaLocation/xsi:noNamespaceSchemaLocation hints fail.

This was theoretically fixed by JDK-8159139, but still doesn't work.

[ppkarwasz]

Shading can cause more problems than it's worth as exemplified with the mess some projects make shading ASM.

I totally agree, but each project should be able to choose its poison.

Unlike ASM, however, XML support is unlikely to change very much in the future, so if a project decides to replace a bunch of cargo-culted hardening options without any tests, with a shaded and relocated class, this still counts as a win.

[garydgregory]

Thanks for the clarifications Piotr. Let's bring the component into Commons and iterate there. It will become more visible and it doesn't need to be perfect or bug free on the first commit. We'll want to create an alpha or milestone or pick-your-name release before a 1.0 anyway. WDYT?

[ppkarwasz]

This was theoretically fixed by JDK-8159139, but still doesn't work.

Apparently OpenJDK never got the fix, probably the fix is only present in Oracle's builds. This mean that on JDK 8 parsing this file will always fail if ACCESS_EXTERNAL_SCHEMA does not contain https. This happens even if an EntityResolver is set.

<configuration
           xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
           xsi:noNamespaceSchemaLocation="https://commons.apache.org/testMultiConfiguration.xsd">
  ...
</configuration>

This is a bummer, because on JDK 9+ libraries can choose to call DocumentBuilderFactory.newDefaultInstance(), which solves the problem of implementation-specific hardenings altogether. Almost: Android's default instance will be different from the one in the JDK and will not accept the same options.

[ppkarwasz]

We'll want to create an alpha or milestone or pick-your-name release before a 1.0 anyway. WDYT?

Sounds good to me: please choose a repository name and I'll transfer the code to Commons. Do we need a new JIRA project too?

[garydgregory]

Here we go:

• https://github.com/apache/commons-xml
• I asked for a COMMONSXML Jira project but it's not there yet. Note that there already is a XMLCOMMONS Jira project but I can't tell what that's part of, maybe Xerces.