Skip to content

ST_GeomFromGML should not resolve external XML entities#5100

Open
Samin061 wants to merge 2 commits into
apache:mainfrom
Samin061:gml-xxe
Open

ST_GeomFromGML should not resolve external XML entities#5100
Samin061 wants to merge 2 commits into
apache:mainfrom
Samin061:gml-xxe

Conversation

@Samin061

Copy link
Copy Markdown
Contributor

Jira Link

Not filed yet; glad to open a CALCITE issue for tracking if you prefer one.

Changes Proposed

ST_GeomFromGML hands the GML text straight to SpatialTypeUtils.fromGml, which parses it with JTS GMLReader. That reader builds a SAX parser from the JAXP defaults, so DOCTYPE declarations and external entities are resolved. The GML argument is ordinary untrusted data (a literal, parameter, or row value), so a document like <!DOCTYPE p [ <!ENTITY x SYSTEM "file:///etc/passwd"> ]> referenced from <gml:coordinates> reads a local file, and an external-DTD reference makes the parser open an outbound connection, giving both file disclosure and SSRF (XXE). The XML functions in XmlFunctions were already hardened against this, but the spatial path slipped through because JTS constructs its own parser instead of reusing that configuration. I noticed it while checking which readers under runtime touch untrusted XML. The fix builds the SAX parser inside fromGml with FEATURE_SECURE_PROCESSING and disallow-doctype-decl and drives JTS GMLHandler directly, matching the existing hardening; valid GML parses exactly as before, and the added test confirms a document carrying a DOCTYPE is rejected.

Repro (with fun=spatial):

SELECT ST_GeomFromGML(
  '<?xml version="1.0"?>
   <!DOCTYPE Point [ <!ENTITY xxe SYSTEM "file:///etc/passwd"> ]>
   <gml:Point xmlns:gml="http://www.opengis.net/gml"><gml:coordinates>&xxe;</gml:coordinates></gml:Point>');

./gradlew :core:test (SpatialTypeUtilsTest, CoreQuidemTest), checkstyle, autostyle and forbiddenApis pass locally.

@xuzifu666

Copy link
Copy Markdown
Member

Is there a corresponding Jira ticket for this? Also, please address the CI issues.

Signed-off-by: bibi samina <sam@bugqore.com>
@Samin061

Copy link
Copy Markdown
Contributor Author

No Jira ticket yet. Happy to open a CALCITE issue if you'd like one for tracking.

The failing check was CheckerFramework: it flagged the null delegate I passed to GMLHandler. I swapped it for a DefaultHandler, which keeps the same behavior (its fatalError still rethrows, matching the null fallback) but isn't null. The -PenableCheckerframework :core:compileJava build is green locally now.

@rubenada

Copy link
Copy Markdown
Contributor

Looks like a duplicate of #5006 ?

@sonarqubecloud

Copy link
Copy Markdown

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants