Skip to content

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

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

Open
Nexory wants to merge 3 commits into
base: master
Choose a base branch
from
+50 −0
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8d0bc1
Harden GMLReader against XXE (disable DTDs and external entities)
Nexory Aug 6, 2026
072304e
Reject the DOCTYPE, and let an unconfigurable parser fail
Nexory Aug 24, 2026
c8cb498
Address review comments
Nexory Aug 26, 2026
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
15 changes: 15 additions & 0 deletions modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java
View file
Open in desktop

@grootstebozewolf grootstebozewolf Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
import java.io.Reader;
import java.io.StringReader;

import java.util.logging.Level;
import java.util.logging.Logger;

import javax.xml.XMLConstants;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.parsers.SAXParser;
import javax.xml.parsers.SAXParserFactory;
Expand All @@ -25,6 +29,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,6 +111,15 @@ public Geometry read(Reader reader, GeometryFactory geometryFactory) throws SAXE

fact.setNamespaceAware(false);
fact.setValidating(false);
// Harden against XXE, as GML is often read from untrusted sources.
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);
}
Comment on lines +115 to +122

@grootstebozewolf grootstebozewolf Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


SAXParser parser = fact.newSAXParser();

Expand Down
35 changes: 35 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,37 @@ private void checkRead(String gml, String wktExpected, int srid) {
checkEqual(expected, g);
assertEquals("SRID incorrect - ", srid, g.getSRID());
}

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();
}
}
Comment on lines +132 to +152

@grootstebozewolf grootstebozewolf Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


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) {
}
}
Comment on lines +155 to +163

@grootstebozewolf grootstebozewolf Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

}