Adopt Apache Commons Secure XML for XML parsing

CHANGES.md

@@ -1,5 +1,15 @@
 # Apache Roller — Changes
 
+## 6.1.7
+
+### Behaviour changes worth reading before upgrading
+
+- **XML parsing uses Apache Commons Secure XML.** Roller now bundles
+  `commons-secure-xml` 1.0.0 and builds all of its XML parsers through it.
+- **Startup fails if the XML-RPC parser cannot be configured.** Roller used to
+  log an error and continue. It now stops at startup, so check the log if a
+  custom XML parser is on the classpath.
+
 ## 6.1.6
 
 Initial installation now requires a one-time, cryptographically secure setup token printed to the server log. Bootstrap access closes as soon as setup finishes — when the first administrator is created on a new site, or when the database upgrade completes on an existing one.

app/pom.xml

@@ -46,6 +46,7 @@ limitations under the License.
         <commons-codec.version>1.22.1</commons-codec.version>
         <commons-text.version>1.15.0</commons-text.version>
         <commons-lang3.version>3.20.0</commons-lang3.version>
+        <commons-secure-xml.version>1.0.0</commons-secure-xml.version>
         <eclipse-link.version>4.0.9</eclipse-link.version>
         <guice.version>7.0.0</guice.version>
         <log4j2.version>2.26.1</log4j2.version>
@@ -375,6 +376,12 @@ limitations under the License.
             <version>${commons-lang3.version}</version>
         </dependency>
 
+        <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-secure-xml</artifactId>
+            <version>${commons-secure-xml.version}</version>
+        </dependency>
+
         <dependency>
             <groupId>org.apache.xmlrpc</groupId>
             <artifactId>xmlrpc-common</artifactId>

app/src/main/java/org/apache/roller/weblogger/business/WebloggerImpl.java

@@ -31,13 +31,8 @@
 import org.apache.roller.weblogger.business.search.IndexManager;
 import org.apache.roller.weblogger.business.themes.ThemeManager;
 import org.apache.roller.weblogger.config.PingConfig;
-import org.apache.xmlrpc.util.SAXParsers;
-import org.xml.sax.SAXNotRecognizedException;
-import org.xml.sax.SAXNotSupportedException;
+import org.apache.roller.weblogger.util.SecureXmlParsers;
 
-import javax.xml.XMLConstants;
-import javax.xml.parsers.ParserConfigurationException;
-import javax.xml.parsers.SAXParserFactory;
 
 import java.io.IOException;
 import java.util.Properties;
