Repository navigation
BRMO-438: Harden XML parsing and transforming against XXE consistently - #2839
Merged
Merged
Conversation
Add SecureXmlFactories with hardened TransformerFactory and DocumentBuilderFactory factory methods and use these everywhere instead of the newInstance() factory methods. The TransformerFactory has secure processing enabled and does not load external DTDs or stylesheets (stylesheets via a URIResolver are still allowed). This fixes XXE in NhrXMLReader, which parsed the input using a StreamSource directly with an unhardened TransformerFactory; setting an empty system id on the source did not prevent external entity resolution. The DocumentBuilderFactory rejects documents with a DOCTYPE, preventing external entities and entity expansion. This hardens previously unprotected parsing in RsgbTransformer, RsgbBRPTransformer, NhrXMLReader, Brk2Bericht, GbavBericht and StUFBGasynchroon. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add SecureXmlFactories.newXMLInputFactory() with DTD and external entity support disabled and use it in the brmo-loader StAX readers. For the BGT and BAG2 loaders, which configure their own StAX factory and do not depend on brmo-loader, also disable external entities inline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
matthijsln
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2839 +/- ##
========================================
- Coverage 37% 37% -0%
Complexity 74 74
========================================
Files 199 200 +1
Lines 13551 13545 -6
Branches 1431 1431
========================================
- Hits 4909 4904 -5
+ Misses 8236 8233 -3
- Partials 406 408 +2
🚀 New features to boost your workflow:
|
Base the hardened factories on Apache Commons Secure XML, which applies implementation-specific securing for the JDK, Xerces, Woodstox and Saxon. The DocumentBuilderFactory and XMLInputFactory still reject documents with a DOCTYPE. The TransformerFactory now resolves external entities to empty content instead of failing, because the library's resolver takes precedence over the accessExternalDTD/accessExternalStylesheet attributes, so these are removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 of 4 tasks
matthijsln
changed the title
mprins
approved these changes
Oct 23, 2026
mprins
pushed a commit
that referenced
this pull request
Oct 23, 2026
) ## Summary Stacked on #2839 (base branch `harden-xml-parsers-and-transformers`); review only the last two commits. - **Replace Aalto with Woodstox** for the BGT and BAG2 loaders. Aalto was added for performance, which is not critical anymore: XML parsing CPU usage is small compared to network and database IO, and loading is multithreaded. `brmo-service` already excluded Aalto and so already ran these loaders on Woodstox. `woodstox-core` is now managed at 7.3.0, which also upgrades the 6.x version pulled in transitively (CXF, xmlsec). - **New `StaxMateFactories`** (bgt-loader, also used by bag2-loader): creates the StaxMate input factory secured by Apache Commons Secure XML (coalescing, no DTD), and **fails with an `IllegalStateException` if the StAX implementation does not natively implement Stax2**. ### Why the Stax2 check StaxMate works with any StAX parser, but for a non-Stax2 parser (such as the JDK default) it wraps the reader in `Stax2ReaderAdapter`, which counts the element depth itself in `next()` and `getElementText()` but not in `nextTag()`. The GeoTools `XmlStreamGeometryReader` reads geometries directly from `SMInputCursor.getStreamReader()` and uses `nextTag()`. StaxMate re-synchronizes cursors using `getDepth()` of the reader, so after a curved geometry (`gml:Arc`) the depth is off and the cursors **silently skip all following attributes** of the object. Polygons happen to balance out, so only the `bgt_wijk_curve.gml` test caught it when Aalto was removed. StaxMate allows but discourages bypassing cursors (see [FasterXML/StaxMate#2](FasterXML/StaxMate#2), still open). With a native Stax2 parser the depth is tracked by the parser itself, so this works. The javadoc of `StaxMateFactories` documents this, and that the check should stay even when StaxMate is upgraded automatically, unless that issue is verified to be resolved. ## Test plan - [x] `BGTObjectStreamerTest` (including `bgt_wijk_curve.gml`) and BAG2 loader tests pass with Woodstox - [x] New `StaxMateFactoriesTest`: native Stax2 reader with Woodstox; `IllegalStateException` when the JDK StAX implementation is selected - [x] Each commit builds and passes the unit tests on its own; brmo-service, nhr-loader and brmo-commandline (CXF with Woodstox 7.3.0) pass - [ ] Integration tests (database profiles) loading BGT and BAG2 data 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
SecureXmlFactories(brmo-loader) with hardened factory methods, based on Apache Commons Secure XML, and uses them instead ofTransformerFactory.newInstance()/DocumentBuilderFactory.newInstance()everywhere, andXMLInputFactory.newInstance()in brmo-loader.Commons Secure XML secures each factory with a recipe for the actual implementation (JDK, Xerces, Woodstox, Saxon) and resolves external references (entities, DTDs, stylesheets,
document()) to empty content. On top of that:newTransformerFactory():SecureTransformerFactoryplusFEATURE_SECURE_PROCESSING. Stylesheets loaded via aURIResolver(thexsl:imports from the classpath) still work.newDocumentBuilderFactory():SecureDocumentBuilderFactoryplusdisallow-doctype-decl, so documents with a DOCTYPE are rejected instead of parsed with empty entities.newXMLInputFactory():SecureXMLInputFactoryplusSUPPORT_DTD=false. Used byGbavXMLReader,Brk2SnapshotXMLReaderandDataComfortXMLReader. These already hadSUPPORT_DTD=false, so this is for consistency/defence in depth.The BGT and BAG2 loaders don't depend on brmo-loader and configure their own StAX factory (currently Aalto via SPI, which Commons Secure XML does not list as supported), so there
IS_SUPPORTING_EXTERNAL_ENTITIES=falseis added inline next to the existingSUPPORT_DTD=false. Both properties are accepted by the JDK parser, Woodstox and Aalto. The follow-up PR #2840 replaces Aalto with Woodstox and uses Commons Secure XML there too.Fixed XXE
NhrXMLReader: the nHR input was parsed by the split transformer as aStreamSourceusing an unhardenedTransformerFactory. Settingsource.setSystemId("")did not prevent resolving an absolutefile://external entity. A test showed the file contents were read before this change. Credit to Ray Sabee (Whitehat Security, @raysabee) for finding and reporting this issue!DocumentBuilderFactoryinRsgbTransformer,RsgbBRPTransformer.merge,NhrXMLReader.extractBSN,Brk2Bericht,GbavBerichtandStUFBGasynchroon.BRPXMLReader,WozXMLReaderandRsgbWOZTransformeralready had hardening inline; they now use the shared factory. An unusedDocumentBuilderFactoryinNHRLoadUtilsis removed.Behaviour change
DocumentBuilderparsing (already the case for BRP and WOZ). None of the supported formats use a DTD.TransformerFactorydoes not reject a document with an external entity but resolves it to empty content: the Commons Secure XML resolver takes precedence over the JAXPaccessExternalDTD/accessExternalStylesheetattributes. The external resource is never read.Test plan
SecureXmlFactoriesTest: external entity not resolved by the transformer (also with aURIResolverforxsl:import); rejected by the document builder and StAX; billion laughs rejected; plain XML still worksRsgb*Transformers on real data🤖 Generated with Claude Code