Harden GMLReader against XXE (disable DTDs and external entities) by Nexory · Pull Request #1221 · locationtech/jts

Nexory · 2026-08-06T13:47:24Z

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

Two features on the factory: FEATURE_SECURE_PROCESSING and disallow-doctype-decl. Without a DOCTYPE there is no external subset and no entity to resolve, so the three further entity features an earlier revision set were dropped as redundant; eight inputs were compared across both revisions with no difference in outcome.

This mirrors the hardening merged for the sibling KMLReader in #1204, which set SUPPORT_DTD=false and IS_SUPPORTING_EXTERNAL_ENTITIES=false. GMLReader uses SAX rather than StAX, so the equivalent is expressed as parser features. No behaviour change for valid GML. read(String, ...) delegates to read(Reader, ...), so both public entry points are covered.

The two calls currently sit in a try/catch that logs a warning, at the request of @jodygarnett. That shape is under discussion in the review: see the open question about failing closed instead.

Test

Two tests in GMLReaderTest: one asserts the referenced file content does not appear in the parsed output, on both the success and the exception path, and one asserts the DOCTYPE is rejected. Full jts-core suite: 2298 tests, 0 failures, unchanged against master.

ppkarwasz · 2026-08-24T15:57:57Z

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


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 · 2026-08-24T19:24:39Z

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 · 2026-08-24T21:01:37Z

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 · 2026-08-26T18:46:36Z

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.

grootstebozewolf

Request changes. Jody’s mechanical notes are done (short comment, tests in GMLReaderTest, try/catch). The remaining problem is that this version fail-opens: on a parser that does not accept the two features, untrusted GML is still read and XXE still works. That is the author’s crimson 1.1.3 table in #1221 (comment).

