Secure getAssociatedStylesheet, and pin the CI Zulu job to JDK 8u152 - #74
Merged
Conversation
The Zulu job exists to cover the oldest JDK 8 behavior the securing has to work against, but 8u201 already carries the 8u162 backport wave. Pin it to 8u152, the last release before XSLTC's getAssociatedStylesheet began honoring the XMLReader carried by a SAXSource: through 8u152 it self-provisions a parser instead, which the newer job cannot exercise. jdk.xml.overrideDefaultParser arrived in the same wave, so OverrideDefaultParserTest now skips where the runtime does not recognize the feature rather than failing on it. Assisted-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33
The PI scan and the href it yields are each derived from the scanned document, and each reached an implementation unfiltered. The scan: Apache Xalan provisions its own parser instead of using the one a SAXSource carries (XALANJ-2849), and the JDK's XSLTC did the same before 8u162, so on Java 8 the document was scanned by a parser this library never configured. Pre-parse to a DOM for those engines, keying on Java 8 as a whole since the patch levels are indistinguishable through any API. Include a SAXSource that carries the caller's own reader: an engine that reaches this path drops that reader anyway, so the choice is only between this parse and the engine's own. The href: XSLTC-lineage engines resolve it during the scan, before the factory's URIResolver is installed, and hand back a live Source naming the absolutized URI, which compiling would then fetch. Route it through the floor like any other content-named reference, so a caller resolver opts it in and everything else resolves to empty. Saxon already behaved this way, so the engines now agree. Assisted-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33
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.
Pushed as a branch on
apache/commons-secure-xmlrather than from a fork, so the workflow change takes effect on this PR's own run.getAssociatedStylesheettakes two untrusted inputs — the document it scans, and thehrefthexml-stylesheetPI names — and each reached an implementation unfiltered. Both are fixed here, and the CI job that makes the first one reproducible is moved at the same time so the change and its coverage land together.The scan
Apache Xalan provisions its own parser rather than using the one a
SAXSourcecarries (XALANJ-2849), which the wrapper already worked around with a DOM pre-parse. The JDK's XSLTC did exactly the same before 8u162: through 8u152getAssociatedStylesheetnever inspects the source and always builds a parser viaFactoryImpl.getSAXFactory, so on Java 8 the document was scanned by a parser this library never configured. With external Xerces on the classpath — whereFEATURE_SECURE_PROCESSINGbounds expansion but does not restrict external DTD or entity resolution — the prolog's external DTD was fetched outright.The DOM pre-parse now applies when the delegate is Xalan or the runtime is Java 8. Java 8 as a whole is the boundary rather than 8u162: the patch levels are indistinguishable through any API, and this method is rarely called, so a runtime that old pays a DOM materialisation it will not notice.
The pre-parse also now covers a
SAXSourcethat carries the caller's own reader. That is the one place in this class where a caller-supplied reader is not honoured, and deliberately so: an engine that reaches this path discards that reader regardless, so the choice is only between this parse and the engine's unsecured one. Previously such a source fell through to the implementation, which the class Javadoc wrongly described as safe.The href
XSLTC-lineage engines resolve the PI href during the scan — before they install the factory's
URIResolver(the misplaced "bug 24187" fix) — and return a liveSAXSourcenaming the absolutized URI. Since compiling the returnedSourceis this method's one documented use, that handed back a URI chosen by the parsed document, to be fetched and executed on the next call.The result is now routed through the same floor as any other content-named reference: a caller's
URIResolveropts an href in, everything else resolves to empty. Saxon already floored the href itself, so this also makes the engines agree.Behavioural change for callers, documented on
SecureTransformerFactory: the returnedSourcecarries empty content rather than naming the URI unless aURIResolveropts that href in. Compiling it therefore compiles the caller's opted-in stylesheet or an empty one — never one the document selected.CI
The Zulu job existed to cover the oldest JDK 8 behaviour the securing has to work against, but its pin (8u201) already carried the 8u162 backport wave, so it duplicated the other Java 8 jobs. It moves to 8u152, the last release before that wave, which is what makes the scan gap above reproducible in CI at all.
jdk.xml.overrideDefaultParserarrived in the same wave, soOverrideDefaultParserTestnow probes for the feature and skips where the runtime lacks it, instead of failing 28 times for its absence.Verification
AssociatedStylesheetTest.secureGetAssociatedStylesheetIgnoresExternalDtdonstockjdkandjdk-xerces).getAssociatedStylesheeton 8u152 with Xerces present, against a prolog declaring an unreachable external DTD — went fromConnectException(fetch attempted) to completing with no fetch.AssociatedStylesheetTestgains an opt-in case and a caller-supplied-reader case; its previous assertion encoded the pre-fix behaviour and was reworked accordingly.javadoc:javadocclean.🤖 Generated with Claude Code