Harden XML parsing via commons-secure-xml

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>

core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java

@@ -51,7 +51,9 @@
 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 @@
      */
     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 @@
         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 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);
@@ -940,7 +941,7 @@
      * @throws SAXException Thrown if a parsing exception occurs
      */
     public Script parse(final File file) throws IOException, SAXException {
         return parse(file.toURL());
     }
 
     /**

core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java

@@ -30,6 +30,7 @@
 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 @@ protected XMLParser createJellyParser() {
      * 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();
     }

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>

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 ParseTag() {
      * 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

jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java

@@ -27,6 +27,7 @@
 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.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.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 TagBodyXMLReader(final Tag tag)
         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 @@ private boolean shouldParseBody() throws JellyTagException {
      * 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 @@ protected URIResolver createURIResolver() {
      *
      * @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 @@ protected XMLReader createXMLReader() throws SAXException {
             xmlReader = new TagBodyXMLReader(this);
         }
         else {
-            xmlReader = XMLReaderFactory.createXMLReader();
+            xmlReader = newSecureXMLReader();
         }
 
         return xmlReader;

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>

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

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

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

pom.xml

@@ -152,6 +152,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>

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>