COMMONSXML-5: SAXParserFactory: capability-driven hardening - #11
Merged
garydgregory merged 2 commits intoJul 5, 2026
Merged
Conversation
Replace the per-implementation class-name dispatch for SAX with a single capability-driven recipe, mirroring the DocumentBuilderFactory rework. A new SAXParserHardener probes feature and property support and adapts: - harden(SAXParserFactory) sets FSP (skipped on Android's Expat, which rejects it) and wraps the factory in the now-universal HardeningSAXParserFactory, since SAXParserFactory exposes no property API. - hardenReader(XMLReader) sets FSP, skips the external DTD subset where supported, applies limits and probes ACCESS_EXTERNAL_*: the bare reader is returned when the JAXP 1.5 properties are honoured (stock JDK), otherwise a deny-all EntityResolver is installed (external Xerces). - Limits.tryApply(XMLReader) picks the Xerces SecurityManager vs JDK limit-property path by class name (the JDK reader is itself a Xerces fork exposing the same property, so a probe cannot tell them apart). - JaxpSetters gains setOptionalFeature(XMLReader) and trySetProperty(XMLReader). StockJdkProvider and XercesProvider lose their SAX recipes; XercesProvider now handles only Schema factories. HardeningSAXParserFactory becomes a single-argument wrapper that returns a HardeningSAXParser, which exposes the hardened reader and a matching SAX 1 Parser (cast or XMLReaderAdapter, lazily) so consumers reaching the parser through either path, including Xalan's identity transformer, get the same hardening. Android keeps its fixups rather than becoming a pass-through. Expat ignores external fetches silently when no resolver is set, so a subset-aware deny-all resolver is installed to fail on external entities while still letting an unused external subset load, and an ExpatReaderWrapper rejects the unsupported namespace-prefixes feature eagerly instead of failing mid-parse. DOM (KXmlParser) remains a pass-through. Behaviour change: the returned SAXParserFactory no longer forces namespace awareness. The previous StockJdkProvider and XercesProvider SAX recipes called setNamespaceAware(true); whether to parse namespace-aware is the caller's decision, not a hardening concern, so it is now left at the implementation default (DOM never set it). The internal Source-parsing path keeps it on: XmlFactories.harden(Source) sets namespace awareness itself, since schema, XSLT and validator sources are parsed in a namespace-aware context. Adds SchemaLocationSaxTest (the SAX counterpart of SchemaLocationDomTest, reusing AttackTestSupport's shared strict reporter) and a SAX case in UnsupportedXmlImplementationTest. AndroidProvider is removed. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
|
The Jira ticket is COMMONSXML-5 |
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
2 participants
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.
Applies the COMMONSXML-1 capability-driven approach to the SAX path, replacing the per-implementation class-name dispatch for
SAXParserFactoryandXMLReaderwith a singleSAXParserHardener.harden(SAXParserFactory)sets FSP and wraps the factory in the now-universalHardeningSAXParserFactory;hardenReader(XMLReader)applies the limits, skips the external DTD subset where supported, and probesACCESS_EXTERNAL_*(bare reader when honoured, deny-allEntityResolverotherwise).HardeningSAXParserFactoryreturns aHardeningSAXParserexposing both the hardenedXMLReaderand a matching SAX 1Parser, so either access path is hardened.StockJdkProvider/XercesProviderlose their SAX recipes;LimitsgainstryApply(XMLReader);JaxpSettersgainssetOptionalFeature(XMLReader)/trySetProperty(XMLReader).AndroidProvideris removed.ExpatReaderWrapperthat rejectsnamespace-prefixeseagerly. Android DOM (KXmlParser) stays a pass-through.Behaviour change: the returned
SAXParserFactoryno longer forces namespace awareness (the previous recipes did); it is left to the caller.XmlFactories.harden(Source)sets it where the schema/XSLT/validator source paths need it.Tests: adds
SchemaLocationSaxTestand a SAX case inUnsupportedXmlImplementationTest; SAX attack assertions remain strict (Android still blocks).mvn verifygreen across stock JDK and external Xerces; the Android/Expat fixups still need a run on the api33 managed device.