@@ -364,20 +359,9 @@ public void initialize() throws InitializationException {
         getIndexManager().initialize();
         getMediaFileManager().initialize();
 
-        // Turn off External DTD support in SAXParser to protect Roller from vulnerability.
-        SAXParserFactory spf = SAXParsers.getSAXParserFactory();
-        try {
-            spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-            spf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
-            spf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
-        } catch (ParserConfigurationException | SAXNotRecognizedException | SAXNotSupportedException e) {
-            String message = "Unable to turn off External DTD support in SAXParser. XML-RLC is vulnerable";
-            if ( log.isDebugEnabled() ) {
-                log.error(message, e);
-            } else {
-                log.error(message);
-            }
-        }
+        // XML-RPC requests are parsed with Roller's standard parser
+        // configuration. If it cannot be applied, startup fails.
+        SecureXmlParsers.installForXmlRpc();
 
         try {
             // Initialize ping systems

app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java

@@ -21,7 +21,6 @@
 import javax.xml.XMLConstants;
 
 import org.jdom2.input.SAXBuilder;
-import org.jdom2.input.sax.XMLReaders;
 
 /**
  * A {@link SAXBuilder} that treats a document strictly as data.
@@ -38,10 +37,12 @@
  * anything. Roller's own descriptors carry no document type declaration, so the
  * strict setting costs them nothing.
  *
- * <p>The settings overlap deliberately. Refusing the declaration outright is
- * what does the work; the remaining ones close the same door at the layers
- * beneath, so a parser configured elsewhere, or a JAXP implementation with
- * different defaults, does not quietly reopen it.
+ * <p>The underlying reader comes from {@link SecureXmlParsers}, which takes its
+ * parser from Apache Commons Secure XML and also refuses document type
+ * declarations. The settings below overlap with that deliberately. Refusing the
+ * declaration outright is what does the work; the remaining ones close the same
+ * door at the layers beneath, so a parser configured elsewhere, or a JAXP
+ * implementation with different defaults, does not quietly reopen it.
  */
 public class SafeSAXBuilder extends SAXBuilder {
 
@@ -54,7 +55,7 @@ public class SafeSAXBuilder extends SAXBuilder {
             "http://apache.org/xml/features/nonvalidating/load-external-dtd";
 
     public SafeSAXBuilder() {
-        super(XMLReaders.NONVALIDATING);
+        super(SecureXmlParsers.JDOM_READERS);
 
         // Secure processing is set explicitly rather than relied on. It is on
         // by default in current JDKs, but that default limits resource

app/src/main/java/org/apache/roller/weblogger/util/SecureXmlParsers.java

@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.  For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.util;
+
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.parsers.SAXParserFactory;
+
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
+import org.apache.xmlrpc.util.SAXParsers;
+import org.jdom2.JDOMException;
+import org.jdom2.input.sax.XMLReaderJDOMFactory;
+import org.xml.sax.SAXException;
+import org.xml.sax.XMLReader;
+
+/**
+ * The XML parser configuration Roller uses for every document it parses itself.
+ *
+ * <p>Parsers come from Apache Commons Secure XML, which configures the JAXP
+ * implementation so that nothing a document names is resolved, and throws
+ * rather than continuing if a setting cannot be applied. On top of that,
+ * Roller refuses any document type declaration: none of the documents Roller
+ * reads needs one.
+ */
+public final class SecureXmlParsers {
+
+    /** Xerces feature name, honoured by the JDK's own parser. */
+    static final String DISALLOW_DOCTYPE =
+            "http://apache.org/xml/features/disallow-doctype-decl";
+
+    /** Supplies JDOM with readers from {@link #newSAXParserFactory()}. */
+    public static final XMLReaderJDOMFactory JDOM_READERS = new XMLReaderJDOMFactory() {
+        @Override
+        public XMLReader createXMLReader() throws JDOMException {
+            try {
+                return newSAXParserFactory().newSAXParser().getXMLReader();
+            } catch (ParserConfigurationException | SAXException e) {
+                throw new JDOMException("Could not create an XML reader", e);
+            }
+        }
+
+        @Override
+        public boolean isValidating() {
+            return false;
+        }
+    };
+
+    private SecureXmlParsers() {
+    }
+
+    /**
+     * Returns a namespace-aware, non-validating SAX parser factory that
+     * resolves nothing a document names and refuses document type
+     * declarations.
+     *
+     * @throws IllegalStateException if the configuration cannot be applied
+     */
+    public static SAXParserFactory newSAXParserFactory() {
+        SAXParserFactory factory = SecureSAXParserFactory.newNSInstance();
+        factory.setValidating(false);
+        try {
+            factory.setFeature(DISALLOW_DOCTYPE, true);
+        } catch (ParserConfigurationException | SAXException e) {
+            throw new IllegalStateException(
+                    "The XML parser does not support refusing document type declarations", e);
+        }
+        return factory;
+    }
+
+    /**
+     * Makes the XML-RPC library parse requests with {@link #newSAXParserFactory()}.
+     *
+     * @throws IllegalStateException if the configuration cannot be applied
+     */
+    public static void installForXmlRpc() {
+        SAXParsers.setSAXParserFactory(newSAXParserFactory());
+    }
+}

app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java

@@ -18,12 +18,18 @@
 package org.apache.roller.weblogger.util;
 
 import java.io.StringReader;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
 
 import org.jdom2.Document;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 /**
@@ -53,4 +59,23 @@ public void anyDoctypeIsRefused() {
                 () -> new SafeSAXBuilder().build(new StringReader(withInternalSubset)),
                 "a document type declaration was accepted");
     }
-}
+
+    /** A declared external entity is refused, and its file is never read into the document. */
+    @Test
+    public void externalEntitiesAreNotRead(@TempDir Path dir) throws Exception {
+        Path marker = dir.resolve("marker.txt");
+        Files.write(marker, "MARKER-CONTENT".getBytes(StandardCharsets.UTF_8));
+        String withEntity = "<?xml version=\"1.0\"?>"
+                + "<!DOCTYPE opml [<!ENTITY m SYSTEM \"" + marker.toUri() + "\">]>"
+                + "<opml version=\"1.1\"><body><outline text=\"&m;\"/></body></opml>";
+        Exception e = assertThrows(Exception.class,
+                () -> new SafeSAXBuilder().build(new StringReader(withEntity)));
+        assertFalse(String.valueOf(e.getMessage()).contains("MARKER-CONTENT"));
+    }
+
+    /** Readers come from the shared parser configuration. */
+    @Test
+    public void readersComeFromTheSharedConfiguration() {
+        assertSame(SecureXmlParsers.JDOM_READERS, new SafeSAXBuilder().getXMLReaderFactory());
+    }
+}

app/src/test/java/org/apache/roller/weblogger/util/SecureXmlParsersTest.java

@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.  For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.util;
+
+import java.io.ByteArrayInputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import javax.xml.parsers.SAXParserFactory;
+
+import org.apache.commons.xml.secure.SecureSAXParserFactory;
+import org.apache.xmlrpc.util.SAXParsers;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.xml.sax.Attributes;
+import org.xml.sax.helpers.DefaultHandler;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class SecureXmlParsersTest {
+
+    private static final String NAMESPACED =
+            "<?xml version=\"1.0\"?><feed xmlns=\"http://www.w3.org/2005/Atom\"><title>t</title></feed>";
+
+    private static final String WITH_DOCTYPE =
+            "<?xml version=\"1.0\"?><!DOCTYPE feed [<!ELEMENT feed ANY>]><feed/>";
+
+    @Test
+    void factoryIsNamespaceAwareAndNonValidating() {
+        SAXParserFactory factory = SecureXmlParsers.newSAXParserFactory();
+        assertTrue(factory.isNamespaceAware());
+        assertFalse(factory.isValidating());
+    }
+
+    @Test
+    void ordinaryNamespacedDocumentsParse() throws Exception {
+        RootRecorder root = new RootRecorder();
+        SecureXmlParsers.newSAXParserFactory().newSAXParser().parse(bytes(NAMESPACED), root);
+        assertEquals("http://www.w3.org/2005/Atom", root.uri);
+        assertEquals("feed", root.localName);
+    }
+
+    @Test
+    void documentTypeDeclarationsAreRefused() {
+        assertThrows(Exception.class, () -> SecureXmlParsers.newSAXParserFactory()
+                .newSAXParser().parse(bytes(WITH_DOCTYPE), new DefaultHandler()));
+    }
+
+    /** The Commons layer on its own does not read a declared external entity. */
+    @Test
+    void commonsLayerDoesNotReadExternalEntities(@TempDir Path dir) throws Exception {
+        Path marker = dir.resolve("marker.txt");
+        Files.write(marker, "MARKER-CONTENT".getBytes(StandardCharsets.UTF_8));
+        String withEntity = "<?xml version=\"1.0\"?>"
+                + "<!DOCTYPE feed [<!ENTITY m SYSTEM \"" + marker.toUri() + "\">]>"
+                + "<feed>&m;</feed>";
+        StringBuilder text = new StringBuilder();
+        try {
+            SecureSAXParserFactory.newNSInstance().newSAXParser().parse(bytes(withEntity),
+                    new DefaultHandler() {
+                        @Override
+                        public void characters(char[] ch, int start, int length) {
+                            text.append(ch, start, length);
+                        }
+                    });
+        } catch (Exception refused) {
+            // Refusing the document outright is also acceptable.
+        }
+        assertFalse(text.toString().contains("MARKER-CONTENT"));
+    }
+
+    @Test
+    void xmlRpcUsesTheSameConfiguration() throws Exception {
+        SAXParserFactory previous = SAXParsers.getSAXParserFactory();
+        try {
+            SecureXmlParsers.installForXmlRpc();
+            SAXParserFactory installed = SAXParsers.getSAXParserFactory();
+            assertTrue(installed.isNamespaceAware());
+            assertThrows(Exception.class, () -> installed.newSAXParser()
+                    .parse(bytes(WITH_DOCTYPE), new DefaultHandler()));
+        } finally {
+            SAXParsers.setSAXParserFactory(previous);
+        }
+    }
+
+    private static ByteArrayInputStream bytes(String xml) {
+        return new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8));
+    }
+
+    private static final class RootRecorder extends DefaultHandler {
+        String uri;
+        String localName;
+
+        @Override
+        public void startElement(String uri, String localName, String qName, Attributes attributes) {
+            if (this.localName == null) {
+                this.uri = uri;
+                this.localName = localName;
+            }
+        }
+    }
+}