org.apache.xmlrpc
xmlrpc-common
diff --git a/app/src/main/java/org/apache/roller/weblogger/business/WebloggerImpl.java b/app/src/main/java/org/apache/roller/weblogger/business/WebloggerImpl.java
index 14e65a9f6..35eccfa55 100644
--- a/app/src/main/java/org/apache/roller/weblogger/business/WebloggerImpl.java
+++ b/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
diff --git a/app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java b/app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java
index 1d3848d73..97c3f3290 100644
--- a/app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java
+++ b/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.
*
- * 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.
+ *
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
diff --git a/app/src/main/java/org/apache/roller/weblogger/util/SecureXmlParsers.java b/app/src/main/java/org/apache/roller/weblogger/util/SecureXmlParsers.java
new file mode 100644
index 000000000..973d93c98
--- /dev/null
+++ b/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.
+ *
+ *
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());
+ }
+}
diff --git a/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java b/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java
index 4794b3bed..f69c06aa5 100644
--- a/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java
+++ b/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");
}
-}
\ No newline at end of file
+
+ /** 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 = ""
+ + "]>"
+ + "";
+ 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());
+ }
+}
diff --git a/app/src/test/java/org/apache/roller/weblogger/util/SecureXmlParsersTest.java b/app/src/test/java/org/apache/roller/weblogger/util/SecureXmlParsersTest.java
new file mode 100644
index 000000000..e55e3fdc1
--- /dev/null
+++ b/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 =
+ "t";
+
+ private static final String WITH_DOCTYPE =
+ "]>";
+
+ @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 = ""
+ + "]>"
+ + "&m;";
+ 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;
+ }
+ }
+ }
+}