Merge pull request #16 from wglasshusain/VELOCITY-931-update-secure-classlist

Velocity 931 update secure classlist

3 files changed, 77 insertions, 38 deletions

diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/SecureIntrospectorImpl.java b/velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/SecureIntrospectorImpl.java
index 38ac7ce8..65aa04cf 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/SecureIntrospectorImpl.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/SecureIntrospectorImpl.java
@@ -87,30 +87,30 @@ public class SecureIntrospectorImpl extends Introspector implements SecureIntros
      */
     public boolean checkObjectExecutePermission(Class clazz, String methodName)
     {
-		/**
-		 * check for wait and notify
-		 */
+        /**
+         * check for wait and notify
+         */
         if (methodName != null &&
             (methodName.equals("wait") || methodName.equals("notify")) )
-		{
-			return false;
-		}
+        {
+            return false;
+        }
 
-		/**
-		 * Always allow the most common classes - Number, Boolean and String
-		 */
-		else if (Number.class.isAssignableFrom(clazz))
-		{
-			return true;
-		}
-		else if (Boolean.class.isAssignableFrom(clazz))
-		{
-			return true;
-		}
-		else if (String.class.isAssignableFrom(clazz))
-		{
-			return true;
-		}
+        /**
+         * Always allow the most common classes - Number, Boolean and String
+         */
+        else if (Number.class.isAssignableFrom(clazz))
+        {
+            return true;
+        }
+        else if (Boolean.class.isAssignableFrom(clazz))
+        {
+            return true;
+        }
+        else if (String.class.isAssignableFrom(clazz))
+        {
+            return true;
+        }
 
         /**
          * Always allow Class.getName()
@@ -121,6 +121,15 @@ public class SecureIntrospectorImpl extends Introspector implements SecureIntros
             return true;
         }
 
+       /**
+       * Always disallow ClassLoader, Thread and subclasses
+       */
+        if (ClassLoader.class.isAssignableFrom(clazz) ||
+                Thread.class.isAssignableFrom(clazz))
+        {
+            return false;
+        }
+
         /**
          * check the classname (minus any array info)
          * whether it matches disallowed classes or packages
diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/SecureIntrospectionTestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/SecureIntrospectionTestCase.java
index 8653f40a..081e48b2 100644
--- a/velocity-engine-core/src/test/java/org/apache/velocity/test/SecureIntrospectionTestCase.java
+++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/SecureIntrospectionTestCase.java
@@ -33,6 +33,9 @@ import org.apache.velocity.util.introspection.SecureUberspector;
 import java.io.IOException;
 import java.io.StringWriter;
 import java.io.Writer;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
 import java.util.Collection;
 import java.util.HashSet;
 
@@ -138,9 +141,9 @@ public class SecureIntrospectionTestCase extends BaseTestCase
 
     private boolean doesStringEvaluate(VelocityEngine ve, Context c, String inputString) throws ParseErrorException, MethodInvocationException, ResourceNotFoundException, IOException
     {
-    	// assume that an evaluation is bad if the input and result are the same (e.g. a bad reference)
-    	// or the result is an empty string (e.g. bad #foreach)
-    	Writer w = new StringWriter();
+        // assume that an evaluation is bad if the input and result are the same (e.g. a bad reference)
+        // or the result is an empty string (e.g. bad #foreach)
+        Writer w = new StringWriter();
         ve.evaluate(c, w, "foo", inputString);
         String result = w.toString();
         return (result.length() > 0 ) &&  !result.equals(inputString);
@@ -163,14 +166,35 @@ public class SecureIntrospectionTestCase extends BaseTestCase
     }
 
 
-	public Collection getCollection()
-	{
-		Collection c = new HashSet();
-		c.add("aaa");
-		c.add("bbb");
-		c.add("ccc");
-		return c;
-	}
+    public Collection getCollection()
+    {
+        Collection c = new HashSet();
+        c.add("aaa");
+        c.add("bbb");
+        c.add("ccc");
+        return c;
+    }
+
+    public ClassLoader getSampleClassLoader1()
+    {
+        return this.getClass().getClassLoader();
+    }
+
+    /**
+     * sample property which is a subclass of ClassLoader
+     * @return
+     */
+    public ClassLoader getSampleClassLoader2()
+    {
+        try
+        {
+            return new URLClassLoader(new URL[]{new URL("file://.")}, this.getClass().getClassLoader());
+        }
+        catch (MalformedURLException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
 
 }
 
diff --git a/velocity-engine-core/src/test/resources/oldproperties/velocity.properties b/velocity-engine-core/src/test/resources/oldproperties/velocity.properties
index b3b61a6e..ce487db0 100644
--- a/velocity-engine-core/src/test/resources/oldproperties/velocity.properties
+++ b/velocity-engine-core/src/test/resources/oldproperties/velocity.properties
@@ -220,15 +220,15 @@ runtime.conversion.handler.class = org.apache.velocity.util.introspection.TypeCo
 # accessed.
 # ----------------------------------------------------------------------------
 
+# Prohibit reflection
 introspector.restrict.packages = java.lang.reflect
 
-# The two most dangerous classes
+# ClassLoader, Thread, and subclasses disabled by default in SecureIntrospectorImpl
 
-introspector.restrict.classes = java.lang.Class
-introspector.restrict.classes = java.lang.ClassLoader
-
-# Restrict these for extra safety
+# Restrict these system classes.  Note that anything in this list is matched exactly.
+# (Subclasses must be explicitly named to be included).
 
+introspector.restrict.classes = java.lang.Class
 introspector.restrict.classes = java.lang.Compiler
 introspector.restrict.classes = java.lang.InheritableThreadLocal
 introspector.restrict.classes = java.lang.Package
@@ -237,10 +237,16 @@ introspector.restrict.classes = java.lang.Runtime
 introspector.restrict.classes = java.lang.RuntimePermission
 introspector.restrict.classes = java.lang.SecurityManager
 introspector.restrict.classes = java.lang.System
-introspector.restrict.classes = java.lang.Thread
 introspector.restrict.classes = java.lang.ThreadGroup
 introspector.restrict.classes = java.lang.ThreadLocal
 
+# Restrict instance managers for common servlet containers  (Tomcat, JBoss, Jetty)
+
+introspector.restrict.classes = org.apache.catalina.core.DefaultInstanceManager
+introspector.restrict.classes = org.apache.tomcat.SimpleInstanceManager
+introspector.restrict.classes = org.wildfly.extension.undertow.deployment.UndertowJSPInstanceManager
+introspector.restrict.classes = org.eclipse.jetty.util.DecoratedObjectFactory
+
 # ----------------------------------------------------------------------------
 # SPACE GOBBLING
 # ----------------------------------------------------------------------------