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