Skip to content

GMLReader and KMLReader XML parser configuration #1244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jodygarnett merged 1 commit into from
Oct 23, 2026
Merged

GMLReader and KMLReader XML parser configuration #1244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.io.Reader;
import java.io.StringReader;

import javax.xml.XMLConstants;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.parsers.SAXParser;
import javax.xml.parsers.SAXParserFactory;
Expand All @@ -25,6 +26,8 @@
import org.locationtech.jts.geom.PrecisionModel;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;
import org.xml.sax.SAXNotRecognizedException;
import org.xml.sax.SAXNotSupportedException;
import org.xml.sax.helpers.DefaultHandler;


Expand Down Expand Up @@ -105,16 +108,51 @@ public Geometry read(Reader reader, GeometryFactory geometryFactory) throws SAXE

fact.setNamespaceAware(false);
fact.setValidating(false);
// GML does not require DOCTYPE
boolean secureProcessing = setFeature(fact, XMLConstants.FEATURE_SECURE_PROCESSING, true);
boolean disallowDocType = setFeature(fact, "http://apache.org/xml/features/disallow-doctype-decl", true);
if (!secureProcessing && !disallowDocType) {
throw new ParserConfigurationException("Parser does not support DOCTYPE restrictions");
}
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
}
Comment on lines +111 to +132

@ppkarwasz ppkarwasz Oct 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jodygarnett jodygarnett Oct 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ppkarwasz ppkarwasz Oct 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. 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.


if(geometryFactory == null)
geometryFactory = new GeometryFactory();

GMLHandler gh = new GMLHandler(geometryFactory,null);
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);
}
};
parser.parse(new InputSource(reader), (DefaultHandler)gh);

return gh.getGeometry();
}

private static boolean setFeature(SAXParserFactory fact, String feature, boolean value) {
try {
fact.setFeature(feature, value);
return true;
} catch (ParserConfigurationException | SAXNotRecognizedException | SAXNotSupportedException e) {
// not supported by this parser
return false;
}
}

}
30 changes: 26 additions & 4 deletions modules/core/src/main/java/org/locationtech/jts/io/kml/KMLReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.locationtech.jts.geom.Polygon;
import org.locationtech.jts.io.ParseException;

import javax.xml.XMLConstants;
import javax.xml.stream.XMLInputFactory;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamReader;
Expand Down Expand Up @@ -57,6 +58,8 @@ public class KMLReader {
private static final String OUTER_BOUNDARY_IS = "outerBoundaryIs";
private static final String INNER_BOUNDARY_IS = "innerBoundaryIs";

private static final String ENTITY_EXPANSION_LIMIT = "jdk.xml.entityExpansionLimit";

private static final String NO_ELEMENT_ERROR = "No element %s found in %s";

/**
Expand Down Expand Up @@ -97,10 +100,29 @@ public KMLReader(GeometryFactory geometryFactory, Collection<String> attributeNa
this.attributeNames = attributeNames == null
? Collections.emptySet()
: new HashSet<>(attributeNames);
// 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);
// KML does not require DOCTYPE
boolean disallowDtd = setProperty(XMLInputFactory.SUPPORT_DTD, false);
if (!disallowDtd) {
throw new IllegalStateException("Parser does not support DOCTYPE restrictions");
}
setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
if (inputFactory.isPropertySupported(ENTITY_EXPANSION_LIMIT)
&& "0".equals(String.valueOf(inputFactory.getProperty(ENTITY_EXPANSION_LIMIT)))) {
inputFactory.setProperty(ENTITY_EXPANSION_LIMIT, "64000");
}
inputFactory.setXMLResolver((publicID, systemID, baseURI, namespace) -> {
throw new XMLStreamException("External entity resolution is not allowed: " + systemID);
});
}

private boolean setProperty(String name, Object value) {
if (inputFactory.isPropertySupported(name)) {
inputFactory.setProperty(name, value);
return true;
}
return false;
}

/**
Expand Down
31 changes: 31 additions & 0 deletions modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.locationtech.jts.io.gml2;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;

import javax.xml.parsers.ParserConfigurationException;

Expand Down Expand Up @@ -126,4 +128,33 @@ private void checkRead(String gml, String wktExpected, int srid) {
checkEqual(expected, g);
assertEquals("SRID incorrect - ", srid, g.getSRID());
}

public void testExternalCoordinatesNotIncluded() throws Exception {
File coordinates = File.createTempFile("coordinates", ".txt");
try {
Files.write(coordinates.toPath(), "5,10".getBytes("UTF-8"));
String gml = "<!DOCTYPE Point [ <!ENTITY coordinates SYSTEM \"" + coordinates.toURI() + "\"> ]>"
+ "<gml:Point><gml:coordinates>&coordinates;</gml:coordinates></gml:Point>";
try {
new GMLReader().read(gml, null);
fail("external coordinates should not be included");
}
catch (SAXException expected) {
}
}
finally {
coordinates.delete();
}
}

public void testDoctypeIsRejected() throws Exception {
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) {
}
}
}
5 changes: 5 additions & 0 deletions modules/core/src/test/java/org/locationtech/jts/io/kml/KMLReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,11 @@ public void testCoordinatesErrors() {
checkExceptionThrown("<Polygon><innerBoundaryIs><LinearRing></LinearRing></innerBoundaryIs></Polygon>", "No element coordinates found in innerBoundaryIs");
}

public void testDoctypeIgnored() {
checkParsingResult("<!DOCTYPE Point><Point><coordinates>1.0,1.0</coordinates></Point>",
"POINT (1 1)", new Map[]{null});
}

public void testUnknownGeometryType() {
checkExceptionThrown("<StrangePoint></StrangePoint>", "Unknown KML geometry type StrangePoint");
}
Expand Down
Loading