mraible commented on code in PR #172: URL: https://github.com/apache/roller/pull/172#discussion_r3891448431
########## app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java: ########## @@ -0,0 +1,215 @@ +/* + * 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. + * + * <p>Weblog templates are authored by weblog administrators, a role Roller + * treats as untrusted and renders under <code>SecureUberspector</code>. 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 = { Review Comment: None of these are the shape Roller's own macros pass to `#parse`. Add `basic:_day|standard` and `basic:basic-custom.css|standard` here; both fail against the current handler and would have caught the regression. ########## app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java: ########## @@ -0,0 +1,121 @@ +/* + * 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 org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.velocity.app.event.IncludeEventHandler; +import org.apache.velocity.context.Context; + +/** + * Keeps <code>#include</code> and <code>#parse</code> inside the template + * namespace they are rendered from. + * + * <p>Weblog templates are authored by weblog administrators, whom Roller treats + * as untrusted: the rendering engine runs them under + * <code>SecureUberspector</code> so they cannot reach arbitrary objects. That + * sandbox governs method calls, not resource resolution, so the include + * directives are confined here instead. + * + * <p>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. + * + * <p>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. + * + * <p>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 <template>|<deviceType>; the device + // type is a rendition selector, not part of the resource name. + String name = path; + int bar = name.indexOf('|'); + if (bar > -1) { + name = name.substring(0, bar); + } + + int dot = name.lastIndexOf('.'); + if (dot == -1) { + // No extension: a stored template id. + return false; + } + return !name.regionMatches(true, dot, ".vm", 0, 3) || dot != name.length() - 3; + } + + /** + * @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; + } + // A colon before any slash indicates a scheme or a Windows drive. + int colon = normalized.indexOf(':'); + if (colon > -1) { + int slash = normalized.indexOf('/'); + return slash == -1 || colon < slash; Review Comment: This refuses every shared-theme template id. `SharedThemeFromDir` builds ids as `themeId + ":" + templateName` (lines 270, 368) and `weblog.vm`'s `includeTemplate` macro does `#parse($pageArg.id + '|' + $model.deviceType)`, so the basic theme's weblog page asks for `basic:_day|standard`, gets null back, and renders nothing for that fragment. Every bundled theme is affected; only custom (UUID-id) templates pass. A scheme is followed by `/` (`file:///`, `http://`, `jar:file:`) or is a single drive letter, so something like `colon > 1 && (slash == -1 || colon < slash) && normalized.charAt(colon + 1) != '/'` is closer, and `ThemeResourceLoader` is the real authority on what a theme id looks like. ########## app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java: ########## @@ -0,0 +1,121 @@ +/* + * 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 org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.velocity.app.event.IncludeEventHandler; +import org.apache.velocity.context.Context; + +/** + * Keeps <code>#include</code> and <code>#parse</code> inside the template + * namespace they are rendered from. + * + * <p>Weblog templates are authored by weblog administrators, whom Roller treats + * as untrusted: the rendering engine runs them under + * <code>SecureUberspector</code> so they cannot reach arbitrary objects. That + * sandbox governs method calls, not resource resolution, so the include + * directives are confined here instead. + * + * <p>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. + * + * <p>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. + * + * <p>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 <template>|<deviceType>; the device + // type is a rendition selector, not part of the resource name. + String name = path; + int bar = name.indexOf('|'); + if (bar > -1) { + name = name.substring(0, bar); + } + + int dot = name.lastIndexOf('.'); + if (dot == -1) { + // No extension: a stored template id. + return false; + } + return !name.regionMatches(true, dot, ".vm", 0, 3) || dot != name.length() - 3; Review Comment: Shared-theme template names are free-form: the basic theme's stylesheet template is literally named `basic-custom.css`, and the template guide documents inlining it through `#includeTemplate`. This check strips `|standard`, sees `.css`, and refuses it. Since the resource loaders are already confined to the theme and webapp namespaces, I'd drop the extension check rather than try to enumerate template-name shapes. ########## app/src/test/resources/WEB-INF/velocity.properties: ########## @@ -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. Review Comment: The commit message says nothing reads this copy. Rather than keep two hand-maintained copies of security-relevant config plus a test that scans both, I'd delete this one. ########## app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java: ########## @@ -0,0 +1,215 @@ +/* + * 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. + * + * <p>Weblog templates are authored by weblog administrators, a role Roller + * treats as untrusted and renders under <code>SecureUberspector</code>. 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)"); Review Comment: cwd-relative, so this only runs from `app/`; surefire sets `project.build.directory` for this module (see `ApplicationResourcesTest`), which would let it run from an IDE rooted at the repo. ########## app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java: ########## @@ -0,0 +1,121 @@ +/* + * 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 org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.velocity.app.event.IncludeEventHandler; +import org.apache.velocity.context.Context; + +/** + * Keeps <code>#include</code> and <code>#parse</code> inside the template + * namespace they are rendered from. + * + * <p>Weblog templates are authored by weblog administrators, whom Roller treats + * as untrusted: the rendering engine runs them under + * <code>SecureUberspector</code> so they cannot reach arbitrary objects. That + * sandbox governs method calls, not resource resolution, so the include + * directives are confined here instead. + * + * <p>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. + * + * <p>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. + * + * <p>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 Review Comment: Once the id bug is fixed this is fine, but note it's unthrottled: any weblog admin can make the server log a WARN per request by leaving a refused include in a public template. Debug, or a once-per-template warning, would be safer. ########## 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 Review Comment: With the `class` loader gone, `RollerVelocity` (lines 66-67) still sets `resource.loader.class.cache` and `resource.loader.class.modification_check_interval` under `themes.reload.mode`; those two lines should go in the same change. ########## app/src/main/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeEventHandler.java: ########## @@ -0,0 +1,121 @@ +/* + * 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 org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.velocity.app.event.IncludeEventHandler; +import org.apache.velocity.context.Context; + +/** + * Keeps <code>#include</code> and <code>#parse</code> inside the template + * namespace they are rendered from. + * + * <p>Weblog templates are authored by weblog administrators, whom Roller treats + * as untrusted: the rendering engine runs them under + * <code>SecureUberspector</code> so they cannot reach arbitrary objects. That + * sandbox governs method calls, not resource resolution, so the include + * directives are confined here instead. + * + * <p>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. + * + * <p>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. + * + * <p>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(); Review Comment: Nit: `trim()` is computed twice, and returning the trimmed value means `#parse(" $pageId")` now resolves a different name than before. Compute it once and return the original unless trimming is intentional. ########## app/src/test/java/org/apache/roller/weblogger/ui/rendering/velocity/ThemeIncludeConfinementTest.java: ########## @@ -0,0 +1,215 @@ +/* + * 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. + * + * <p>Weblog templates are authored by weblog administrators, a role Roller + * treats as untrusted and renders under <code>SecureUberspector</code>. 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. + * + * <p>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"); Review Comment: Nit: the temp directory is never deleted; a `@TempDir` parameter does the cleanup. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
