#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. + * + *
Names are also held to the shapes a template can actually take: a stored + * template id, which carries no extension, or a Velocity template file. A name + * that asks for some other kind of file is not a template reference at all, and + * refusing it keeps the directives pointed at templates no matter what a loader + * further down happens to be able to resolve. + * + *
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) || isNotATemplateName(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.warn("Refusing #" + directiveName + " of '" + path
+ + "' from '" + currentResourcePath + "': outside the template namespace");
+ return null;
+ }
+
+ return path;
+ }
+
+ /**
+ * Stored templates are resolved by id and carry no extension; theme
+ * resources are Velocity templates. A name bearing any other extension is
+ * asking for something that is not a template.
+ *
+ * @return true when the name is not one of those two shapes
+ */
+ private boolean isNotATemplateName(String path) {
+ // Stored template ids arrive as | Weblog templates are authored by weblog administrators, a role Roller
+ * treats as untrusted and renders under 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");
+
+ assertFalse(render(dir, true, true).contains("REACHED"),
+ "the include handler still admitted a name that is not a template");
+ }
+
+ /**
+ * 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
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("src", "main", "webapp", "WEB-INF", "velocity.properties"),
+ Paths.get("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",
+ "",
+ " ",
+ // Not template names: a plain name needs no traversal to reach
+ // whatever a loader can resolve, so shape is checked too.
+ "secret.properties",
+ "web.xml",
+ "some/config.properties",
+ "weblog.vm.bak",
+ "notes.txt",
+ };
+ 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
+ "_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.
+ *
+ *