diff --git a/core/pom.xml b/core/pom.xml index ed6b76214..dc2f8768d 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -52,6 +52,10 @@ + + org.apache.commons + commons-secure-xml + javax.servlet servlet-api 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 7d076cd0c..dfda9d33a 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.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 SAXParser getParser() { 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 boolean getValidating() { 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 c7f263333..0bddc9f02 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.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(); } diff --git a/jelly-tags/xml/pom.xml b/jelly-tags/xml/pom.xml index 507d57835..ac1d1ddbc 100644 --- a/jelly-tags/xml/pom.xml +++ b/jelly-tags/xml/pom.xml @@ -31,6 +31,10 @@ The Jelly XML Tag Library. + + org.apache.commons + commons-secure-xml + commons-jelly commons-jelly 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 b701d5457..a861da5b0 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 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 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 348fe06c0..cc96a0712 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.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; diff --git a/jelly-tags/xmlunit/pom.xml b/jelly-tags/xmlunit/pom.xml index f578cd108..2dd847ba0 100644 --- a/jelly-tags/xmlunit/pom.xml +++ b/jelly-tags/xmlunit/pom.xml @@ -31,6 +31,10 @@ + + org.apache.commons + commons-secure-xml + xmlunit xmlunit 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 6bcab4348..383f8c732 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 1097b80ab..a1a93c42d 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 ffd2ae103..0913c1492 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 b186102c2..02c3be108 100644 --- a/pom.xml +++ b/pom.xml @@ -152,6 +152,15 @@ scp://people.apache.org/www/commons.apache.org/${pom.artifactId.substring(8)}/ + + + + org.apache.commons + commons-secure-xml + 1.0.0 + + + javax.servlet diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 7b3382edf..5c2b3ffe2 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -48,6 +48,7 @@ The type attribute can be add,update,fix,remove. Extract commons version into new POM property ${mx4j.version}. + 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. Throw specialized RuntimeExceptions instead of plain RuntimeExceptions. Deprecate NestedRuntimeException for RuntimeException. Fix building on modern Java.