Emond Papegaaij created WICKET-7201:
---------------------------------------
Summary: Back-port the XsltTransformer secure-processing hardening
to 10.x, 9.x and 8.x
Key: WICKET-7201
URL: https://issues.apache.org/jira/browse/WICKET-7201
Project: Wicket
Issue Type: Improvement
Components: wicket-core
Affects Versions: 9.24.0, 8.19.0, 10.10.0
Reporter: Emond Papegaaij
{{XsltTransformer}} builds its transformer with a bare
{{TransformerFactory.newInstance()}} and then parses the decorated component's
rendered markup with it. Without {{XMLConstants.FEATURE_SECURE_PROCESSING}} the
JDK resolves external general entities and external DTD subsets, and
{{xsl:import}} of an external stylesheet.
Master gained the hardening in August:
{code:java}
TransformerFactory tFactory = TransformerFactory.newInstance();
tFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
{code}
The three release lines still have the bare {{newInstance()}} — no
{{setFeature}}, no {{setAttribute}}, no {{XMLConstants}} import — and the
regression test that came with the master change is absent there too.
h3. This is defence in depth, not a live vulnerability
Please read this part before treating the ticket as either urgent or pointless.
No route was found by which an attacker supplies the XML. What prevents it is
ordinary markup escaping, not the transformer: model data cannot contain {{<}},
{{>}} or {{&}} in the default configuration, and a DOCTYPE must precede the
root element. Reaching the sink needs an application to XSLT-decorate a
component, render that region body-only, place unescaped output first inside
it, *and* feed that output untrusted data. The regression test that accompanies
the master fix bypasses the render pipeline and hand-writes the malicious
source, which is itself an indication that no chain through a normal render was
demonstrated.
So there is nothing to embargo and nothing to announce. It is a missing second
line of defence that master has and the release lines do not.
For context on why it is worth closing anyway: the same missing {{setFeature}}
call in the sibling class {{XSLTResourceStream}} was published as
CVE-2024-36522, "Apache Wicket: Remote code execution via XSLT injection",
CWE-74, rated High, and fixed in 8.16.0, 9.18.0 and 10.1.0. That advisory names
only that one class. Someone who read it and upgraded could reasonably believe
Wicket's XSLT external-entity exposure was closed on their line;
{{XsltTransformer}} still has the bare {{newInstance()}} two years later. The
two classes share the defect and were treated differently.
h3. What to change
# Add the {{setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true)}} call and
the {{javax.xml.XMLConstants}} import to {{XsltTransformer}} on 10.x, 9.x and
8.x.
# Port the regression test that came with the master change to all three lines.
h3. Two traps for whoever implements it
*Do not verify by reading the feature back.*
{{getFeature(FEATURE_SECURE_PROCESSING)}} returns {{true}} on an *unhardened*
factory while external DTD access is still fully open. Reading it tells you
nothing. A valid check asserts that an external entity in the source is refused
— which is what the regression test does.
*Do not also set {{accessExternalDTD}} and {{accessExternalStylesheet}}.* On
the JDK's built-in transformer the feature already implies both, and setting
them explicitly changes nothing on JDK 8 through 25. They would only matter if
a third-party {{TransformerFactory}} such as Xalan-J or Saxon won the JAXP
lookup. Wicket depends on neither, and that case is untested — please leave it
out of scope here rather than adding lines whose effect nobody has measured.
h3. Not part of this ticket
{{XSLTResourceStream}} is already hardened on all four lines. Only its
caller-supplied-factory constructor is not, identically on every line, so that
is a separate concern and not a back-port gap.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
