From 3610feedcabd9e4aeea1367fd332248d02cbd66a Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Fri, 2 Oct 2026 18:25:23 -0400 Subject: [PATCH 1/2] Adopt Apache Commons Secure XML for XML parsing Roller configured its XML parsers by hand in two places: SafeSAXBuilder for JDOM, and WebloggerImpl for the XML-RPC library. The WebloggerImpl block logged an error and carried on if the parser did not accept a setting. New SecureXmlParsers takes its SAXParserFactory from Apache Commons Secure XML 1.0.0, which throws if a setting cannot be applied, and refuses document type declarations on top, as Roller did before. SafeSAXBuilder gets its readers from it, and WebloggerImpl installs it for XML-RPC, so startup fails rather than continuing with an unconfigured parser. Behaviour for ordinary documents is unchanged; SafeSAXBuilder keeps its existing settings as an overlapping layer. --- app/pom.xml | 7 + .../weblogger/business/WebloggerImpl.java | 24 +--- .../roller/weblogger/util/SafeSAXBuilder.java | 13 +- .../weblogger/util/SecureXmlParsers.java | 94 ++++++++++++++ .../weblogger/util/SafeSAXBuilderTest.java | 27 +++- .../weblogger/util/SecureXmlParsersTest.java | 122 ++++++++++++++++++ 6 files changed, 260 insertions(+), 27 deletions(-) create mode 100644 app/src/main/java/org/apache/roller/weblogger/util/SecureXmlParsers.java create mode 100644 app/src/test/java/org/apache/roller/weblogger/util/SecureXmlParsersTest.java diff --git a/app/pom.xml b/app/pom.xml index 449c5ab5b..7eddcef14 100644 --- a/app/pom.xml +++ b/app/pom.xml @@ -46,6 +46,7 @@ limitations under the License. 1.22.1 1.15.0 3.20.0 + 1.0.0 4.0.9 7.0.0 2.26.1 @@ -375,6 +376,12 @@ limitations under the License. ${commons-lang3.version} + + org.apache.commons + commons-secure-xml + ${commons-secure-xml.version} + + 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; + } + } + } +} From 3e05daf175c641b6b37653e67e6eaf06fa947df8 Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Sun, 4 Oct 2026 08:22:40 -0400 Subject: [PATCH 2/2] Note the Commons Secure XML adoption in CHANGES.md --- CHANGES.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 2f4ad53f8..d6988febd 100644 --- a/CHANGES.md +++ b/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.