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..2b7cf4775 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,8 @@ import org.apache.roller.weblogger.pojos.Weblog; import org.jdom2.Document; import org.jdom2.Element; -import org.jdom2.input.SAXBuilder; +import org.jdom2.input.JDOMParseException; +import org.apache.roller.weblogger.util.SafeSAXBuilder; /* * JPABookmarkManagerImpl.java @@ -142,7 +143,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 ); @@ -157,6 +158,10 @@ public void importBookmarks( for (Element elem : body.getChildren()) { importOpmlElement(elem, newFolder ); } + } catch (JDOMParseException ex) { + throw new WebloggerException( + "Unable to import bookmarks: XML document type declarations are not supported", + ex); } catch (Exception ex) { throw new WebloggerException(ex); } 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..1d3848d73 --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/util/SafeSAXBuilder.java @@ -0,0 +1,77 @@ +/* + * 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 org.jdom2.input.SAXBuilder; +import org.jdom2.input.sax.XMLReaders; + +/** + * 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_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"; + + public SafeSAXBuilder() { + super(XMLReaders.NONVALIDATING); + + // 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_PARAMETER_ENTITIES, false); + setFeature(LOAD_EXTERNAL_DTD, false); + + // JDOM maps this setting to the external-general-entities parser + // feature, so it supplies that part of the policy without duplicate + // configuration. + setExpandEntities(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..97386f3af --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/business/BookmarkImportParsingTest.java @@ -0,0 +1,134 @@ +/* + * 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.InputStream; +import java.nio.charset.StandardCharsets; + +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.jdom2.input.JDOMParseException; +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 assertDoctypeRejected(String opml) throws Exception { + Exception failure = null; + try { + bookmarkManager().importBookmarks( + TestUtils.getManagedWebsite(testWeblog), folderName, opml); + } catch (Exception expected) { + failure = expected; + } finally { + TestUtils.endSession(true); + } + assertTrue(failure instanceof org.apache.roller.weblogger.WebloggerException, + "DOCTYPE input must produce a WebloggerException: " + failure); + assertTrue(failure.getMessage().contains("document type declarations"), + "the import error should explain the rejected input: " + failure.getMessage()); + assertTrue(failure.getCause() instanceof JDOMParseException, + "the parse cause should be retained: " + failure.getCause()); + } + + /** Ordinary OPML, with no declarations in it, must still import. */ + @Test + public void ordinaryOpmlStillImports() throws Exception { + String opml; + try (InputStream in = getClass().getResourceAsStream("/bookmarks.opml")) { + assertTrue(in != null, "the ordinary OPML fixture must be on the classpath"); + opml = new String(in.readAllBytes(), StandardCharsets.UTF_8); + } + try { + bookmarkManager().importBookmarks(TestUtils.getManagedWebsite(testWeblog), + folderName, opml); + } finally { + 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" + + "" + + ""; + + assertDoctypeRejected(opml); + + assertTrue(importedBookmarks().isEmpty(), + "a document carrying a DOCTYPE was still imported"); + } +} 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