Harden XML parsing via commons-secure-xml

pom.xml

@@ -139,6 +139,11 @@
   </contributors>
 
   <dependencies>
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-secure-xml</artifactId>
+      <version>1.0.0</version>
+    </dependency>
     <dependency>
       <groupId>commons-logging</groupId>
       <artifactId>commons-logging</artifactId>

src/changes/changes.xml

@@ -30,6 +30,8 @@
         [18-10-2018] Before executing invoke handlers after a macrostep all internal events must have been processed
       </action>
       <action type="fix" dev="ggregory" due-to="Gary Gregory">Fix Apache RAT plugin console warnings.</action>
+      <action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz, Gary Gregory">Create XML parsers, stream readers and transformers through org.apache.commons:commons-secure-xml, so external entities and DTDs are no longer fetched by default.</action>
+      <action type="fix" dev="pkarwasz">ContentParser.parseXml now parses its argument as XML content instead of interpreting it as a URI.</action>
       <!-- UPDATE -->
       <action dev="woonsan" type="update" issue="SCXML-284" due-to="Allon Mureinik">
         [10-10-2018] Clear up exception handling in tests

src/main/java/org/apache/commons/scxml2/io/ContentParser.java

@@ -18,17 +18,16 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.StringReader;
 import java.io.StringWriter;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.Properties;
 
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.transform.OutputKeys;
 import javax.xml.transform.Transformer;
 import javax.xml.transform.TransformerException;
-import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
 
@@ -37,8 +36,11 @@
 import org.apache.commons.scxml2.model.NodeValue;
 import org.apache.commons.scxml2.model.ParsedValue;
 import org.apache.commons.scxml2.model.TextValue;
+import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
+import org.apache.commons.xml.secure.SecureTransformerFactory;
 import org.w3c.dom.Document;
 import org.w3c.dom.Node;
+import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 
 import com.fasterxml.jackson.core.JsonParser;
