GMLReader and KMLReader XML parser configuration by jodygarnett · Pull Request #1244 · locationtech/jts

jodygarnett · 2026-10-13T18:34:57Z

GML and KML do not require DOCTYPE. Apply recommended parser settings to both readers, logging any the parser does not support, so the configuration does not depend on a specific parser.

Follow up to #1204, alternative to #1221 and #1230 with no additional dependencies.

Thanks to @Nexory and @ppkarwasz also!

Nexory · 2026-10-13T18:46:20Z

Thanks, this works for me. The one thing #1244 does not have yet is a test.
The two from #1221, testExternalEntityIsNotResolved and testDoctypeIsRejected in GMLReaderTest, pass on #1244 unchanged and fail on master, so feel free to copy them over. I'll close #1221 once this is merged.

jodygarnett · 2026-10-13T18:47:53Z

 */
 public class GMLReader 
 {
+	private static final Logger LOGGER = Logger.getLogger(GMLReader.class.getName());


@dr-jts I am using standard java logging here, not sure if JTS has a "policy" about logging?

JTS so far hasn't done any logging internally. I think it's better to continue with the policy of no logging (since that introduces a new mode that is potentially externally visible).

Can't the exceptions just be passed up for clients to handle?

Yes they can, the wisdom of providing a detailed helpful exception, vs providing a helpful log and generic exception, is one of these annoying tradeoffs.

However in this case I am logging a warning saying "hey your parser does not provde this setting" so outright failure is a bit mean spirited. Different enviornments can be configured with different xml parsers.

Would you prefer something to System.err or System.out, or silient ignore.

just adding a comment for now, no output

jodygarnett · 2026-10-14T04:55:18Z

Thanks, this works for me. The one thing #1244 does not have yet is a test. The two from #1221, testExternalEntityIsNotResolved and testDoctypeIsRejected in GMLReaderTest, pass on #1244 unchanged and fail on master, so feel free to copy them over. I'll close #1221 once this is merged.

I have adapted those as examples.

ppkarwasz · 2026-10-20T12:22:22Z

+		// GML does not require DOCTYPE
+		setFeature(fact, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+		setFeature(fact, "http://apache.org/xml/features/disallow-doctype-decl", true);
+		setFeature(fact, "http://xml.org/sax/features/external-general-entities", false);
+		setFeature(fact, "http://xml.org/sax/features/external-parameter-entities", false);
+		setFeature(fact, "http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+		try {
+			fact.setXIncludeAware(false);
+		} catch (UnsupportedOperationException e) {
+			// not supported by this parser
+		}

 		SAXParser parser = fact.newSAXParser();
+		try {
+			parser.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+			parser.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
+		} catch (SAXNotRecognizedException | SAXNotSupportedException e) {
+			// not supported by this parser
+		}


Except FEATURE_SECURE_PROCESSING, these setting are not really necessary, because the resolver on line 136 takes care of external resources.

Setting FEATURE_SECURE_PROCESSING should probably fail closed instead of fail open: theoretically this property must be supported by all JAXP implementations, although Android doesn't support it.

That is fair feedback about failing closed, still if we have all the subsequent other safety measures it should be fine? Do you think ...

In practice everything will be fine, my remark is mostly a question of style: there are only so many maintained SAX implementations left in 2026 (I am aware of Xerces, its JDK fork, Android's ExpatReader and Oracle's XDK).

You can achieve the same result with far less lines of code:

FEATURE_SECURE_PROCESSING guarantees that XML will be processed in a secure way. It's supported by everything, except Android. This feature in practice prevents entity expansion attacks, so disabling DOCTYPE is no longer necessary.

Implementing EntityResolver on line 134 takes care of the rest:

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

Lines 134 to 139 in 8b5e408

GMLHandler gh = new GMLHandler(geometryFactory,null) {

@Override

public InputSource resolveEntity(String publicId, String systemId) throws SAXException {

throw new SAXException("External entity resolution is not allowed: " + systemId);

}

};

If a file has a DOCTYPE with external references, these lines will block it. If the DOCTYPE only contains internal entities, FSP will prevent Billion Laughs style attacks.

Keeping the rest is not wrong, it is just not necessary and introduces differences of behavior between SAX implementations. For example disable-doctype-decl is only recognized by parsers from the Xerces lineage. XML files that contain a DOCTYPE with only internal entity definitions will work on some parsers and fail on others.

ppkarwasz · 2026-10-20T12:28:12Z

+        setProperty(XMLInputFactory.SUPPORT_DTD, false);
+        setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
+        setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+        setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");


The ACCESS_EXTERNAL_* properties are not necessary, since SUPPORT_DTD and IS_SUPPORTING_EXTERNAL_ENTITIES are set to false.

The SUPPORT_DTD and IS_SUPPORTING_EXTERNAL_ENTITIES properties are recognized by all implementations, so similarly to FSP above, they could fail closed.

After being burned so many times I would prefer to have a belt and suspenders (even if it is just to keep security scans from complaining).

So even if not necessary, it will reasure code reviewers to see it there.

Reassuring code reviewers (and silencing reporters) is the reason we created Commons Secure XML. 😉 This is especially useful in cases when you don't want to waste time to configure an XML parser securely (e.g. you are parsing a trusted configuration file), but reporters keep reporting a potential XXE injection.

Anyway, you do have a belt and suspenders a couple of lines below:

jts/modules/core/src/main/java/org/locationtech/jts/io/kml/KMLReader.java

Lines 106 to 108 in 8b5e408

inputFactory.setXMLResolver((publicID, systemID, baseURI, namespace) -> {

throw new XMLStreamException("External entity resolution is not allowed: " + systemID);

});

Nice I was not aware of https://commons.apache.org/proper/commons-secure-xml/ - while I do not wish to add a dependency to such a simple library as JTS I am going to recommend it for other projects.

We did anticipate that some library do not want additional dependencies, which is why we “modularised” Commons Secure XML as much as possible. If shaded its footprint is quite small: JTS with Commons Secure XML shaded adds around 16 KiB to the library, compared to the 2 KiB of the code in this PR.

GML and KML do not require DOCTYPE. Apply recommended parser settings to both readers.

jodygarnett · 2026-10-23T17:37:48Z

Okay updated and squashed down to a single commit, that does not provide a lot of detail (due to security related topic).

jodygarnett changed the title ~~Harden GMLReader and KMLReader XML parser configuration~~ GMLReader and KMLReader XML parser configuration Oct 13, 2026

This was referenced Oct 13, 2026

Harden GMLReader against XXE (disable DTDs and external entities) #1221

Closed

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

Closed

jodygarnett force-pushed the gml-reader branch from f772414 to a3f4169 Compare October 13, 2026 18:44

jodygarnett commented Oct 13, 2026

View reviewed changes

ppkarwasz suggested changes Oct 20, 2026

View reviewed changes

GMLReader and KMLReader XML parser configuration

d6502cd

GML and KML do not require DOCTYPE. Apply recommended parser settings to both readers.

jodygarnett force-pushed the gml-reader branch from 8b5e408 to d6502cd Compare October 23, 2026 17:35

jodygarnett merged commit 6a6837e into locationtech:master Oct 23, 2026
2 checks passed

jodygarnett deleted the gml-reader branch October 23, 2026 17:41

	GMLHandler gh = new GMLHandler(geometryFactory,null) {
	@Override
	public InputSource resolveEntity(String publicId, String systemId) throws SAXException {
	throw new SAXException("External entity resolution is not allowed: " + systemId);
	}
	};

	inputFactory.setXMLResolver((publicID, systemID, baseURI, namespace) -> {
	throw new XMLStreamException("External entity resolution is not allowed: " + systemID);
	});

Conversation

jodygarnett commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodygarnett Oct 14, 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!

Choose a reason for hiding this comment

Uh oh!

jodygarnett commented Oct 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodygarnett Oct 23, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodygarnett commented Oct 23, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jodygarnett commented Oct 13, 2026 •

edited

Loading

Nexory commented Oct 13, 2026 •

edited

Loading

jodygarnett Oct 14, 2026 •

edited

Loading

jodygarnett Oct 23, 2026 •

edited

Loading