diff --git a/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java b/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java index 5b4224e09..9c4b85606 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPABookmarkManagerImpl.java @@ -34,7 +34,7 @@ import org.apache.roller.weblogger.pojos.Weblog; import org.jdom2.Document; import org.jdom2.Element; -import org.jdom2.input.SAXBuilder; +import org.apache.roller.weblogger.util.SafeSAXBuilder; /* * JPABookmarkManagerImpl.java @@ -142,7 +142,7 @@ public void importBookmarks( try { // Build JDOC document OPML string - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); StringReader reader = new StringReader( opml ); Document doc = builder.build( reader ); diff --git a/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java b/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java index bef2ca50a..0fb84f6a6 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeMetadataParser.java @@ -25,7 +25,7 @@ import org.jdom2.Document; import org.jdom2.Element; import org.jdom2.JDOMException; -import org.jdom2.input.SAXBuilder; +import org.apache.roller.weblogger.util.SafeSAXBuilder; import java.io.IOException; import java.io.InputStream; @@ -52,7 +52,7 @@ public ThemeMetadata unmarshall(InputStream instream) ThemeMetadata theme = new ThemeMetadata(); - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); Document doc = builder.build(instream); // start at root and get theme id, name, description and author diff --git a/app/src/main/java/org/apache/roller/weblogger/config/runtime/RuntimeConfigDefsParser.java b/app/src/main/java/org/apache/roller/weblogger/config/runtime/RuntimeConfigDefsParser.java index ad2b95e48..02b211948 100644 --- a/app/src/main/java/org/apache/roller/weblogger/config/runtime/RuntimeConfigDefsParser.java +++ b/app/src/main/java/org/apache/roller/weblogger/config/runtime/RuntimeConfigDefsParser.java @@ -29,7 +29,7 @@ import org.jdom2.Document; import org.jdom2.Element; import org.jdom2.JDOMException; -import org.jdom2.input.SAXBuilder; +import org.apache.roller.weblogger.util.SafeSAXBuilder; /** @@ -57,7 +57,7 @@ public RuntimeConfigDefs unmarshall(InputStream instream) RuntimeConfigDefs configs = new RuntimeConfigDefs(); - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); Document doc = builder.build(instream); Element root = doc.getRootElement(); diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/core/util/menu/MenuHelper.java b/app/src/main/java/org/apache/roller/weblogger/ui/core/util/menu/MenuHelper.java index cafe178c5..381e53cd2 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/core/util/menu/MenuHelper.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/core/util/menu/MenuHelper.java @@ -42,7 +42,7 @@ import org.jdom2.Document; import org.jdom2.Element; import org.jdom2.JDOMException; -import org.jdom2.input.SAXBuilder; +import org.apache.roller.weblogger.util.SafeSAXBuilder; /** * A helper class for dealing with UI menus. @@ -332,7 +332,7 @@ private static ParsedMenu unmarshall(String menuId, InputStream instream) ParsedMenu config = new ParsedMenu(); - SAXBuilder builder = new SAXBuilder(); + SafeSAXBuilder builder = new SafeSAXBuilder(); Document doc = builder.build(instream); Element root = doc.getRootElement(); 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 new file mode 100644 index 000000000..f8b1ca418 --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java @@ -0,0 +1,127 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. 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.XMLConstants; +import javax.xml.parsers.SAXParserFactory; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.jdom2.JDOMException; +import org.jdom2.input.SAXBuilder; +import org.jdom2.input.sax.XMLReaderJDOMFactory; +import org.xml.sax.XMLReader; + +/** + * A {@link SAXBuilder} that treats a document strictly as data. + * + *

An XML document can name resources for the parser to go and read: a + * document type declaration can point at an external subset, and entity + * declarations can point at files or URLs. Resolving those makes the parser act + * on behalf of whoever wrote the document, which is only appropriate when the + * document is Roller's own. + * + *

Roller parses documents from user input and from its own menu, theme and + * configuration descriptors alike. Rather than track which parser is on which + * side, every retained JDOM parser is built here, and none of them resolve + * 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. + */ +public class SafeSAXBuilder extends SAXBuilder { + + /** Xerces feature names, honoured by the JDK's own parser. */ + private static final String DISALLOW_DOCTYPE = + "http://apache.org/xml/features/disallow-doctype-decl"; + private static final String EXTERNAL_GENERAL_ENTITIES = + "http://xml.org/sax/features/external-general-entities"; + private static final String EXTERNAL_PARAMETER_ENTITIES = + "http://xml.org/sax/features/external-parameter-entities"; + private static final String LOAD_EXTERNAL_DTD = + "http://apache.org/xml/features/nonvalidating/load-external-dtd"; + + private static final Log LOG = LogFactory.getLog(SafeSAXBuilder.class); + + public SafeSAXBuilder() { + super(new HardenedReaders()); + + // Secure processing is set explicitly rather than relied on. It is on + // by default in current JDKs, but that default limits resource + // consumption; it does not by itself stop external resolution. + setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + + // A document that declares a doctype is refused. Everything an entity + // could name has to be declared first, so this is the setting the rest + // stand behind. + setFeature(DISALLOW_DOCTYPE, true); + + setFeature(EXTERNAL_GENERAL_ENTITIES, false); + setFeature(EXTERNAL_PARAMETER_ENTITIES, false); + setFeature(LOAD_EXTERNAL_DTD, false); + + setExpandEntities(false); + } + + /** + * Supplies the reader, so that the two access properties can be applied + * where a parser that does not recognise them can be tolerated. + * + *

They are JAXP properties rather than SAX ones, and Roller ships its + * own Xerces, which rejects them outright at the SAX layer. Setting them + * through the builder would therefore fail every parse. They are still + * worth setting where they are understood, because they deny the protocols + * outright, so they are applied here and a rejection is logged and passed + * over — the features above are what carry the guarantee. + */ + private static final class HardenedReaders implements XMLReaderJDOMFactory { + + @Override + public XMLReader createXMLReader() throws JDOMException { + try { + SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + factory.setValidating(false); + XMLReader reader = factory.newSAXParser().getXMLReader(); + denyProtocol(reader, XMLConstants.ACCESS_EXTERNAL_DTD); + denyProtocol(reader, XMLConstants.ACCESS_EXTERNAL_SCHEMA); + return reader; + } catch (Exception ex) { + throw new JDOMException("Unable to create an XML reader", ex); + } + } + + private void denyProtocol(XMLReader reader, String property) { + try { + reader.setProperty(property, ""); + } catch (Exception unsupported) { + LOG.debug("XML reader does not recognise " + property + + "; the parser features are what constrain resolution", unsupported); + } + } + + @Override + public boolean isValidating() { + return false; + } + } +} diff --git a/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java b/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java new file mode 100644 index 000000000..4b43d2db8 --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java @@ -0,0 +1,122 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. 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.business; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.roller.weblogger.TestUtils; +import org.apache.roller.weblogger.pojos.User; +import org.apache.roller.weblogger.pojos.Weblog; +import org.apache.roller.weblogger.pojos.WeblogBookmark; +import org.apache.roller.weblogger.pojos.WeblogBookmarkFolder; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers the OPML bookmark import's handling of document type declarations: + * documents that carry one are refused, while ordinary OPML still imports. + */ +public class BookmarkImportParsingTest { + + private static final Log log = LogFactory.getLog(BookmarkImportParsingTest.class); + + private User testUser = null; + private Weblog testWeblog = null; + private final String folderName = "ZZZ_import_parsing_ZZZ"; + + @BeforeEach + public void setUp() throws Exception { + TestUtils.setupWeblogger(); + testUser = TestUtils.setupUser("importParsingTestUser"); + testWeblog = TestUtils.setupWeblog("importParsingTestWeblog", testUser); + TestUtils.endSession(true); + } + + @AfterEach + public void tearDown() throws Exception { + try { + TestUtils.teardownWeblog(testWeblog.getId()); + TestUtils.teardownUser(testUser.getUserName()); + TestUtils.endSession(true); + } catch (Exception ex) { + log.error("ERROR in tearDown", ex); + } + } + + private BookmarkManager bookmarkManager() { + return WebloggerFactory.getWeblogger().getBookmarkManager(); + } + + /** @return the bookmarks imported into the test folder, empty if none */ + private java.util.List importedBookmarks() throws Exception { + testWeblog = TestUtils.getManagedWebsite(testWeblog); + WeblogBookmarkFolder folder = bookmarkManager().getFolder(testWeblog, folderName); + if (folder == null) { + return java.util.Collections.emptyList(); + } + return folder.retrieveBookmarks(); + } + + private void tryImport(String opml) { + try { + bookmarkManager().importBookmarks( + TestUtils.getManagedWebsite(testWeblog), folderName, opml); + TestUtils.endSession(true); + } catch (Exception expected) { + // A refusal to parse is one acceptable outcome; the assertions in + // each test say what must be true either way. + log.debug("import raised: " + expected); + } + } + + /** Ordinary OPML, with no declarations in it, must still import. */ + @Test + public void ordinaryOpmlStillImports() throws Exception { + byte[] opml = Files.readAllBytes( + new File("src/test/resources/bookmarks.opml").toPath()); + bookmarkManager().importBookmarks(TestUtils.getManagedWebsite(testWeblog), + folderName, new String(opml, StandardCharsets.UTF_8)); + TestUtils.endSession(true); + + assertFalse(importedBookmarks().isEmpty(), + "ordinary OPML no longer imports any bookmarks"); + } + + /** The rejection must not depend on where the DOCTYPE points. */ + @Test + public void aDoctypeAloneIsEnoughToBeRefused() throws Exception { + String opml = "" + + "]>" + + "t" + + "" + + ""; + + tryImport(opml); + + assertTrue(importedBookmarks().isEmpty(), + "a document carrying a DOCTYPE was still imported"); + } +} \ No newline at end of file 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 new file mode 100644 index 000000000..4794b3bed --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/util/SafeSAXBuilderTest.java @@ -0,0 +1,56 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. 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.StringReader; + +import org.jdom2.Document; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * The parser contract, checked directly rather than through a caller. + */ +public class SafeSAXBuilderTest { + + private static final String ORDINARY = + "t" + + ""; + + /** Ordinary XML, carrying no declarations, still parses. */ + @Test + public void ordinaryDocumentsStillParse() throws Exception { + Document doc = new SafeSAXBuilder().build(new StringReader(ORDINARY)); + assertNotNull(doc.getRootElement()); + assertEquals("opml", doc.getRootElement().getName()); + } + + /** Any document type declaration is refused, whatever it points at. */ + @Test + public void anyDoctypeIsRefused() { + String withInternalSubset = "" + + "]>" + + ""; + assertThrows(Exception.class, + () -> new SafeSAXBuilder().build(new StringReader(withInternalSubset)), + "a document type declaration was accepted"); + } +} \ No newline at end of file