Harden GMLReader against XXE (disable DTDs and external entities)

[Nexory]

Problem

GMLReader.read() builds its SAXParserFactory with only setNamespaceAware(false) and setValidating(false), so DOCTYPE processing and external entity resolution stay enabled. GML is routinely read from untrusted input (files, WFS responses, uploads), so a crafted document can disclose local files or trigger SSRF via an external entity (XXE):

<!DOCTYPE foo [ <!ENTITY xxe SYSTEM "file:///etc/hostname"> ]>
<gml:Point><gml:coordinates>&xxe;</gml:coordinates></gml:Point>

Fix

Enable JAXP secure processing and disable DTDs / external entities on the factory (disallow-doctype-decl, external-general-entities, external-parameter-entities, load-external-dtd). This is the same hardening merged for the sibling KMLReader in #1204 (which set SUPPORT_DTD=false and IS_SUPPORTING_EXTERNAL_ENTITIES=false); GMLReader uses SAX (SAXParserFactory) rather than StAX, so the equivalent is expressed as parser features, but the effect (no DTDs, no external entities) is identical. No behaviour change for valid GML, and no signature change (setFeature only throws SAXException subclasses, already declared). read(String, ...) delegates to read(Reader, ...), so both public entry points are covered.

Test

GMLReaderXXETest demonstrates the bug in the absence of the fix (per CONTRIBUTING): without the change testExternalEntityIsNotResolved leaks the referenced file content and testDoctypeIsRejected fails; with it both pass and testBenignGmlStillParses confirms valid GML is unaffected. Full jts-core suite: 2298 tests, 0 failures.

[ppkarwasz]

The three additional features are over-kill: without a DOCTYPE declaration, there is no external subset nor external entities.

Also note that this will fail on Android and any JAXP implementation that doesn't support FEATURE_SECURE_PROCESSING (required by the JAXP specification) or disallow-doctype-decl (only supported by Xerces-derivatives). Even if Android's expat based parser does not support these feature, it is in practice safe to use, because it does not resolve entities by default.

Shameless advertising: we will release Apache Commons XML in the next couple of weeks, which handles all these subtleties of JAXP implementations. The library can be both used as external dependency or shaded with <minimizeJar> and delegates the hassle of properly configuring an XML parser upstream.

[Nexory]

Thanks, both points are right and the push has them.

On the portability one: SAXParserFactory.setFeature throws
SAXNotRecognizedException for a name the implementation does not know, and
that class extends SAXException, which read() already declares. The old code
therefore compiled and would have aborted the read at runtime, in a way the
caller cannot tell apart from malformed XML. Each feature is now applied through
a helper that ignores an unsupported one.

On the three extra features, I first wanted to keep them and argue that they
are the remaining layer where disallow-doctype-decl does not apply. That was
only ever measured on Xerces with the DOCTYPE feature switched off, which is not
the same thing, so I tried it on a parser that is not a Xerces derivative:
crimson 1.1.3, selected through javax.xml.parsers.SAXParserFactory.

feature	Xerces (JDK 8)	crimson 1.1.3
FEATURE_SECURE_PROCESSING	accepted	SAXNotRecognizedException
disallow-doctype-decl	accepted	SAXNotRecognizedException
external-general-entities	accepted	SAXNotSupportedException
external-parameter-entities	accepted	SAXNotSupportedException
nonvalidating/load-external-dtd	accepted	SAXNotRecognizedException

All five throw there, including the two in the SAX namespace, so those three are
not a fallback on such an implementation. They are gone. What is left is
FEATURE_SECURE_PROCESSING and disallow-doctype-decl, each applied on its own.

One consequence of the guard is worth writing down, since it is the trade you
are pointing at. On crimson the same XXE payload now parses and resolves the
external entity, where the unguarded version would have thrown on the first
setFeature. The guard turns a loud failure into a silent absence of hardening.
That is the right call for a reader that must keep working, but it means the
hardening is only as good as the parser in use, and nothing in the API says
which one that is.

Two corrections to my own PR while I am here. The description claimed
setFeature only throws SAXException subclasses; it also declares
ParserConfigurationException, which is not one. And the new test asserted only
inside a catch, so it would have passed without running a single assertion had
read() returned normally. Both are fixed.

A new dependency is out of scope for this PR.

[Nexory]

You are right, and the suggestion is in.

I had written that the guard turns a loud failure into a silent absence of
hardening, and then kept the guard anyway. Making security not optional is the
better answer to that, and skipping the one parser that is safe unconfigured is
cheaper than trying to configure everything.

Measured on the same XXE payload, guarded against unguarded:

	Xerces (JDK 8)	crimson 1.1.3
guarded	DOCTYPE rejected	parses, resolves the entity
unguarded, Android skipped	DOCTYPE rejected	throws while configuring

So on a parser that cannot be configured the read now fails instead of quietly
running without the hardening.

I took the factory class name from AOSP rather than the suggestion alone:
luni/.../org/apache/harmony/xml/parsers/SAXParserFactoryImpl.setFeature rejects
every name outside http://xml.org/sax/features/, and ExpatReader.setFeature
returns immediately for the two SAX ones when the value is false, with a comment
that it is already the default. That matches what you describe. I have not run
this on a device, so the Android half rests on reading the source and on your
test suite, not on a measurement of mine.

mvn clean install on JDK 8 in a pinned container: 2467 tests, none failing.
The new test goes from 2 failures to 0 across the change.

[jodygarnett]

Thanks for the suggestion/improvement. Some feedback provided below.

[Nexory]

Done, all three:

• the comment is one line and the reasoning moved here
• the two settings are in a try/catch that logs a warning
• the tests are in GMLReaderTest and the separate class is gone. I dropped the
  third one, a benign parse, since GMLReaderTest already covers that.

The try/catch also removed the factory class name check, so that is one less
thing to maintain.

Two things I cannot decide, both yours.

jts-core has no dependencies, and there is no logging anywhere under
modules/*/src/main, so I used java.util.logging. It would be the first in the
module. Happy to drop it, or to use whatever you prefer.

The other is the trade-off the try/catch makes. Measured on the same payload:

	Xerces (JDK 8)	crimson 1.1.3
try/catch and warn	DOCTYPE rejected	parses, resolves the entity
features set unguarded	DOCTYPE rejected	throws while configuring

So on a parser that cannot be configured, the reader now parses untrusted GML
without the hardening and leaves a log line. Android is unaffected either way,
since its parser does not resolve external references.

While checking for duplicates I found that KMLReader was hardened in #1204 and
does it unguarded:

// Disable DTDs completely (prevents DOCTYPE declarations)
inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
// Prevent external entity expansion from DTDs
inputFactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);

The APIs differ in whether they force the question: setProperty throws an
unchecked exception, while setFeature throws checked ones. read already
declares SAXException and ParserConfigurationException, so leaving the calls
unguarded compiles as it stands, and would match the sibling reader.

I have built it the way you asked. Say which of the two you want and I will make
it that.

mvn clean install on JDK 8 in a pinned container: 2467 tests across the five
modules, none failing. GMLReaderTest is 14 tests and goes from 2 failures to 0
across the change.