KMLReader (#1204) sets SUPPORT_DTD / IS_SUPPORTING_EXTERNAL_ENTITIES with no swallow. Match that.

Blockers

One try wraps both features. If FEATURE_SECURE_PROCESSING throws, disallow-doctype-decl is never attempted. Split them, or do not swallow.
Swallowing turns a config failure into a working XXE. Set the two features unguarded (read already declares the exception types). Skip only org.apache.harmony.xml.parsers.SAXParserFactoryImpl by class name. Android is safe unconfigured; crimson is not. Everywhere else, fail the read.
java.util.logging is new to jts-core. Do not mint the module’s first JUL sink for a warning that does not close the hole. Do not add Commons XML.

Nits: empty catch on the DOCTYPE test is fine for this JUnit 3 file; PR body still mentions GMLReaderXXETest and the extra features; commits 2 and 3 need Signed-off-by.

grootstebozewolf · 2026-09-03T03:54:00Z

+		try {
+			fact.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+			fact.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+		}
+		catch (SAXNotRecognizedException | SAXNotSupportedException | ParserConfigurationException e) {
+			Logger.getLogger(GMLReader.class.getName())
+					.log(Level.WARNING, "SAX parser does not support XXE hardening", e);
+		}


This is fail-open, and one try wraps both features.

If FEATURE_SECURE_PROCESSING throws, disallow-doctype-decl is never attempted. If errors are going to be ignored, each setFeature needs its own try/catch; otherwise a later supported hardening is skipped.

Worse, swallowing turns a config failure into a working XXE. That is not hypothetical — the crimson 1.1.3 table in #1221 (comment) shows the try/catch path parses the payload and resolves the entity, while the unguarded path throws while configuring.

KMLReader (#1204) sets SUPPORT_DTD / IS_SUPPORTING_EXTERNAL_ENTITIES with no swallow. Match that: set the two features unguarded (read already declares the exception types). Skip only org.apache.harmony.xml.parsers.SAXParserFactoryImpl by class name (three lines, no essay). Android rejects names outside the SAX namespace and does not resolve external references, so it is safe unconfigured. Crimson is not. Everywhere else, fail the read rather than parse untrusted GML unhardened.

java.util.logging is also new to jts-core — there is no logger under modules/*/src/main today. Do not mint one for a warning that does not close the hole. If the catch stays, drop the log; if the catch goes, the imports go with it. Do not add Commons XML.

grootstebozewolf · 2026-09-03T03:54:01Z

+  public void testExternalEntityIsNotResolved() throws Exception {
+    File secretFile = File.createTempFile("jts-xxe", ".txt");
+    String secret = "JTS-XXE-CANARY-SECRET";
+    Files.write(secretFile.toPath(), secret.getBytes("UTF-8"));
+    try {
+      String gml = "<?xml version=\"1.0\"?>\n"
+          + "<!DOCTYPE foo [ <!ENTITY xxe SYSTEM \"" + secretFile.toURI() + "\"> ]>\n"
+          + "<gml:Point><gml:coordinates>&xxe;</gml:coordinates></gml:Point>";
+      String observed;
+      try {
+        observed = String.valueOf(new GMLReader().read(gml, null));
+      }
+      catch (Exception e) {
+        observed = String.valueOf(e.getMessage());
+      }
+      assertFalse(observed.contains(secret));
+    }
+    finally {
+      secretFile.delete();
+    }
+  }


Canary is structured correctly: the secret assert runs on both the success and exception paths, so this cannot pass by never asserting (the first-revision bug). Enough to lock the JDK/Xerces case.

It does not lock the fail-open path. On a parser that rejects both features, read now returns a geometry and this test still passes as long as the canary string is absent for some other reason. That is why the factory change needs to fail closed rather than relying on this test alone.

grootstebozewolf · 2026-09-03T03:54:02Z

+    String gml = "<?xml version=\"1.0\"?>\n<!DOCTYPE foo>\n"
+        + "<gml:Point><gml:coordinates>5,10</gml:coordinates></gml:Point>";
+    try {
+      new GMLReader().read(gml, null);
+      fail("expected a DOCTYPE to be rejected");
+    }
+    catch (Exception e) {
+    }
+  }


Fine for this JUnit 3 file. The empty catch only proves some exception, which is enough next to the canary test. No need to grow a separate GMLReaderXXETest again.

grootstebozewolf · 2026-09-03T03:54:03Z

The two-feature set is the right pair on JDK Xerces: without a DOCTYPE there is no subset and no entity to resolve. The three extra Apache/SAX entity features were correctly dropped.

read(String, …) already delegates to read(Reader, …), so one factory site covers both entry points. Direct GMLHandler use is caller-owned and out of scope; mention that in the class javadoc if you want, not as a blocker.

Jody’s mechanical notes (short comment, tests in GMLReaderTest, try/catch) are done. The remaining product question is fail-closed vs warn-and-continue — please fail closed, as above.

Also: PR body still talks about GMLReaderXXETest and the three extra features; update it. Commits 2 and 3 have no Signed-off-by (the first commit does); Eclipse DCO wants every commit.

ppkarwasz · 2026-09-03T07:43:07Z

We have just started the release process for Apache Commons Secure XML (see the release vote), so I drafted #1230 as an alternative to this PR. The main difference is the failure mode: where this PR logs a warning and continues with an unhardened parser if a feature is not recognized, Commons Secure XML fails closed by contract.

The trade-off is straightforward: about 10 lines of hand-written hardening, the way it has been done for the past 25 years, versus an external dependency of roughly 70 KiB (about 10 KiB if shaded). Both are valid choices, and #1230 exists so the maintainers can compare them side by side.

Not sure if it is relevant for JTS users, but Commons Secure XML also hardens Android as a best effort. Android's expat-based parser is rather secure by default (although the details depend on the expat version used), so it is whitelisted, while on the JVM fail-close is part of the contract.

GMLReader configured the SAX parser with only namespace-awareness and validation disabled, leaving DOCTYPE processing and external entity resolution enabled. GML is commonly read from untrusted sources (files, WFS responses, uploads), so a crafted document could disclose local files or trigger SSRF via an external entity (XXE). Enable JAXP secure processing and disable DTDs and external entities on the SAXParserFactory. There is no behaviour change for valid GML, and no signature change (setFeature only throws SAXException subclasses, which are already declared). This mirrors the KMLReader hardening in locationtech#1204. Adds GMLReaderXXETest: without the fix the external entity is resolved and a DOCTYPE is accepted; with it both are rejected and benign GML still parses. Signed-off-by: Nexory <St4yl3r30@hotmail.de>

SAXParserFactory.setFeature throws SAXNotRecognizedException for a feature name the implementation does not recognize, and that class extends SAXException, which read() already declares. The first version of this change therefore compiled but would abort the read on such an implementation, and the caller could not tell that apart from malformed XML. Rejecting the DOCTYPE is what does the work: without one there is no internal or external subset, so no entity can be declared in the first place. The three other feature names are gone, since they add nothing where that applies. Android's parser is skipped by name. It refuses every feature outside the SAX namespace and does not resolve external references anyway, so there is nothing to configure. Everywhere else the features are set without a guard: measured on crimson 1.1.3, a guarded version parses the XXE payload and resolves the entity, while an unguarded one throws while configuring. A parser that cannot be configured should fail here rather than read untrusted input unhardened. Also fixes two problems in the test: it asserted only inside the catch block, so it would have passed without running a single assertion had read() returned normally, and it was missing the license header that CONTRIBUTING.md requires. Signed-off-by: Nexory <St4yl3r30@hotmail.de>

- Set the parser features in a try/catch and log a warning instead of letting an unconfigurable parser fail the read. This also removes the factory class name check. - Shorten the comment. - Move the tests into GMLReaderTest and drop the benign parse case, which GMLReaderTest already covers. Signed-off-by: Nexory <St4yl3r30@hotmail.de>

Nexory · 2026-09-03T17:48:19Z

Thanks, both of your code points hold when I check them.

The single try does wrap both calls, so a throw on FEATURE_SECURE_PROCESSING
skips disallow-doctype-decl entirely. And on the logger: there is no
java.util.logging and no other logging framework anywhere under src/main in
any module today, so this would be the first, for a warning that does not close
the hole.

For the record on how that shape got there: the unguarded version was what I had
after the crimson measurement, and the try/catch came in at @jodygarnett's
request. The measurement in that table argues the way you do, so the question is
his to settle rather than mine to relitigate. Whichever of the three shapes the
maintainers pick, guarded per feature, fail closed with the Android factory
skipped by class name, or #1230, I will implement it.

Two things you raised are fixed either way:

All three commits now carry Signed-off-by. The rebase that added it also
had to restore the SSH signatures, which the first attempt silently dropped.
The description no longer mentions GMLReaderXXETest or the three removed
features.

Nexory closed this Aug 6, 2026

Nexory reopened this Aug 6, 2026

ppkarwasz reviewed Aug 24, 2026

View reviewed changes

Comment thread modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java Outdated

Nexory force-pushed the harden/gmlreader-xxe branch from 1930b13 to 32c2d6d Compare August 24, 2026 21:00

ppkarwasz approved these changes Aug 24, 2026

View reviewed changes

jodygarnett requested changes Aug 26, 2026

View reviewed changes

Comment thread modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java Outdated

Comment thread modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java Outdated

grootstebozewolf suggested changes Sep 3, 2026

View reviewed changes

This was referenced Sep 3, 2026

[CALCITE-7601] Harden ST_GeomFromGML against external entity expansion apache/calcite#5006

Open

Harden GMLReader against XXE (use Commons Secure XML) #1230

Open

Nexory added 3 commits September 3, 2026 19:26

Nexory force-pushed the harden/gmlreader-xxe branch from 8d14e99 to c8cb498 Compare September 3, 2026 17:47

Uh oh!

Conversation

Nexory commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Test

Uh oh!

ppkarwasz Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nexory commented Aug 24, 2026

Uh oh!

Uh oh!

Nexory commented Aug 24, 2026

Uh oh!

jodygarnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Nexory commented Aug 26, 2026

Uh oh!

grootstebozewolf left a comment

Choose a reason for hiding this comment

Uh oh!

grootstebozewolf Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

grootstebozewolf Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

grootstebozewolf Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

grootstebozewolf Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

ppkarwasz commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nexory commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Nexory commented Aug 6, 2026 •

edited

Loading

ppkarwasz Aug 24, 2026 •

edited

Loading

ppkarwasz commented Sep 3, 2026 •

edited

Loading