Harden XsltTransformer against XXE in the transformed document - #1537
Open
Nexory wants to merge 2 commits into
Open
Harden XsltTransformer against XXE in the transformed document#1537Nexory wants to merge 2 commits into
Nexory wants to merge 2 commits into
Conversation
XsltTransformer.transform() built its TransformerFactory with a plain TransformerFactory.newInstance() and then transformed the component's rendered output as the XML source document. That source is attacker-influenceable (it is whatever the decorated component rendered, including user model data), so an external entity in it was resolved: a crafted document could read local files or reach internal URLs. Enable FEATURE_SECURE_PROCESSING on the factory, mirroring what XSLTResourceStream already does; on the JDK this also denies external DTD and stylesheet access, so the external entity is rejected instead of resolved. Legitimate stylesheet transforms are unaffected (the existing OutputTransformerContainer tests still pass).
Confirms that an external general entity in the transformed source document is not resolved. Reuses the identity anyName.xsl stylesheet from the outputTransformer tests; the test fails (the secret leaks into the output) without the FEATURE_SECURE_PROCESSING guard.
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
1 participant
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.
What
XsltTransformer.transform()builds itsTransformerFactorywith a plainTransformerFactory.newInstance()and then transforms the decoratedcomponent's rendered output as the XML source document. That source is
attacker-influenceable (it is whatever the component rendered, including user
model data), so an external entity in it is resolved: a crafted document can
read local files or reach internal URLs (XXE).
XSLTResourceStream(wicket-util) already hardens its factory withFEATURE_SECURE_PROCESSING(PR #862), but the other XSLT entry point,XsltTransformer, was left untouched. A grep forFEATURE_SECURE_PROCESSINGreturns only that one site.
Fix
Enable
FEATURE_SECURE_PROCESSINGon the factory, mirroringXSLTResourceStream. On the JDK this also denies external DTD and stylesheetaccess, so the external entity is rejected instead of resolved. No change for
legitimate stylesheet transforms.
Verification
the source document leaks a local file's contents into the transform output;
with
FEATURE_SECURE_PROCESSINGset the parse is rejected (theaccessExternalDTDrestriction denies thefileaccess).OutputTransformerContainerand transformer tests still pass,so legitimate stylesheet transforms are unaffected.
This is defense-in-depth hardening, mirroring the existing
XSLTResourceStreamchange.