@@ -224,7 +226,8 @@ public ParsedValue parseResource(final String resourceURL) throws IOException {
     public Node parseXml(final String xmlString) throws IOException {
         Document doc;
         try {
-            doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(xmlString);
+            // Wrap in an InputSource: DocumentBuilder.parse(String) would interpret the content as a URI.
+            doc = SecureDocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new InputSource(new StringReader(xmlString)));
         } catch (SAXException | ParserConfigurationException e) {
             throw new IOException(e);
         }
@@ -252,7 +255,7 @@ public String toJson(final Object jsonObject) throws IOException {
     public String toXml(final Node node) throws IOException {
         try {
             final StringWriter writer = new StringWriter();
-            final Transformer transformer = TransformerFactory.newInstance().newTransformer();
+            final Transformer transformer = SecureTransformerFactory.newInstance().newTransformer();
             final Properties outputProps = new Properties();
             outputProps.put(OutputKeys.OMIT_XML_DECLARATION, "no");
             outputProps.put(OutputKeys.STANDALONE, "no");

src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java

@@ -31,7 +31,6 @@
 import java.util.List;
 import java.util.Map;
 
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.stream.Location;
 import javax.xml.stream.XMLInputFactory;
@@ -98,6 +97,9 @@
 import org.apache.commons.scxml2.model.TransitionType;
 import org.apache.commons.scxml2.model.TransitionalState;
 import org.apache.commons.scxml2.model.Var;
+import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
+import org.apache.commons.xml.secure.SecureSchemaFactory;
+import org.apache.commons.xml.secure.SecureXMLInputFactory;
 import org.w3c.dom.Attr;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
@@ -589,9 +591,11 @@ private static XMLStreamReader getReader(final Configuration configuration, fina
             throws IOException, XMLStreamException {
 
         // Instantiate the XMLInputFactory
-        XMLInputFactory factory = XMLInputFactory.newInstance();
+        final XMLInputFactory factory;
         if (configuration.factoryId != null && configuration.factoryClassLoader != null) {
-            factory = XMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader);
+            factory = SecureXMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader);
+        } else {
+            factory = SecureXMLInputFactory.newInstance();
         }
         factory.setEventAllocator(configuration.allocator);
         if (factory.isPropertySupported(XMLInputFactory_JDK_PROP_REPORT_CDATA)) {
@@ -623,7 +627,7 @@ private static XMLStreamReader getReader(final Configuration configuration, fina
             // Validation requires us to use a Source
 
             final URL scxmlSchema = new URL("TODO"); // TODO, point to appropriate location
-            final SchemaFactory schemaFactory = SchemaFactory.newInstance("http://www.w3.org/2001/XMLSchema");
+            final SchemaFactory schemaFactory = SecureSchemaFactory.newInstance("http://www.w3.org/2001/XMLSchema");
             Schema schema;
             try {
                 schema = schemaFactory.newSchema(scxmlSchema);
@@ -1341,7 +1345,7 @@ private static Element readElement(final XMLStreamReader reader)
         // Create a document in which to build the DOM node
         Document document;
         try {
-            document = DocumentBuilderFactory.newInstance().newDocumentBuilder().newDocument();
+            document = SecureDocumentBuilderFactory.newInstance().newDocumentBuilder().newDocument();
         } catch (final ParserConfigurationException pce) {
             throw new XMLStreamException(ERR_PARSER_CFG);
         }

src/main/java/org/apache/commons/scxml2/io/SCXMLWriter.java

@@ -81,6 +81,7 @@
 import org.apache.commons.scxml2.model.Transition;
 import org.apache.commons.scxml2.model.TransitionTarget;
 import org.apache.commons.scxml2.model.Var;
+import org.apache.commons.xml.secure.SecureTransformerFactory;
 import org.w3c.dom.Node;
 
 /**
@@ -329,25 +330,34 @@ private static String escapeXML(final String str) {
 
     /**
      * Gets a {@link Transformer} instance that pretty prints the output.
+     * <p>
+     * A failure here can only be caused by the TrAX implementation available on the class path: either no
+     * {@link TransformerFactory} can be instantiated at all, or the one that is instantiated rejects the
+     * output properties this writer requires.
+     * </p>
      *
      * @return Transformer The indenting {@link Transformer} instance.
+     * @throws IllegalStateException if no suitable {@link Transformer} can be created.
      */
     private static Transformer getTransformer() {
-        Transformer transformer;
         final Properties outputProps = new Properties();
         outputProps.put(OutputKeys.OMIT_XML_DECLARATION, "yes");
         outputProps.put(OutputKeys.STANDALONE, "no");
         outputProps.put(OutputKeys.INDENT, "yes");
+        TransformerFactory factory = null;
         try {
-            final TransformerFactory tfFactory = TransformerFactory.newInstance();
-            transformer = tfFactory.newTransformer();
+            factory = SecureTransformerFactory.newInstance();
+            final Transformer transformer = factory.newTransformer();
             transformer.setOutputProperties(outputProps);
-        } catch (TransformerFactoryConfigurationError | TransformerConfigurationException t) {
-            final org.apache.commons.logging.Log log = LogFactory.getLog(SCXMLWriter.class);
-            log.error(t.getMessage(), t);
-            return null;
+            return transformer;
+        } catch (final IllegalArgumentException | TransformerConfigurationException | TransformerFactoryConfigurationError t) {
+            final String message = "Unable to create the XML transformer used to pretty print SCXML documents: " +
+                    (factory != null
+                            ? "the TrAX implementation " + factory.getClass().getName() + " does not support the required output properties."
+                            : "no TrAX implementation is available on the class path.");
+            LogFactory.getLog(SCXMLWriter.class).error(message, t);
+            throw new IllegalStateException(message, t);
         }
-        return transformer;
     }
 
     /**
@@ -1130,7 +1140,7 @@ private static void writePretty(final Configuration configuration, final OutputS
             prettyPrintResult = scxmlResult;
         }
 
-        final TransformerFactory factory = TransformerFactory.newInstance();
+        final TransformerFactory factory = SecureTransformerFactory.newInstance();
         try {
             final Transformer transformer = factory.newTransformer();
             if (configuration.encoding != null) {

src/test/java/org/apache/commons/scxml2/io/ContentParserTest.java

@@ -17,12 +17,20 @@
 package org.apache.commons.scxml2.io;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
 
+import javax.xml.parsers.DocumentBuilder;
+
+import org.apache.commons.scxml2.model.NodeValue;
+import org.apache.commons.scxml2.model.ParsedValue;
 import org.junit.jupiter.api.Test;
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
 
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.databind.ObjectMapper;
@@ -54,6 +62,31 @@ void testParseJson() throws Exception {
         assertEquals(jsonArray, contentParser.parseJson(jsonArrayString));
     }
 
+    /**
+     * The XML string must be parsed as content.
+     *
+     * <p>{@link DocumentBuilder#parse(String)}, previously used, interpreted it as a URI.</p>
+     */
+    @Test
+    void testParseXml() throws Exception {
+        final ContentParser contentParser = new ContentParser();
+
+        final Node node = contentParser.parseXml("<?xml version=\"1.0\"?><root attr=\"value\">text</root>");
+        assertInstanceOf(Element.class, node);
+        assertEquals("root", node.getNodeName());
+        assertEquals("value", ((Element) node).getAttribute("attr"));
+        assertEquals("text", node.getTextContent());
+
+        final ParsedValue parsedValue = contentParser.parseContent("<?xml version=\"1.0\"?><root attr=\"value\">text</root>");
+        assertInstanceOf(NodeValue.class, parsedValue);
+        assertEquals("root", ((Node) parsedValue.getValue()).getNodeName());
+
+        // Round trip: the serialized node parses back into an equivalent node
+        final String xml = contentParser.toXml(node);
+        assertTrue(xml.contains("<root attr=\"value\">text</root>"), xml);
+        assertEquals("text", contentParser.parseXml(xml).getTextContent());
+    }
+
     @Test
     void testSpaceNormalizeContent() {
         assertNull(ContentParser.spaceNormalizeContent(null));