diff --git a/dotCMS/src/main/java/org/apache/velocity/util/introspection/SecureIntrospectorImpl.java b/dotCMS/src/main/java/org/apache/velocity/util/introspection/SecureIntrospectorImpl.java index 2063b57d6d16..19ca9c96e5b3 100644 --- a/dotCMS/src/main/java/org/apache/velocity/util/introspection/SecureIntrospectorImpl.java +++ b/dotCMS/src/main/java/org/apache/velocity/util/introspection/SecureIntrospectorImpl.java @@ -19,8 +19,23 @@ * under the License. */ +import java.io.File; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.RandomAccessFile; +import java.io.Reader; +import java.io.Writer; import java.lang.reflect.Method; +import java.net.URI; +import java.net.URL; +import java.nio.channels.Channel; +import java.nio.file.FileSystem; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.spi.FileSystemProvider; +import com.dotmarketing.util.Config; import com.dotmarketing.util.Logger; /** @@ -59,6 +74,7 @@ public SecureIntrospectorImpl(String[] badClasses, String[] badPackages) * @return Method object retrieved by Introspector * @throws IllegalArgumentException The parameter passed in were incorrect. */ + @Override public Method getMethod(Class clazz, String methodName, Object[] params) throws IllegalArgumentException { @@ -86,6 +102,7 @@ public Method getMethod(Class clazz, String methodName, Object[] params) * @param methodName Name of method to be called * @see org.apache.velocity.util.introspection.SecureIntrospectorControl#checkObjectExecutePermission(java.lang.Class, java.lang.String) */ + @Override public boolean checkObjectExecutePermission(Class clazz, String methodName) { /** @@ -136,6 +153,18 @@ else if (Class.class.isAssignableFrom(clazz) && return false; } + /** + * Extend restricted-class coverage to the file, IO and network-resource type families. + * These are matched by type hierarchy so that concrete platform implementations (for + * example the JDK's internal Path implementation) are covered as well, which an + * exact-string class/package list cannot reach. The static utility holders Files and + * Paths are final and have no restricted supertype, so they are matched by identity. + */ + if (isRestrictedResourceType(clazz)) + { + return false; + } + /** @@ -167,6 +196,138 @@ else if (Class.class.isAssignableFrom(clazz) && } } + /** + * Operator-configurable restricted packages/classes (additive to the code-level floor + * above). This lets deployments push the sandbox toward a stricter, package-level stance + * as new holes are found, without a code change — for example + * {@code DOT_VELOCITY_INTROSPECTOR_RESTRICT_PACKAGES=java.nio,java.net}. An allow-list + * (ALLOW_CLASSES_PROP) carves specific classes back out of that configurable denial. + * Denials are logged by getMethod(). Defaults are empty, so behavior is unchanged unless + * configured. + */ + if (isConfigRestricted(className, packageName)) + { + return false; + } + return true; } + + /** dotCMS Config key (env: DOT_VELOCITY_INTROSPECTOR_RESTRICT_PACKAGES): extra restricted package prefixes. */ + public static final String RESTRICT_PACKAGES_PROP = "velocity.introspector.restrict.packages"; + + /** dotCMS Config key (env: DOT_VELOCITY_INTROSPECTOR_RESTRICT_CLASSES): extra restricted class names. */ + public static final String RESTRICT_CLASSES_PROP = "velocity.introspector.restrict.classes"; + + /** dotCMS Config key (env: DOT_VELOCITY_INTROSPECTOR_ALLOW_CLASSES): classes carved out of the configurable denial. */ + public static final String ALLOW_CLASSES_PROP = "velocity.introspector.allow.classes"; + + private static final String[] EMPTY = new String[0]; + + /** + * Reads a comma-separated dotCMS Config list, tolerating an uninitialized Config so this + * vendored introspector stays usable outside a running dotCMS (e.g. plain unit tests). + * + * @param prop the Config/env property name + * @return the configured values, or an empty array if unset or unavailable + */ + private static String[] configList(final String prop) + { + try + { + final String[] values = Config.getStringArrayProperty(prop, EMPTY); + return values == null ? EMPTY : values; + } + catch (final Throwable t) + { + return EMPTY; + } + } + + /** + * Determines whether the given class is denied by the operator-configurable layer. An + * explicit allow-list entry wins, so a broadened package can still exempt specific classes. + * This layer never overrides the code-level floor or the reflection/system denials above. + * + * @param className fully-qualified class name (array markers already stripped) + * @param packageName package of the class ("" for the default package) + * @return {@code true} if the configurable layer denies the class, {@code false} otherwise + */ + private static boolean isConfigRestricted(final String className, final String packageName) + { + for (final String allowed : configList(ALLOW_CLASSES_PROP)) + { + if (className.equals(allowed.trim())) + { + return false; + } + } + + for (final String badClass : configList(RESTRICT_CLASSES_PROP)) + { + if (className.equals(badClass.trim())) + { + return true; + } + } + + for (final String badPackage : configList(RESTRICT_PACKAGES_PROP)) + { + final String prefix = badPackage.trim(); + if (!prefix.isEmpty() + && (packageName.equals(prefix) || packageName.startsWith(prefix + "."))) + { + return true; + } + } + + return false; + } + + /** + * The supertypes whose method calls are restricted. Matching by hierarchy means every + * concrete subtype and implementation is covered without enumerating platform-internal + * class names. + */ + private static final Class[] RESTRICTED_SUPERTYPES = { + File.class, Path.class, RandomAccessFile.class, InputStream.class, + OutputStream.class, Reader.class, Writer.class, Channel.class, + FileSystem.class, FileSystemProvider.class, URL.class, URI.class + }; + + /** + * The final utility holders that expose file-system operations but have no restricted + * supertype; matched by identity. + */ + private static final Class[] RESTRICTED_EXACT_TYPES = { + Files.class, Paths.class + }; + + /** + * Determines whether method execution on the given class is restricted because it belongs to + * the file, IO or network-resource type families. + * + * @param clazz class a method is about to be resolved on + * @return {@code true} if the class is a restricted resource type (deny), {@code false} otherwise + */ + private static boolean isRestrictedResourceType(final Class clazz) + { + for (final Class supertype : RESTRICTED_SUPERTYPES) + { + if (supertype.isAssignableFrom(clazz)) + { + return true; + } + } + + for (final Class exact : RESTRICTED_EXACT_TYPES) + { + if (exact == clazz) + { + return true; + } + } + + return false; + } } diff --git a/dotCMS/src/main/resources/org/apache/velocity/runtime/defaults/velocity.properties b/dotCMS/src/main/resources/org/apache/velocity/runtime/defaults/velocity.properties index 83520c86c09a..a2f54d7d7501 100644 --- a/dotCMS/src/main/resources/org/apache/velocity/runtime/defaults/velocity.properties +++ b/dotCMS/src/main/resources/org/apache/velocity/runtime/defaults/velocity.properties @@ -278,3 +278,25 @@ introspector.restrict.classes = javax.management.MBeanServer introspector.restrict.classes = java.net.Socket introspector.restrict.classes = javax.script.ScriptEngine introspector.restrict.classes = javax.script.ScriptEngineManager + +# File / network-resource types. These document the restricted-class intent alongside the +# entries above; note that this exact-string list cannot match concrete platform +# implementations (e.g. the JDK's internal Path type), so the hierarchy-aware check in +# SecureIntrospectorImpl is the actual enforcement floor for these families. +introspector.restrict.classes = java.net.URL +introspector.restrict.classes = java.net.URI +introspector.restrict.classes = java.nio.file.Files +introspector.restrict.classes = java.nio.file.Paths + +# These velocity.properties lists are loaded once at engine init and matched by exact string. +# For runtime- and environment-configurable coverage that can push toward a stricter, +# package-level (prefix) stance without a code change, use the dotCMS Config properties +# (env-overridable) read by SecureIntrospectorImpl, additive to its code-level type-hierarchy +# floor: +# velocity.introspector.restrict.packages (env DOT_VELOCITY_INTROSPECTOR_RESTRICT_PACKAGES) +# comma-separated package PREFIXES to deny, e.g. "java.nio,java.net" +# velocity.introspector.restrict.classes (env DOT_VELOCITY_INTROSPECTOR_RESTRICT_CLASSES) +# comma-separated exact class names to deny +# velocity.introspector.allow.classes (env DOT_VELOCITY_INTROSPECTOR_ALLOW_CLASSES) +# comma-separated exact class names carved back out of the configurable denial +# Defaults are empty (behavior unchanged); denials are logged by the introspector. diff --git a/dotCMS/src/test/java/com/dotcms/security/SecureIntrospectorImplTest.java b/dotCMS/src/test/java/com/dotcms/security/SecureIntrospectorImplTest.java new file mode 100644 index 000000000000..be48b7e14839 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/security/SecureIntrospectorImplTest.java @@ -0,0 +1,205 @@ +package com.dotcms.security; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.Reader; +import java.io.Writer; +import java.net.URI; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.HashMap; +import com.dotmarketing.util.Config; +import org.apache.velocity.util.introspection.SecureIntrospectorImpl; +import org.junit.Test; + +/** + * Contract test for the Velocity {@link SecureIntrospectorImpl} restricted-class coverage. + * + *

The introspector gates every method call a Velocity template may perform. It is + * constructed with the same restricted class/package lists that ship in + * {@code org/apache/velocity/runtime/defaults/velocity.properties}. Those lists are + * exact-string matched, so the code-level check must independently deny method access on + * the file, IO, and network-resource type families (including their concrete platform + * implementations, reached by type hierarchy) that the string lists do not enumerate. + * + *

This test pins that contract: the file/IO/network-resource families are denied, the + * reflection/system families stay denied, and the common value types stay allowed. It is a + * plain unit test (no Velocity engine or services required) mirroring the sibling + * {@code SstiGetUrlReproTest}. + */ +public class SecureIntrospectorImplTest { + + /** + * The class list exactly as shipped in velocity.properties today. It intentionally does + * NOT enumerate the file/IO/network-resource families — proving the denial below comes + * from the code-level (hierarchy-aware) check, not from these strings. + */ + private static final String[] SHIPPED_BAD_CLASSES = { + "java.lang.Class", "java.lang.ClassLoader", + "org.apache.velocity.app.VelocityEngine", "org.apache.velocity.runtime.RuntimeInstance", + "org.apache.velocity.util.introspection.SecureUberspector", + "org.apache.velocity.util.introspection.SecureUberspectorImpl", + "java.lang.Compiler", "java.lang.InheritableThreadLocal", "java.lang.Package", + "java.lang.Process", "java.lang.Runtime", "java.lang.RuntimePermission", + "java.lang.SecurityManager", "java.lang.System", "java.lang.Thread", + "java.lang.ThreadGroup", "java.lang.ThreadLocal", "java.lang.ProcessBuilder", + "java.lang.Reflect", "javax.management.MBeanServer", "java.net.Socket", + "javax.script.ScriptEngine", "javax.script.ScriptEngineManager" + }; + + private static final String[] SHIPPED_BAD_PACKAGES = { "java.lang.reflect" }; + + private SecureIntrospectorImpl introspector() { + return new SecureIntrospectorImpl(SHIPPED_BAD_CLASSES, SHIPPED_BAD_PACKAGES); + } + + private void assertDenied(final Class clazz, final String method) { + assertFalse("Introspector must deny " + clazz.getName() + "#" + method, + introspector().checkObjectExecutePermission(clazz, method)); + } + + private void assertAllowed(final Class clazz, final String method) { + assertTrue("Introspector must allow " + clazz.getName() + "#" + method, + introspector().checkObjectExecutePermission(clazz, method)); + } + + /** + * The file, IO, and network-resource type families must be denied — including a concrete + * {@link Path} implementation resolved from the platform file system, which the exact-string + * lists never name and only a hierarchy-aware check can cover. + */ + @Test + public void file_io_and_network_resource_families_are_denied() { + assertDenied(File.class, "toPath"); + assertDenied(File.class, "getCanonicalPath"); + + assertDenied(Path.class, "toFile"); + // Concrete platform Path implementation (e.g. sun.nio.fs.UnixPath) — reached only via hierarchy. + final Class concretePath = Paths.get("x").getClass(); + assertDenied(concretePath, "toFile"); + + assertDenied(java.io.RandomAccessFile.class, "readFully"); + assertDenied(java.io.RandomAccessFile.class, "write"); + + assertDenied(InputStream.class, "readAllBytes"); + assertDenied(OutputStream.class, "write"); + assertDenied(Reader.class, "read"); + assertDenied(Writer.class, "write"); + + assertDenied(Files.class, "readAllBytes"); + assertDenied(Paths.class, "get"); + + assertDenied(URL.class, "openStream"); + assertDenied(URI.class, "toURL"); + } + + /** + * Regression guard: the reflection/system families that were already restricted must stay + * denied (this change only extends coverage; it must not weaken the existing sandbox). + */ + @Test + public void reflection_and_system_families_stay_denied() { + assertDenied(Runtime.class, "exec"); + assertDenied(Class.class, "forName"); + assertDenied(ClassLoader.class, "loadClass"); + assertDenied(Thread.class, "start"); + assertDenied(ProcessBuilder.class, "start"); + assertDenied(java.lang.reflect.Method.class, "invoke"); + } + + /** + * The common value types and ordinary collection types templates rely on must stay allowed; + * the added restriction must not shadow the existing fast-path allows. + */ + @Test + public void common_value_and_ordinary_types_stay_allowed() { + assertAllowed(String.class, "length"); + assertAllowed(Integer.class, "intValue"); // Number subtype + assertAllowed(Boolean.class, "booleanValue"); + assertAllowed(Class.class, "getName"); // explicitly allowed fast-path + assertAllowed(ArrayList.class, "add"); + assertAllowed(HashMap.class, "get"); + } + + /** + * Defense-in-depth: the shipped restricted-class list should also enumerate the + * exact-matchable network-resource and file-system utility types, so the configuration + * documents the intent alongside the existing restricted classes. The code-level check + * remains the enforcement floor; this keeps the config in sync with it. + */ + @Test + public void restricted_class_config_enumerates_the_exact_resource_types() throws Exception { + final java.util.Properties props = new java.util.Properties(); + try (InputStream in = getClass().getClassLoader().getResourceAsStream( + "org/apache/velocity/runtime/defaults/velocity.properties")) { + assertTrue("velocity.properties must be present on the classpath", in != null); + props.load(in); + } + final String restrictedClasses = String.join(",", props.getProperty( + "introspector.restrict.classes", "")); + // Properties.load keeps only the last value for a repeated key, so re-read raw lines. + final java.util.List configuredClasses = new java.util.ArrayList<>(); + try (java.io.BufferedReader reader = new java.io.BufferedReader(new java.io.InputStreamReader( + getClass().getClassLoader().getResourceAsStream( + "org/apache/velocity/runtime/defaults/velocity.properties")))) { + String line; + while ((line = reader.readLine()) != null) { + final String trimmed = line.trim(); + if (trimmed.startsWith("introspector.restrict.classes")) { + final int eq = trimmed.indexOf('='); + if (eq > -1) { + configuredClasses.add(trimmed.substring(eq + 1).trim()); + } + } + } + } + for (final String expected : new String[]{ + "java.net.URL", "java.net.URI", "java.nio.file.Files", "java.nio.file.Paths"}) { + assertTrue("velocity.properties introspector.restrict.classes must enumerate " + + expected, configuredClasses.contains(expected) + || restrictedClasses.contains(expected)); + } + } + + /** + * The operator-configurable layer denies a whole package by prefix (so deployments can push + * the sandbox toward a stricter stance without a code change), while an allow-list entry + * carves a specific class back out. Defaults are empty, so this only takes effect when + * configured. + */ + @Test + public void configurable_packages_are_denied_with_allowlist_carveout() { + final SecureIntrospectorImpl sandbox = introspector(); + try { + // Without configuration, an ordinary type is allowed. + assertAllowed(java.time.LocalDate.class, "getYear"); + + // Configuring a restricted package prefix denies the package and its subpackages. + Config.setProperty(SecureIntrospectorImpl.RESTRICT_PACKAGES_PROP, "java.time"); + assertFalse("configured restricted package must be denied", + sandbox.checkObjectExecutePermission(java.time.LocalDate.class, "getYear")); + assertFalse("subpackage of a configured restricted package must be denied (prefix match)", + sandbox.checkObjectExecutePermission(java.time.chrono.IsoChronology.class, "getId")); + + // An allow-list entry carves one class back out of the configurable denial. + Config.setProperty(SecureIntrospectorImpl.ALLOW_CLASSES_PROP, "java.time.LocalDate"); + assertTrue("allow-listed class must be permitted despite the package restriction", + sandbox.checkObjectExecutePermission(java.time.LocalDate.class, "getYear")); + assertFalse("a non-allow-listed class in the restricted package stays denied", + sandbox.checkObjectExecutePermission(java.time.chrono.IsoChronology.class, "getId")); + + // The configurable layer never overrides the code-level floor. + assertDenied(File.class, "getCanonicalPath"); + } finally { + Config.setProperty(SecureIntrospectorImpl.RESTRICT_PACKAGES_PROP, ""); + Config.setProperty(SecureIntrospectorImpl.ALLOW_CLASSES_PROP, ""); + } + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java index f9332c46ce24..43d85ffe64fd 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java @@ -45,6 +45,7 @@ com.dotcms.workflow.helper.TestSystemActionMappingsHandlerMerger.class, com.dotcms.concurrent.lock.DotKeyLockManagerTest.class, com.dotcms.rendering.velocity.ASTMethodTest.class, + com.dotcms.rendering.velocity.SecureIntrospectorRenderTest.class, com.dotcms.rendering.velocity.VelocityMacroCacheTest.class, com.dotcms.rendering.velocity.VelocityUtilTest.class, com.dotcms.rendering.velocity.viewtools.navigation.NavToolTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/rendering/velocity/SecureIntrospectorRenderTest.java b/dotcms-integration/src/test/java/com/dotcms/rendering/velocity/SecureIntrospectorRenderTest.java new file mode 100644 index 000000000000..8bae3826b752 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/rendering/velocity/SecureIntrospectorRenderTest.java @@ -0,0 +1,79 @@ +package com.dotcms.rendering.velocity; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; + +import com.dotcms.rendering.velocity.util.VelocityUtil; +import com.dotcms.util.IntegrationTestInitService; +import java.io.File; +import java.nio.file.Path; +import java.nio.file.Paths; +import org.apache.velocity.context.Context; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Render-time counterpart of {@code SecureIntrospectorImplTest}: verifies that the Velocity + * engine actually applies the restricted-class coverage when merging a template. A template + * that receives a {@link File} or {@link Path} through the context must not be able to resolve + * file-system methods on it, so the introspector guard is proven to be wired into the engine, + * not just correct in isolation. + */ +public class SecureIntrospectorRenderTest { + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + } + + /** + * Control: an ordinary value method still resolves, confirming the engine renders normally + * and the assertions below are about the guard, not a broken setup. + */ + @Test + public void ordinary_value_methods_still_render() throws Exception { + final Context ctx = VelocityUtil.getBasicContext(); + ctx.put("greeting", "Hello World"); + assertEquals("HELLO WORLD", VelocityUtil.eval("$greeting.toUpperCase()", ctx).trim()); + } + + /** + * A {@link File} in the context must not expose its file-system path to a template: the + * denied method does not resolve, so the real path never appears in the rendered output. + */ + @Test + public void file_methods_do_not_resolve_at_render_time() throws Exception { + final File secret = File.createTempFile("dotcms-introspector-render", ".txt"); + secret.deleteOnExit(); + final String realPath = secret.getCanonicalPath(); + + final Context ctx = VelocityUtil.getBasicContext(); + ctx.put("file", secret); + + final String rendered = VelocityUtil.eval("[$file.getCanonicalPath()]", ctx); + assertFalse("File path must not be reachable from a template; rendered: " + rendered, + rendered.contains(realPath)); + } + + /** + * A concrete platform {@link Path} implementation (e.g. the JDK's internal Path type) must + * also be blocked — this is the case only the hierarchy-aware check covers. + */ + @Test + public void concrete_path_methods_do_not_resolve_at_render_time() throws Exception { + final File secret = File.createTempFile("dotcms-introspector-render-path", ".txt"); + secret.deleteOnExit(); + final String realPath = secret.getCanonicalPath(); + final Path path = Paths.get(realPath); + // Guard the premise: this is a concrete platform implementation, not the Path interface. + Assert.assertNotEquals(Path.class, path.getClass()); + + final Context ctx = VelocityUtil.getBasicContext(); + ctx.put("path", path); + + final String rendered = VelocityUtil.eval("[$path.toString()][$path.toFile()]", ctx); + assertFalse("Concrete Path methods must not be reachable from a template; rendered: " + + rendered, rendered.contains(realPath)); + } +}