Skip to content

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

Draft
ppkarwasz wants to merge 1 commit into
locationtech:masterfrom
ppkarwasz:feat/use-commons-xml
Draft

Harden GMLReader against XXE (use Commons Secure XML)#1230
ppkarwasz wants to merge 1 commit into
locationtech:masterfrom
ppkarwasz:feat/use-commons-xml

Conversation

@ppkarwasz

Copy link
Copy Markdown

Note

Draft until Apache Commons Secure XML 1.0.0 reaches Maven Central. The release is currently under vote, so the POM temporarily adds the Apache staging repository (after Central). That block will be removed from this PR once the release is published.

Alternative to #1221, same problem, different fix.

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, so a crafted document can disclose local files or trigger SSRF through an external entity (XXE):

<!DOCTYPEfoo [ <!ENTITYxxe SYSTEM "file:///etc/hostname"> ]>
<gml:Point><gml:coordinates>&xxe;</gml:coordinates></gml:Point>

KMLReader received the equivalent hardening in #1204.

Fix

Create the factory through SecureSAXParserFactory.newInstance() from Apache Commons Secure XML instead of SAXParserFactory.newInstance(). Every factory the library returns makes the same three guarantees regardless of which JAXP implementation is on the classpath:

  • external DTDs are not fetched,
  • external entities are not resolved,
  • internal entity expansion is bounded (secure processing plus the implementation's limits, so Billion Laughs is rejected).

Compared with #1221, which sets Xerces-specific feature names by hand and logs a warning when the parser does not recognize them, the library either secures the factory or fails closed; there is no silent fallback to an unhardened parser. The reader keeps setNamespaceAware(false) / setValidating(false) (it deliberately accepts unbound gml: prefixes), keeps its signatures, and parses valid GML exactly as before.

The class Javadoc now says where the parser comes from and links to the library's package documentation and threat model, so security scanners have something to point at instead of flagging the reader.

Points for the reviewers

  • First runtime dependency of jts-core.org.apache.commons:commons-secure-xml is a ~75 KB OSGi bundle with no transitive dependencies, compatible with Java 8; maven-bundle-plugin imports it as org.apache.commons.xml.secure;version="[1.0,2)" without any manifest change. If an external dependency is not acceptable, the library is designed to be shaded: with minimizeJar, the SAX entry point adds about 10 kB to jts-core, and JTS still picks up fixes by bumping the version.
  • No new tests, on purpose (see Verification): they would only re-test another library's contract.
  • Javadoc autolinking. The PR adds a maven-javadoc-plugin<links> entry to https://javadoc.io (unresolvable until 1.0.0 is published) so that {@link SecureSAXParserFactory} in the GMLReader Javadoc becomes a hyperlink, and automated tools following it land on the documented guarantees rather than reporting a false positive. I can switch it to the Javadoc on https://commons.apache.org, which always shows the latest version (a plus or a minus depending on the shading decision above), or drop the autolinking entirely.

Verification

As noted above, there are no new XXE unit tests: the guarantees belong to the library and are tested there. Instead, a forbidden-apis check now fails the jts-core build whenever a raw SAX factory method is used (SAXParserFactory.newInstance/newDefaultInstance/newNSInstance overloads and XMLReaderFactory.createXMLReader). Running it against the unpatched reader:

[ERROR] Forbidden method invocation: javax.xml.parsers.SAXParserFactory#newInstance() [Create SAX parsers through org.apache.commons.xml.secure.SecureSAXParserFactory, which never resolves external DTDs or entities]
[ERROR] in org.locationtech.jts.io.gml2.GMLReader (GMLReader.java:111)
[ERROR] Scanned 758 class file(s) for forbidden API invocations (in 0.12s), 1 error(s).

mvn clean install on Temurin 8 passes (jts-core: 2296 tests, 0 failures).

🤖 Generated with Claude Code

GMLReader created its SAX parser through a stock SAXParserFactory with
only namespace awareness and validation switched off, so a document
could declare external entities and have the parser fetch them.
Create the factory through Apache Commons Secure XML instead, which
guarantees on every JAXP implementation that external DTDs and
entities are never resolved and that entity expansion is bounded.
Enforce the choice with a forbidden-apis check that rejects the raw
SAX factory methods in `jts-core`, and link the Javadoc to the
library's documentation.
Commons Secure XML 1.0.0 is under vote: the POM temporarily adds the
Apache staging repository after Central; remove it once the release
reaches Maven Central.
Assisted-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Piotr P. Karwasz <piotr@eclipse.copernik.eu>
@ppkarwasz

Copy link
Copy Markdown
Author

One more design choice worth considering:

Commons Secure XML also backports newDefaultInstance() from JDK 9, so GMLReader could create its parser with SecureSAXParserFactory.newDefaultInstance() instead of newInstance(). That pins the reader to the JDK's built-in parser and opts out of JAXP pluggability, so a stray Xerces on the classpath can no longer change how JTS parses GML. Users who need a different parser already have that option: GMLHandler lets them drive any SAX parser of their own.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@ppkarwasz