[alhudz]

Jira Link

CALCITE-7601

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.

[ppkarwasz]

There is a proposed patch upstream locationtech/jts#1221, so I second the decision of closing this PR.

[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.

[alhudz]

@ppkarwasz on the Checker Framework question: it doesn't come from any annotation in JTS. The nullness checker defaults every unannotated parameter to @NonNull, library bytecode included, and there's no stub for JTS under src/main/config/checkerframework, so a literal null argument gets flagged:

SpatialTypeUtils.java:154: error: [argument.type.incompatible] incompatible argument for parameter delegate of GMLHandler.
  found   : null (NullType)
  required: @Initialized @NonNull ErrorHandler

On the PMC's position: agreed, the parser is built inside GMLReader, so JTS is the right place for the fix. Fine to close this one in favour of locationtech/jts#1221.

[ppkarwasz]

In #5302 I created an alternative PR that uses commons-secure-xml to apply the hardening. The PR is just a fallback in case no hardening is applied in JTS and Calcite needs to do a release.

I think both PRs can be ignored for now, but should be evaluated again shortly before a new release to see what is the current situation.