This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-jelly.git
The following commit(s) were added to refs/heads/master by this push:
new 44a663be Harden XML parsing via commons-secure-xml (#108)
44a663be is described below
commit 44a663beee6d93417469382b4016719796197062
Author: Piotr P. Karwasz <[email protected]>
AuthorDate: Mon Sep 7 23:42:35 2026 +0200
Harden XML parsing via commons-secure-xml (#108)
* Harden XML parsing via commons-secure-xml
Create SAX parsers and readers through
org.apache.commons:commons-secure-xml. The secure factory enables
FEATURE_SECURE_PROCESSING and installs a non-removable entity-resolver
floor on every parser it produces: external DTD and entity lookups that
a caller-set resolver does not resolve are resolved to empty content
instead of being fetched, and internal entity expansion is bounded,
regardless of the JAXP implementation on the classpath.
Changes:
- Add the commons-secure-xml dependency (1.0.0-SNAPSHOT until its first
release) to core, jelly-tags/xml and jelly-tags/xmlunit.
- core XMLParser keeps the documented
JellyContext.setAllowDtdToCallExternalEntities(true) opt-in working by
using a plain factory on that path; the default path uses the secure
factory, and a factory assigned to the protected static field still
wins. The flag-dependent choice is no longer cached in that field.
- core ParseTag (which had no hardening at all) now creates its reader
through the secure factory.
- jelly-tags/xml: TransformTag's readers and ParseTag's dom4j SAXReader
are built from the secure factory; dom4j and XMLReaderFactory
otherwise provision readers through JAXP at their own defaults, and
the deprecated org.xml.sax.driver system property no longer selects
the reader class. The TransformerFactory itself stays unsecured for
now: Xalan, which this module puts on the class path, drops the
attributes of xsl:namespace-alias literal result elements under
secure processing (XSLTElementProcessor rejects "foreign" attributes
as non-fatal errors), silently breaking stylesheets such as the
Schematron skeleton.
- jelly-tags/xmlunit: the assertion tags' dom4j SAXReaders are built
from the secure factory.
- jelly-tags/html is unchanged: NekoHTML is an HTML scanner, not an XML
parser.
- Run the CI and CodeQL builds with -Puse-apache-snapshots (inherited
from the org.apache:apache parent POM) so the commons-secure-xml
SNAPSHOT resolves; CodeQL's autobuild receives the profile through
MAVEN_ARGS.
Assisted-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MHgnMnGWHQoH2zD2jFdoMT
* Use the Commons Secure XML 1.0.0 release candidate
Bump org.apache.commons:commons-secure-xml from 1.0.0-SNAPSHOT to 1.0.0
and add the temporary staging repository
https://repository.apache.org/content/repositories/orgapachecommons-1962/
after Central, so the vote gets downstream CI results. Drop the
-Puse-apache-snapshots profile from the CI workflows, which the release
version no longer needs. Remove the staging repository once 1.0.0 is
released.
Assisted-By: Claude Fable 5.1 <[email protected]>
Claude-Session: https://claude.ai/code/session_0167e29ScPEdfzJnEFm95imK
* Bump Apache Commons Secure XML from 1.0.0-SNAPSHOT to 1.0.0
* fix: use allow-all resolver for `allowDtdToCallExternalEntities`
* fix: simplify getParser()
* Update XMLReader factory method to use newNSInstance
* Remove namespace awareness from XMLReader creation
Removed namespace awareness setting from XMLReader factory method.
* Update XMLParser to use newNSInstance for factory
---------
Co-authored-by: Gary Gregory <[email protected]>
Co-authored-by: Gary Gregory <[email protected]>
---
core/pom.xml | 4 ++++
.../org/apache/commons/jelly/parser/XMLParser.java | 13 +++++------
.../apache/commons/jelly/tags/core/ParseTag.java | 4 ++--
jelly-tags/xml/pom.xml | 4 ++++
.../apache/commons/jelly/tags/xml/ParseTag.java | 13 +++++++++--
.../commons/jelly/tags/xml/TransformTag.java | 25 +++++++++++++++++-----
jelly-tags/xmlunit/pom.xml | 4 ++++
.../commons/jelly/tags/xmlunit/ActualTag.java | 11 +++++++++-
.../tags/xmlunit/AssertDocumentsEqualTag.java | 11 +++++++++-
.../commons/jelly/tags/xmlunit/ExpectedTag.java | 11 +++++++++-
pom.xml | 9 ++++++++
src/changes/changes.xml | 1 +
12 files changed, 92 insertions(+), 18 deletions(-)
diff --git a/core/pom.xml b/core/pom.xml
index f3acc7c1..8c61ef84 100644
--- a/core/pom.xml
+++ b/core/pom.xml
@@ -52,6 +52,10 @@
</plugins>
</build>
<dependencies>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-secure-xml</artifactId>
+ </dependency>
<dependency>
<groupId>javax.servlet</groupId>
<artifactId>servlet-api</artifactId>
diff --git a/core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java
b/core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java
index 7d076cd0..dfda9d33 100644
--- a/core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java
+++ b/core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java
@@ -51,7 +51,9 @@ import org.apache.commons.jelly.impl.TextScript;
import org.apache.commons.jelly.util.ClassLoaderUtils;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
import org.xml.sax.Attributes;
+import org.xml.sax.EntityResolver;
import org.xml.sax.ErrorHandler;
import org.xml.sax.InputSource;
import org.xml.sax.Locator;
@@ -76,6 +78,8 @@ public class XMLParser extends DefaultHandler {
*/
protected static SAXParserFactory factory = null;
+ private static final EntityResolver ALLOW_ALL_RESOLVER = (publicId,
systemId) -> new InputSource(systemId);
+
/** JellyContext which is used to locate tag libraries*/
private JellyContext context = new JellyContext();
@@ -811,9 +815,8 @@ public class XMLParser extends DefaultHandler {
synchronized (this) {
try {
if (factory == null) {
- factory = SAXParserFactory.newInstance();
+ factory = SecureSAXParserFactory.newNSInstance();
}
- factory.setNamespaceAware(true);
factory.setValidating(validating);
parser = factory.newSAXParser();
return parser;
@@ -876,10 +879,8 @@ public class XMLParser extends DefaultHandler {
public synchronized XMLReader getXMLReader() throws SAXException {
if (reader == null) {
reader = getParser().getXMLReader();
- if (!allowDtdToCallExternalEntities) {
-
reader.setFeature("http://xml.org/sax/features/external-general-entities";,
false);
-
reader.setFeature("http://xml.org/sax/features/external-parameter-entities";,
false);
-
reader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd";,
false);
+ if (isAllowDtdToCallExternalEntities()) {
+ reader.setEntityResolver(ALLOW_ALL_RESOLVER);
}
if (this.defaultNamespaceURI != null) {
reader = new DefaultNamespaceFilter(this.defaultNamespaceURI,
reader);
diff --git
a/core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java
b/core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java
index c7f26333..0bddc9f0 100644
--- a/core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java
+++ b/core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java
@@ -30,6 +30,7 @@ import org.apache.commons.jelly.XMLOutput;
import org.apache.commons.jelly.parser.XMLParser;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
import org.xml.sax.ContentHandler;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;
@@ -76,8 +77,7 @@ public class ParseTag extends TagSupport {
* Factory method to create a new XMLReader
*/
protected XMLReader createXMLReader() throws ParserConfigurationException,
SAXException {
- final SAXParserFactory factory = SAXParserFactory.newInstance();
- factory.setNamespaceAware(true);
+ final SAXParserFactory factory =
SecureSAXParserFactory.newNSInstance();
final SAXParser parser = factory.newSAXParser();
return parser.getXMLReader();
}
diff --git a/jelly-tags/xml/pom.xml b/jelly-tags/xml/pom.xml
index 507d5783..ac1d1ddb 100644
--- a/jelly-tags/xml/pom.xml
+++ b/jelly-tags/xml/pom.xml
@@ -31,6 +31,10 @@
<description>The Jelly XML Tag Library.</description>
<dependencies>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-secure-xml</artifactId>
+ </dependency>
<dependency>
<groupId>commons-jelly</groupId>
<artifactId>commons-jelly</artifactId>
diff --git
a/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/ParseTag.java
b/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/ParseTag.java
index b701d545..a861da5b 100644
---
a/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/ParseTag.java
+++
b/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/ParseTag.java
@@ -16,13 +16,17 @@
*/
package org.apache.commons.jelly.tags.xml;
+import javax.xml.parsers.ParserConfigurationException;
+
import org.apache.commons.jelly.JellyTagException;
import org.apache.commons.jelly.MissingAttributeException;
import org.apache.commons.jelly.XMLOutput;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
import org.dom4j.Document;
import org.dom4j.io.SAXReader;
+import org.xml.sax.SAXException;
/** A tag which parses some XML and defines a variable with the parsed
Document.
* The XML can either be specified as its body or can be passed in via the
@@ -47,8 +51,13 @@ public class ParseTag extends ParseTagSupport {
* Factory method to create a new SAXReader
*/
@Override
- protected SAXReader createSAXReader() {
- return new SAXReader(validate);
+ protected SAXReader createSAXReader() throws SAXException {
+ // dom4j builds its reader through JAXP internally; hand it one from
the secure factory instead.
+ try {
+ return new
SAXReader(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader(),
validate);
+ } catch (final ParserConfigurationException e) {
+ throw new SAXException(e);
+ }
}
@Override
diff --git
a/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java
b/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java
index 348fe06c..cc96a071 100644
---
a/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java
+++
b/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java
@@ -27,6 +27,7 @@ import java.net.URL;
import java.util.Iterator;
import java.util.List;
+import javax.xml.parsers.ParserConfigurationException;
import javax.xml.transform.Result;
import javax.xml.transform.TransformerConfigurationException;
import javax.xml.transform.TransformerFactory;
@@ -48,6 +49,7 @@ import org.apache.commons.jelly.impl.StaticTagScript;
import org.apache.commons.jelly.impl.TagScript;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
import org.dom4j.Document;
import org.dom4j.io.DocumentResult;
import org.dom4j.io.DocumentSource;
@@ -61,7 +63,6 @@ import org.xml.sax.SAXNotRecognizedException;
import org.xml.sax.SAXNotSupportedException;
import org.xml.sax.XMLReader;
import org.xml.sax.ext.LexicalHandler;
-import org.xml.sax.helpers.XMLReaderFactory;
/** A tag which parses some XML, applies an xslt transform to it
* and defines a variable with the transformed Document.
@@ -129,7 +130,7 @@ public class TransformTag extends ParseTag {
private void doInvokeBody() throws SAXException {
try {
if (this.shouldParseBody()) {
- final XMLReader anXMLReader =
XMLReaderFactory.createXMLReader();
+ final XMLReader anXMLReader = newSecureXMLReader();
anXMLReader.setContentHandler(this.xmlOutput);
anXMLReader.setProperty(LEXICAL_HANDLER_PROPERTY,this.xmlOutput);
final StringWriter writer = new StringWriter();
@@ -408,6 +409,10 @@ public class TransformTag extends ParseTag {
* Constructor for TransformTag.
*/
public TransformTag() {
+ // Not the secure factory: Xalan (on the class path here) drops the
attributes of
+ // xsl:namespace-alias literal result elements under secure processing
(XSLTElementProcessor
+ // rejects "foreign" attributes), silently breaking stylesheets like
the Schematron skeleton.
+ // The stylesheet is part of the script; the readers parsing the
transform INPUT are secured.
this.tf = (SAXTransformerFactory) TransformerFactory.newInstance();
}
@@ -462,9 +467,19 @@ public class TransformTag extends ParseTag {
*
* @return XMLReader for the transform input
* @throws SAXException
- * If the value of the "org.xml.sax.driver" system property
- * is null, or if the class cannot be loaded and instantiated.
+ * If the reader cannot be created.
*/
+ /**
+ * Creates a namespace-aware XMLReader through the secure SAX parser
factory.
+ */
+ private static XMLReader newSecureXMLReader() throws SAXException {
+ try {
+ return
SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader();
+ } catch (final ParserConfigurationException e) {
+ throw new SAXException(e);
+ }
+ }
+
protected XMLReader createXMLReader() throws SAXException {
XMLReader xmlReader = null;
final Object xmlReaderSourceObj = this.getXml();
@@ -474,7 +489,7 @@ public class TransformTag extends ParseTag {
xmlReader = new TagBodyXMLReader(this);
}
else {
- xmlReader = XMLReaderFactory.createXMLReader();
+ xmlReader = newSecureXMLReader();
}
return xmlReader;
diff --git a/jelly-tags/xmlunit/pom.xml b/jelly-tags/xmlunit/pom.xml
index f578cd10..2dd847ba 100644
--- a/jelly-tags/xmlunit/pom.xml
+++ b/jelly-tags/xmlunit/pom.xml
@@ -31,6 +31,10 @@
<dependencies>
<!-- START for compilation -->
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-secure-xml</artifactId>
+ </dependency>
<dependency>
<groupId>xmlunit</groupId>
<artifactId>xmlunit</artifactId>
diff --git
a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ActualTag.java
b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ActualTag.java
index 6bcab434..383f8c73 100644
---
a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ActualTag.java
+++
b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ActualTag.java
@@ -17,16 +17,25 @@
package org.apache.commons.jelly.tags.xmlunit;
+import javax.xml.parsers.ParserConfigurationException;
+
import org.apache.commons.jelly.JellyTagException;
import org.apache.commons.jelly.XMLOutput;
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
import org.dom4j.Document;
import org.dom4j.io.SAXReader;
+import org.xml.sax.SAXException;
public class ActualTag extends XMLUnitTagSupport {
@Override
protected SAXReader createSAXReader() {
- return new SAXReader();
+ // dom4j builds its reader through JAXP internally; hand it one from
the secure factory instead.
+ try {
+ return new
SAXReader(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader());
+ } catch (final ParserConfigurationException | SAXException e) {
+ throw new IllegalStateException("Unable to create a new XML
reader", e);
+ }
}
@Override
diff --git
a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/AssertDocumentsEqualTag.java
b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/AssertDocumentsEqualTag.java
index 1097b80a..a1a93c42 100644
---
a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/AssertDocumentsEqualTag.java
+++
b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/AssertDocumentsEqualTag.java
@@ -17,12 +17,16 @@
package org.apache.commons.jelly.tags.xmlunit;
+import javax.xml.parsers.ParserConfigurationException;
+
import org.apache.commons.jelly.JellyTagException;
import org.apache.commons.jelly.XMLOutput;
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
import org.custommonkey.xmlunit.Diff;
import org.custommonkey.xmlunit.XMLUnit;
import org.dom4j.Document;
import org.dom4j.io.SAXReader;
+import org.xml.sax.SAXException;
/**
* Compares two XML documents using XMLUnit (http://xmlunit.sourceforge.net/).
@@ -46,7 +50,12 @@ public class AssertDocumentsEqualTag extends
XMLUnitTagSupport {
@Override
protected SAXReader createSAXReader() {
- return new SAXReader();
+ // dom4j builds its reader through JAXP internally; hand it one from
the secure factory instead.
+ try {
+ return new
SAXReader(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader());
+ } catch (final ParserConfigurationException | SAXException e) {
+ throw new IllegalStateException("Unable to create a new XML
reader", e);
+ }
}
@Override
diff --git
a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ExpectedTag.java
b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ExpectedTag.java
index ffd2ae10..0913c149 100644
---
a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ExpectedTag.java
+++
b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ExpectedTag.java
@@ -17,16 +17,25 @@
package org.apache.commons.jelly.tags.xmlunit;
+import javax.xml.parsers.ParserConfigurationException;
+
import org.apache.commons.jelly.JellyTagException;
import org.apache.commons.jelly.XMLOutput;
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
import org.dom4j.Document;
import org.dom4j.io.SAXReader;
+import org.xml.sax.SAXException;
public class ExpectedTag extends XMLUnitTagSupport {
@Override
protected SAXReader createSAXReader() {
- return new SAXReader();
+ // dom4j builds its reader through JAXP internally; hand it one from
the secure factory instead.
+ try {
+ return new
SAXReader(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader());
+ } catch (final ParserConfigurationException | SAXException e) {
+ throw new IllegalStateException("Unable to create a new XML
reader", e);
+ }
}
@Override
diff --git a/pom.xml b/pom.xml
index 8787493d..c097a0c0 100644
--- a/pom.xml
+++ b/pom.xml
@@ -153,6 +153,15 @@
<url>scp://people.apache.org/www/commons.apache.org/${pom.artifactId.substring(8)}/</url>
</site>
</distributionManagement>
+ <dependencyManagement>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-secure-xml</artifactId>
+ <version>1.0.0</version>
+ </dependency>
+ </dependencies>
+ </dependencyManagement>
<dependencies>
<dependency>
<groupId>javax.servlet</groupId>
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 8d4f6d8b..efebd7a0 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -48,6 +48,7 @@ The <action> type attribute can be add,update,fix,remove.
<!-- ADD -->
<action type="add" dev="ggregory" due-to="Gary Gregory">Extract commons
version into new POM property ${mx4j.version}.</action>
<!-- FIX -->
+ <action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz, Gary
Gregory">Create XML parsers and readers through
org.apache.commons:commons-secure-xml, so external entities and DTDs are no
longer fetched unless JellyContext.setAllowDtdToCallExternalEntities(true) is
used.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">Throw
specialized RuntimeExceptions instead of plain RuntimeExceptions.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">Deprecate
NestedRuntimeException for RuntimeException.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">Fix building on
modern Java.</action>
