[CALCITE-7601] Harden ST_GeomFromGML against external entity expansion

[alhudz]

Jira Link

A Jira can be filed for this if preferred; raising the patch first since the change is small and self-contained.

Changes Proposed

Repro: SELECT ST_GeomFromGML(g) where g is a GML string carrying a DOCTYPE with an external entity, e.g. <!DOCTYPE x [ <!ENTITY e SYSTEM "file:///etc/passwd"> ]> referenced from <gml:coordinates>&e;,0</gml:coordinates>.

Expected: the entity is not resolved.

Actual: fromGml builds a JTS GMLReader, whose internal SAXParserFactory leaves DOCTYPE and external general/parameter entities enabled, so the parser fetches the entity target and inlines it into the geometry. That is local file read / SSRF (XXE) from row data, since the GML argument crosses the trust boundary at the ST_GeomFromGML SQL function.

Fix: parse with a SAXParser configured with disallow-doctype-decl and external entities off, feeding JTS's own GMLHandler. Same hardening already used in XmlFunctions and DiffRepository.

Test: SpatialTypeUtilsTest gets a regression that points an external entity at a temp file holding a valid coordinate, so an unguarded parser would return POINT (7 8) while the guarded one rejects the document.

[ppkarwasz]

Setting all three features is overkill, since disallow-doctype-decl will throw on a DOCTYPE declaration and will therefore not parse any external subset (non-validating/load-external-dtd) nor entities.

[alhudz]

Good catch, dropped both external-entity lines. disallow-doctype-decl throws on the DOCTYPE so nothing reaches the entity-resolution path anyway. Test still rejects the payload.

[xuzifu666]

Thank you for your contribution @alhudz . Please refer to other PRs. We need Jira to illustrate the issue, and we strictly require that the submitted information be consistent with Jira's.

[ppkarwasz]

Disclaimer: some shameless self-promotion 😉

Since changes like this one have been landing in many OSS projects lately, a month ago I wrote a small reusable library that hands you a hardened JAXP factory with a single method call:

https://javadoc.io/static/eu.copernik/copernik-xml-factory/0.1.0/eu/copernik/xml/factory/XmlFactories.html

The library code itself wouldn't add much to a PR like this. What might be worth it is everything around the code: a continuously evolving test suite that runs against every JAXP implementation still in the wild, and the fact that a single dependency can act as a "scapegoat" for SAST tools and reviewers. Instead of every project re-deriving the full set of (often cargo-culted) properties and tripping XXE/SSRF warnings whenever one is missing, the hardening lives in one audited place.

Note

The library is incubating, with the hope of landing in either Apache Commons or Xerces. I haven't had definitive feedback from either project yet, so I'll keep maintaining it for the foreseeable future.

[alhudz]

I have created CALCITE-7601 to track this issue and have updated the PR information to reference the Jira so that the descriptions remain consistent.

[alhudz]

Pushed a fix for the autostyleJavaCheck failure (just reflowed the test, ran autostyleApply). The hardening and the regression test are unchanged.

[alhudz]

Pushed a fix for the failing spatial.iq tests. The hardened parser had namespaceAware on, which rejected the existing gml:-prefixed samples that never declare the namespace; JTS's GMLReader parses non-namespace-aware, so I matched that. disallow-doctype-decl still blocks the XXE payload, and the quidem cases plus the regression test pass locally.

[mihaibudiu]

This is better, but the checker framework is still unhappy

[alhudz]

That was the null delegate passed to GMLHandler. JTS's own GMLReader passes null there as well, but the parameter isn't annotated so the nullness checker flags it. Suppressed argument.type.incompatible on that line to match the existing pattern in the codebase. Runtime behaviour is unchanged and the regression test still rejects the XXE payload.

[ppkarwasz]

I am curious: where does CheckerFramework take the information those arguments should not be null? I don't see any annotations in GMLReader and adjacent classes.

[rubenada]

Please be aware that there's been some discussion within Calcite PMC to decide whether or not we should accept this patch; and it seems that, for the moment, the PMC leans towards not accepting it (and consider that this falls under JTS responsibility).
So, please do not merge this PR for the time being.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.