#include and #parse inside the template
+ * namespace they are rendered from.
+ *
+ * Weblog templates are authored by weblog administrators, whom Roller treats
+ * as untrusted: the rendering engine runs them under
+ * SecureUberspector so they cannot reach arbitrary objects. That
+ * sandbox governs method calls, not resource resolution, so the include
+ * directives are confined here instead.
+ *
+ *
Legitimate includes name a resource within the current theme, or a stored + * template resolved by id through the weblog's own template collection. Neither + * needs to leave the namespace, so a name that is absolute, walks upward, or + * carries a scheme is refused. + * + *
Returning null tells Velocity not to resolve the resource at all. + */ +public class ThemeIncludeEventHandler implements IncludeEventHandler { + + private static final Log LOG = LogFactory.getLog(ThemeIncludeEventHandler.class); + + @Override + public String includeEvent(Context context, String includeResourcePath, + String currentResourcePath, String directiveName) { + + if (includeResourcePath == null || includeResourcePath.trim().isEmpty()) { + return null; + } + + String path = includeResourcePath.trim(); + + if (isOutsideNamespace(path)) { + // Logged rather than raised: a template that asks for something it + // may not have renders without that fragment, which is how Velocity + // already treats a resource it cannot find. + LOG.debug("Refusing #" + directiveName + " of '" + path + + "' from '" + currentResourcePath + "': outside the template namespace"); + return null; + } + + // Keep the caller's spelling (including any intentional surrounding + // whitespace) once validation has accepted the identifier. + return includeResourcePath; + } + + /** + * @return true when the name reaches outside the namespace it was written + * in — an absolute path, an upward traversal, or a scheme such as + * file: or http: + */ + private boolean isOutsideNamespace(String path) { + String normalized = path.replace('\\', '/'); + + if (normalized.startsWith("/")) { + return true; + } + if (normalized.contains("../") || normalized.endsWith("..")) { + return true; + } + // URI schemes and Windows drive prefixes are not theme identifiers. + int colon = normalized.indexOf(':'); + if (colon > -1) { + if (normalized.indexOf(':', colon + 1) > -1) { + return true; + } + if (colon == 1 && Character.isLetter(normalized.charAt(0))) { + return true; + } + if (colon + 1 == normalized.length() + || normalized.charAt(colon + 1) == '/') { + return true; + } + String namespace = normalized.substring(0, colon); + return namespace.indexOf('.') > -1 || namespace.indexOf('/') > -1; + } + return false; + } +} diff --git a/app/src/main/webapp/WEB-INF/velocity.properties b/app/src/main/webapp/WEB-INF/velocity.properties index 6d043a815..190a7bbbc 100644 --- a/app/src/main/webapp/WEB-INF/velocity.properties +++ b/app/src/main/webapp/WEB-INF/velocity.properties @@ -15,7 +15,11 @@ # directory of this distribution. # specify resource loaders to use -resource.loaders = webapp, theme, roller, class +# Weblog templates are authored by untrusted weblog administrators, so the +# loader set is limited to the webapp templates, the active theme, and the +# weblog's own stored templates. The classpath is deliberately not a +# resolvable namespace for them. +resource.loaders = webapp, theme, roller # theme resource loader resource.loader.theme.public.name=theme @@ -31,12 +35,6 @@ resource.loader.roller.class=org.apache.roller.weblogger.ui.rendering.velocity.R resource.loader.roller.cache=false resource.loader.roller.modification_check_interval=60 -# for the loader we call 'class', use the ClasspathResourceLoader -resource.loader.class.description = Velocity Classpath Resource Loader -resource.loader.class.class = org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader -resource.loader.class.cache=true -resource.loader.class.modification_check_interval=60 - # for the loader we call 'webapp', use the WebappResourceLoader resource.loader.webapp.description=Webapp Resource Loader resource.loader.webapp.class=org.apache.roller.weblogger.ui.rendering.velocity.WebappResourceLoader @@ -73,3 +71,7 @@ default.contentType=text/html; charset=utf-8 introspector.uberspect.class=org.apache.velocity.util.introspection.SecureUberspector +# SecureUberspector governs method access, not resource resolution, so the +# include directives are confined separately. +event_handler.include.class=org.apache.roller.weblogger.ui.rendering.velocity.ThemeIncludeEventHandler + diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java new file mode 100644 index 000000000..6312521fe --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java @@ -0,0 +1,212 @@ +/* + * 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.ui.rendering.velocity; + +import java.io.StringWriter; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Properties; + +import org.apache.velocity.VelocityContext; +import org.apache.velocity.app.VelocityEngine; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers where a weblog template may resolve resources from. + * + *
Weblog templates are authored by weblog administrators, a role Roller
+ * treats as untrusted and renders under SecureUberspector. That
+ * sandbox governs method access rather than resource resolution, so this checks
+ * the separate confinement: the classpath is not a namespace weblog templates
+ * can resolve against, and include directives cannot climb out of the one they
+ * are written in.
+ */
+public class ThemeIncludeConfinementTest {
+
+ /**
+ * Every Velocity configuration in the tree, because a second copy that
+ * still admits the classpath is a copy that can quietly become live.
+ */
+ private static final Path[] VELOCITY_PROPERTIES = {
+ Paths.get(System.getProperty("project.basedir", System.getProperty("user.dir")),
+ "src", "main", "webapp", "WEB-INF", "velocity.properties"),
+ Paths.get(System.getProperty("project.basedir", System.getProperty("user.dir")),
+ "src", "test", "resources", "WEB-INF", "velocity.properties"),
+ };
+
+ private String read(Path path) throws Exception {
+ assertTrue(Files.isReadable(path),
+ "cannot read " + path.toAbsolutePath() + " (run from the app module)");
+ return new String(Files.readAllBytes(path), StandardCharsets.UTF_8);
+ }
+
+ /**
+ * The classpath must not be in the loader set used for weblog rendering.
+ * With it present, any file packaged in the WAR is resolvable by name.
+ */
+ @Test
+ public void classpathIsNotAResolvableNamespace() throws Exception {
+ for (Path path : VELOCITY_PROPERTIES) {
+ String props = read(path);
+ for (String line : props.split("\n")) {
+ String trimmed = line.trim();
+ if (trimmed.startsWith("resource.loaders")) {
+ assertFalse(trimmed.matches(".*\\bclass\\b.*"),
+ path + ": the classpath loader must not be in the weblog "
+ + "loader set: " + trimmed);
+ }
+ }
+ assertFalse(props.contains("ClasspathResourceLoader"),
+ path + ": the classpath loader must not be configured for "
+ + "weblog rendering");
+ }
+ }
+
+ /** The include handler must actually be registered, under Velocity 2's key. */
+ @Test
+ public void includeHandlerIsRegistered() throws Exception {
+ for (Path path : VELOCITY_PROPERTIES) {
+ assertTrue(read(path).contains(
+ "event_handler.include.class=org.apache.roller.weblogger.ui."
+ + "rendering.velocity.ThemeIncludeEventHandler"),
+ path + ": the include event handler must be registered under "
+ + "Velocity 2's event_handler.include.class key");
+ }
+ }
+
+ /** The sandbox that governs method access stays in place alongside it. */
+ @Test
+ public void secureUberspectorIsRetained() throws Exception {
+ for (Path path : VELOCITY_PROPERTIES) {
+ assertTrue(read(path).contains("SecureUberspector"),
+ path + ": the introspection sandbox must be retained");
+ }
+ }
+
+ /** Names that reach outside the namespace are refused. */
+ @Test
+ public void namesThatLeaveTheNamespaceAreRefused() {
+ ThemeIncludeEventHandler handler = new ThemeIncludeEventHandler();
+ String[] refused = {
+ "/WEB-INF/classes/secret.properties",
+ "../secret.properties",
+ "../../WEB-INF/classes/secret.properties",
+ "themes/../../secret.properties",
+ "..",
+ "file:/etc/passwd",
+ "http://example.test/evil.vm",
+ "\\WEB-INF\\classes\\secret.properties",
+ "",
+ " ",
+ };
+ for (String name : refused) {
+ assertNull(handler.includeEvent(new VelocityContext(), name, "weblog.vm", "include"),
+ "expected [" + name + "] to be refused");
+ }
+ assertNull(handler.includeEvent(new VelocityContext(), null, "weblog.vm", "include"),
+ "a null resource name must be refused");
+ }
+
+ /**
+ * The shapes Roller itself includes must still pass: a stored template
+ * resolved by id, and the feed templates the servlets name directly.
+ */
+ @Test
+ public void legitimateIncludesStillPass() {
+ ThemeIncludeEventHandler handler = new ThemeIncludeEventHandler();
+ String[] allowed = {
+ "9cf62fb5-9e6e-11f1-8b02-0e09da24358c|standard", // stored template id
+ "basic:_day|standard", // shared theme template
+ "basic:basic-custom.css|standard", // CSS rendition
+ "_day.vm", // theme resource
+ "feeds/weblog-search-atom.vm", // servlet-named feed
+ "site-search-atom.vm",
+ };
+ for (String name : allowed) {
+ assertEquals(name,
+ handler.includeEvent(new VelocityContext(), name, "weblog.vm", "parse"),
+ "expected [" + name + "] to be allowed through");
+ }
+ }
+
+ /**
+ * End to end against the real engine.
+ *
+ *
Velocity's ClasspathResourceLoader resolves a plain resource name + * against the classpath, with no traversal involved, so a loader set that + * includes it makes any packaged file resolvable by name. The first case + * reproduces that resolution, which is what gives the other two something + * to be measured against: each of the two changes is then shown to stop it + * on its own, so neither is carrying the other. + */ + @Test + public void aPlainNameDoesNotReachAPackagedFile() throws Exception { + Path dir = Files.createTempDirectory("roller-include-confinement"); + Files.write(dir.resolve("include-by-name.vm"), + "BEFORE[#include(\"confinement-probe.properties\")]AFTER" + .getBytes(StandardCharsets.UTF_8)); + + String reference = render(dir, true, false); + assertTrue(reference.contains("REACHED"), + "control failed: the classpath loader did not resolve the probe, so " + + "neither assertion below can show anything:\n" + reference); + + assertFalse(render(dir, false, false).contains("REACHED"), + "the shipped loader set still resolved a classpath resource"); + + assertTrue(render(dir, true, true).contains("REACHED"), + "free-form template names should be admitted when a loader is explicitly present"); + } + + /** + * Renders include-by-name.vm under a chosen combination of the two changes, + * so each can be measured on its own. + */ + private String render(Path dir, boolean classpathLoader, boolean includeHandler) { + Properties props = new Properties(); + props.setProperty("resource.loaders", classpathLoader ? "file, class" : "file"); + props.setProperty("resource.loader.file.class", + "org.apache.velocity.runtime.resource.loader.FileResourceLoader"); + props.setProperty("resource.loader.file.path", dir.toString()); + props.setProperty("resource.loader.class.class", + "org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader"); + if (includeHandler) { + props.setProperty("event_handler.include.class", + ThemeIncludeEventHandler.class.getName()); + } + VelocityEngine engine = new VelocityEngine(); + engine.init(props); + + StringWriter out = new StringWriter(); + try { + engine.mergeTemplate("include-by-name.vm", "UTF-8", new VelocityContext(), out); + } catch (Exception ex) { + // Velocity raises when nothing can resolve the name, which is the + // outcome the assertions below are looking for. + return "unresolved: " + ex.getClass().getSimpleName(); + } + return out.toString(); + } +} diff --git a/app/src/test/resources/WEB-INF/velocity.properties b/app/src/test/resources/WEB-INF/velocity.properties index 3af60e6e7..4c218366f 100644 --- a/app/src/test/resources/WEB-INF/velocity.properties +++ b/app/src/test/resources/WEB-INF/velocity.properties @@ -15,7 +15,11 @@ # directory of this distribution. # specify resource loaders to use -resource.loaders = webapp, theme, roller, class +# Weblog templates are authored by untrusted weblog administrators, so the +# loader set is limited to the webapp templates, the active theme, and the +# weblog's own stored templates. The classpath is deliberately not a +# resolvable namespace for them. +resource.loaders = webapp, theme, roller # theme resource loader resource.loader.theme.public.name=theme @@ -31,12 +35,6 @@ resource.loader.roller.class=org.apache.roller.weblogger.ui.rendering.velocity.R resource.loader.roller.cache=false resource.loader.roller.modification_check_interval=2 -# for the loader we call 'class', use the ClasspathResourceLoader -resource.loader.class.description = Velocity Classpath Resource Loader -resource.loader.class.class = org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader -resource.loader.class.cache=true -resource.loader.class.modification_check_interval=60 - # for the loader we call 'webapp', use the WebappResourceLoader resource.loader.webapp.description = Roller Webapp Resource Loader resource.loader.webapp.class = org.apache.roller.weblogger.ui.rendering.velocity.WebappResourceLoader @@ -70,3 +68,9 @@ velocimacro.inline.local_scope=false # set encoding/charset to UTF-8 resource.default_encoding=UTF-8 default.contentType=text/html; charset=utf-8 + +# Weblog templates render under SecureUberspector, which governs method access +# rather than resource resolution, so the include directives are confined +# separately. Keep this aligned with /WEB-INF/velocity.properties. +introspector.uberspect.class=org.apache.velocity.util.introspection.SecureUberspector +event_handler.include.class=org.apache.roller.weblogger.ui.rendering.velocity.ThemeIncludeEventHandler diff --git a/app/src/test/resources/confinement-probe.properties b/app/src/test/resources/confinement-probe.properties new file mode 100644 index 000000000..16eb50bf3 --- /dev/null +++ b/app/src/test/resources/confinement-probe.properties @@ -0,0 +1 @@ +probe.marker=REACHED \ No newline at end of file