From f8d0bc1d78a7152a7abb7c5ace862e92a6f72014 Mon Sep 17 00:00:00 2001 From: Nexory Date: Thu, 6 Aug 2026 15:46:04 +0200 Subject: [PATCH 1/3] Harden GMLReader against XXE (disable DTDs and external entities) 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 #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 --- .../locationtech/jts/io/gml2/GMLReader.java | 9 ++ .../jts/io/gml2/GMLReaderXXETest.java | 82 +++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java diff --git a/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java b/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java index 7bef25880f..0fee770eb7 100644 --- a/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java +++ b/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java @@ -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; @@ -105,6 +106,14 @@ public Geometry read(Reader reader, GeometryFactory geometryFactory) throws SAXE fact.setNamespaceAware(false); fact.setValidating(false); + // Harden against XXE: disable DOCTYPE/DTDs and external entities (JAXP secure processing). + // GML input is frequently untrusted (files, WFS responses, uploads); the default SAX parser + // resolves external entities, enabling file disclosure and SSRF. + fact.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + fact.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + 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); SAXParser parser = fact.newSAXParser(); diff --git a/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java b/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java new file mode 100644 index 0000000000..a8481c5e70 --- /dev/null +++ b/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java @@ -0,0 +1,82 @@ +package org.locationtech.jts.io.gml2; + +import java.io.File; +import java.nio.file.Files; + +import org.locationtech.jts.geom.Geometry; + +import junit.textui.TestRunner; +import test.jts.GeometryTestCase; + +/** + * Tests that {@link GMLReader} does not resolve external XML entities (XXE). + * GML is frequently read from untrusted sources (files, WFS responses, uploads), + * so the underlying SAX parser must not fetch external entities or process DTDs. + */ +public class GMLReaderXXETest extends GeometryTestCase { + + public static void main(String[] args) { + TestRunner.run(GMLReaderXXETest.class); + } + + public GMLReaderXXETest(String name) { + super(name); + } + + /** + * An external general entity referencing a local file must not be resolved. + * Without the hardening the file content leaks into the parse (and, here, into + * the exception raised while parsing it as a coordinate); with it, the DOCTYPE + * is rejected before any entity is resolved. + */ + 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 = + "\n" + + " ]>\n" + + "&xxe;"; + try { + new GMLReader().read(gml, null); + } + catch (Exception e) { + // The parser may legitimately reject the input; it must never expose the + // external file's content (which would prove the entity was resolved). + assertFalse("GMLReader resolved an external entity (XXE): " + e.getMessage(), + String.valueOf(e.getMessage()).contains(secret)); + } + } + finally { + secretFile.delete(); + } + } + + /** + * A DOCTYPE declaration must be rejected outright (billion-laughs / DTD surface). + */ + public void testDoctypeIsRejected() throws Exception { + String gml = + "\n" + + "\n" + + "5,10"; + try { + new GMLReader().read(gml, null); + fail("expected a DOCTYPE to be rejected"); + } + catch (Exception e) { + // expected: parser refuses the DOCTYPE + } + } + + /** + * Legitimate GML without a DOCTYPE must still parse unchanged. + */ + public void testBenignGmlStillParses() throws Exception { + Geometry g = new GMLReader().read( + "5,10", null); + assertNotNull(g); + assertEquals("POINT (5 10)", g.toText()); + } +} From 072304ecd4eb78cc1207983d571e8bcdd44f52c9 Mon Sep 17 00:00:00 2001 From: Nexory Date: Mon, 24 Aug 2026 19:46:11 +0200 Subject: [PATCH 2/3] Reject the DOCTYPE, and let an unconfigurable parser fail 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 --- .../locationtech/jts/io/gml2/GMLReader.java | 21 +++++++++------- .../jts/io/gml2/GMLReaderXXETest.java | 24 +++++++++++++++---- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java b/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java index 0fee770eb7..3442ae3fa7 100644 --- a/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java +++ b/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java @@ -106,14 +106,19 @@ public Geometry read(Reader reader, GeometryFactory geometryFactory) throws SAXE fact.setNamespaceAware(false); fact.setValidating(false); - // Harden against XXE: disable DOCTYPE/DTDs and external entities (JAXP secure processing). - // GML input is frequently untrusted (files, WFS responses, uploads); the default SAX parser - // resolves external entities, enabling file disclosure and SSRF. - fact.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - fact.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - 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); + // Harden against XXE. GML input is frequently untrusted (files, WFS responses, + // uploads) and the default SAX parser resolves external entities, which allows + // file disclosure and SSRF. Rejecting the DOCTYPE covers it: without one there is + // no internal or external subset, so no entity can be declared in the first place. + // + // Android's parser rejects every feature name outside the SAX namespace and does + // not resolve external references in the first place, so it is skipped. Anywhere + // else the features are set without a guard: a parser that cannot be configured + // should fail here rather than parse untrusted input without the hardening. + if (!"org.apache.harmony.xml.parsers.SAXParserFactoryImpl".equals(fact.getClass().getName())) { + fact.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + fact.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + } SAXParser parser = fact.newSAXParser(); diff --git a/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java b/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java index a8481c5e70..ea01d1d554 100644 --- a/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java +++ b/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java @@ -1,3 +1,14 @@ +/* + * Copyright (c) 2026 Vivid Solutions. + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v2.0 + * and Eclipse Distribution License v. 1.0 which accompanies this distribution. + * The Eclipse Public License is available at http://www.eclipse.org/legal/epl-v20.html + * and the Eclipse Distribution License is available at + * + * http://www.eclipse.org/org/documents/edl-v10.php. + */ package org.locationtech.jts.io.gml2; import java.io.File; @@ -38,15 +49,18 @@ public void testExternalEntityIsNotResolved() throws Exception { "\n" + " ]>\n" + "&xxe;"; + String observed; try { - new GMLReader().read(gml, null); + observed = String.valueOf(new GMLReader().read(gml, null)); } catch (Exception e) { - // The parser may legitimately reject the input; it must never expose the - // external file's content (which would prove the entity was resolved). - assertFalse("GMLReader resolved an external entity (XXE): " + e.getMessage(), - String.valueOf(e.getMessage()).contains(secret)); + // The parser may legitimately reject the input. Either way the external + // file's content must not appear, neither in the parsed geometry nor in + // the message of the exception raised while parsing it. + observed = String.valueOf(e.getMessage()); } + assertFalse("GMLReader resolved an external entity (XXE): " + observed, + observed.contains(secret)); } finally { secretFile.delete(); From c8cb498bf27dcd4c117ee8bbff11baaef5bba4af Mon Sep 17 00:00:00 2001 From: Nexory Date: Wed, 26 Aug 2026 20:16:05 +0200 Subject: [PATCH 3/3] Address review comments - 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 --- .../locationtech/jts/io/gml2/GMLReader.java | 21 ++-- .../jts/io/gml2/GMLReaderTest.java | 35 +++++++ .../jts/io/gml2/GMLReaderXXETest.java | 96 ------------------- 3 files changed, 46 insertions(+), 106 deletions(-) delete mode 100644 modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java diff --git a/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java b/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java index 3442ae3fa7..6f43e1198b 100644 --- a/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java +++ b/modules/core/src/main/java/org/locationtech/jts/io/gml2/GMLReader.java @@ -15,6 +15,9 @@ 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; @@ -26,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; @@ -106,19 +111,15 @@ public Geometry read(Reader reader, GeometryFactory geometryFactory) throws SAXE fact.setNamespaceAware(false); fact.setValidating(false); - // Harden against XXE. GML input is frequently untrusted (files, WFS responses, - // uploads) and the default SAX parser resolves external entities, which allows - // file disclosure and SSRF. Rejecting the DOCTYPE covers it: without one there is - // no internal or external subset, so no entity can be declared in the first place. - // - // Android's parser rejects every feature name outside the SAX namespace and does - // not resolve external references in the first place, so it is skipped. Anywhere - // else the features are set without a guard: a parser that cannot be configured - // should fail here rather than parse untrusted input without the hardening. - if (!"org.apache.harmony.xml.parsers.SAXParserFactoryImpl".equals(fact.getClass().getName())) { + // 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); + } SAXParser parser = fact.newSAXParser(); diff --git a/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderTest.java b/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderTest.java index debbb660da..96ddaddf4c 100644 --- a/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderTest.java +++ b/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderTest.java @@ -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; @@ -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 = "\n" + + " ]>\n" + + "&xxe;"; + 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(); + } + } + + public void testDoctypeIsRejected() throws Exception { + String gml = "\n\n" + + "5,10"; + try { + new GMLReader().read(gml, null); + fail("expected a DOCTYPE to be rejected"); + } + catch (Exception e) { + } + } } diff --git a/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java b/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java deleted file mode 100644 index ea01d1d554..0000000000 --- a/modules/core/src/test/java/org/locationtech/jts/io/gml2/GMLReaderXXETest.java +++ /dev/null @@ -1,96 +0,0 @@ -/* - * Copyright (c) 2026 Vivid Solutions. - * - * All rights reserved. This program and the accompanying materials - * are made available under the terms of the Eclipse Public License v2.0 - * and Eclipse Distribution License v. 1.0 which accompanies this distribution. - * The Eclipse Public License is available at http://www.eclipse.org/legal/epl-v20.html - * and the Eclipse Distribution License is available at - * - * http://www.eclipse.org/org/documents/edl-v10.php. - */ -package org.locationtech.jts.io.gml2; - -import java.io.File; -import java.nio.file.Files; - -import org.locationtech.jts.geom.Geometry; - -import junit.textui.TestRunner; -import test.jts.GeometryTestCase; - -/** - * Tests that {@link GMLReader} does not resolve external XML entities (XXE). - * GML is frequently read from untrusted sources (files, WFS responses, uploads), - * so the underlying SAX parser must not fetch external entities or process DTDs. - */ -public class GMLReaderXXETest extends GeometryTestCase { - - public static void main(String[] args) { - TestRunner.run(GMLReaderXXETest.class); - } - - public GMLReaderXXETest(String name) { - super(name); - } - - /** - * An external general entity referencing a local file must not be resolved. - * Without the hardening the file content leaks into the parse (and, here, into - * the exception raised while parsing it as a coordinate); with it, the DOCTYPE - * is rejected before any entity is resolved. - */ - 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 = - "\n" - + " ]>\n" - + "&xxe;"; - String observed; - try { - observed = String.valueOf(new GMLReader().read(gml, null)); - } - catch (Exception e) { - // The parser may legitimately reject the input. Either way the external - // file's content must not appear, neither in the parsed geometry nor in - // the message of the exception raised while parsing it. - observed = String.valueOf(e.getMessage()); - } - assertFalse("GMLReader resolved an external entity (XXE): " + observed, - observed.contains(secret)); - } - finally { - secretFile.delete(); - } - } - - /** - * A DOCTYPE declaration must be rejected outright (billion-laughs / DTD surface). - */ - public void testDoctypeIsRejected() throws Exception { - String gml = - "\n" - + "\n" - + "5,10"; - try { - new GMLReader().read(gml, null); - fail("expected a DOCTYPE to be rejected"); - } - catch (Exception e) { - // expected: parser refuses the DOCTYPE - } - } - - /** - * Legitimate GML without a DOCTYPE must still parse unchanged. - */ - public void testBenignGmlStillParses() throws Exception { - Geometry g = new GMLReader().read( - "5,10", null); - assertNotNull(g); - assertEquals("POINT (5 10)", g.toText()); - } -}