CVE-2022-24897: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: APIs to evaluate content with Velocity is a package for APIs to evaluate content with Velocity. Starting with version 2.3 and prior to 12.6.7, 12.10.3, and 13.0, the velocity scripts are not properly sandboxed against using the Java File API to perform read or write operations on the filesystem. Writing an attacking script in Velocity requires the Script rights in XWiki so not all users can use it, and it also requires finding an XWiki API which returns a File. The problem has been patched in versions 12.6.7, 12.10.3, and 13.0. There is no easy workaround for fixing this vulnerability other than upgrading and being careful when giving Script rights.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 7.5
Published: 2022-05-02T22:15Z
Repository: https://github.com/xwiki/xwiki-commons
Language: Java
Files changed: xwiki-commons-core/xwiki-commons-velocity/src/main/java/org/xwiki/velocity/introspection/SecureIntrospector.java, xwiki-commons-core/xwiki-commons-velocity/src/test/java/org/xwiki/velocity/introspection/SecureIntrospectorTest.java
Commit date: 2021-01-06 10:11:42 +0100
Commit message: XWIKI-5168: Don't allow some methods in velocity introspector (#127)


Diff: diff --git a/xwiki-commons-core/xwiki-commons-velocity/src/main/java/org/xwiki/velocity/introspection/SecureIntrospector.java b/xwiki-commons-core/xwiki-commons-velocity/src/main/java/org/xwiki/velocity/introspection/SecureIntrospector.java
index 88f97af9b..b6683653e 100644
--- a/xwiki-commons-core/xwiki-commons-velocity/src/main/java/org/xwiki/velocity/introspection/SecureIntrospector.java
+++ b/xwiki-commons-core/xwiki-commons-velocity/src/main/java/org/xwiki/velocity/introspection/SecureIntrospector.java
@@ -17,11 +17,15 @@
  * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
  * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
  */
 package org.xwiki.velocity.introspection;
 
+import java.io.File;
+import java.util.Arrays;
+import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Map;
 import java.util.Set;
 
 import org.apache.velocity.util.introspection.SecureIntrospectorImpl;
 import org.slf4j.Logger;
 
@@ -31,54 +35,96 @@
  * @version $Id$
  * @since 5.4RC1
  */
 public class SecureIntrospector extends SecureIntrospectorImpl
 {
-    private final Set<String> secureClassMethods = new HashSet<>();
+    private static final String GETNAME = "getname";
+    private final Map<Class, Set<String>> whitelistedMethods;
 
     /**
      * @param badClasses forbidden classes
      * @param badPackages forbidden packages
      * @param log the log
      */
     public SecureIntrospector(String[] badClasses, String[] badPackages, Logger log)
     {
         super(badClasses, badPackages, log);
 
-        this.secureClassMethods.add("getname");
-        this.secureClassMethods.add("getName");
-        this.secureClassMethods.add("getsimpleName");
-        this.secureClassMethods.add("getSimpleName");
+        this.whitelistedMethods = new HashMap<>();
+        this.prepareWhitelistClass();
+        this.prepareWhiteListFile();
+    }
 
-        this.secureClassMethods.add("isarray");
-        this.secureClassMethods.add("isArray");
-        this.secureClassMethods.add("isassignablefrom");
-        this.secureClassMethods.add("isAssignableFrom");
-        this.secureClassMethods.add("isenum");
-        this.secureClassMethods.add("isEnum");
-        this.secureClassMethods.add("isinstance");
-        this.secureClassMethods.add("isInstance");
-        this.secureClassMethods.add("isinterface");
-        this.secureClassMethods.add("isInterface");
-        this.secureClassMethods.add("islocalClass");
-        this.secureClassMethods.add("isLocalClass");
-        this.secureClassMethods.add("ismemberclass");
-        this.secureClassMethods.add("isMemberClass");
-        this.secureClassMethods.add("isprimitive");
-        this.secureClassMethods.add("isPrimitive");
-        this.secureClassMethods.add("issynthetic");
-        this.secureClassMethods.add("isSynthetic");
-        this.secureClassMethods.add("getEnumConstants");
+    private void prepareWhitelistClass()
+    {
+        Set<String> whitelist = new HashSet<>(Arrays.asList(
+            GETNAME,
+            "getsimpleName",
+            "isarray",
+            "isassignablefrom",
+            "isenum",
+            "isinstance",
+            "isinterface",
+            "islocalclass",
+            "ismemberclass",
+            "isprimitive",
+            "issynthetic",
+            "getenumconstants"
+        ));
+        this.whitelistedMethods.put(Class.class, whitelist);
+    }
 
-        // TODO: add more when needed
+    private void prepareWhiteListFile()
+    {
+        Set<String> whitelist = new HashSet<>(Arrays.asList(
+            "canexecute",
+            "canread",
+            "canwrite",
+            "compareto",
+            "createtempfile",
+            "equals",
+            "getabsolutefile",
+            "getabsolutepath",
+            "getcanonicalfile",
+            "getcanonicalpath",
+            "getfreespace",
+            GETNAME,
+            "getparent",
+            "getparentfile",
+            "getpath",
+            "gettotalspace",
+            "getusablespace",
+            "hashcode",
+            "isabsolute",
+            "isdirectory",
+            "isfile",
+            "ishidden",
+            "lastmodified",
+            "length",
+            "topath",
+            "tostring",
+            "touri",
+            "tourl",
+            "getclass"
+        ));
+        this.whitelistedMethods.put(File.class, whitelist);
     }
 
     @Override
     public boolean checkObjectExecutePermission(Class clazz, String methodName)
     {
-        if (Class.class.isAssignableFrom(clazz) && methodName != null && this.secureClassMethods.contains(methodName)) {
-            return true;
-        } else {
-            return super.checkObjectExecutePermission(clazz, methodName);
+        Boolean result = null;
+        if (methodName != null) {
+            for (Map.Entry<Class, Set<String>> classSetEntry : this.whitelistedMethods.entrySet()) {
+                if (classSetEntry.getKey().isAssignableFrom(clazz)) {
+                    result = classSetEntry.getValue().contains(methodName.toLowerCase());
+                    break;
+                }
+            }
+        }
+
+        if (result == null) {
+            result = super.checkObjectExecutePermission(clazz, methodName);
         }
+        return result;
     }
 }
diff --git a/xwiki-commons-core/xwiki-commons-velocity/src/test/java/org/xwiki/velocity/introspection/SecureIntrospectorTest.java b/xwiki-commons-core/xwiki-commons-velocity/src/test/java/org/xwiki/velocity/introspection/SecureIntrospectorTest.java
new file mode 100644
index 000000000..cd8c7a9df
--- /dev/null
+++ b/xwiki-commons-core/xwiki-commons-velocity/src/test/java/org/xwiki/velocity/introspection/SecureIntrospectorTest.java
@@ -0,0 +1,80 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.xwiki.velocity.introspection;
+
+import java.io.File;
+import java.util.ArrayList;
+
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.slf4j.Logger;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for {@link SecureIntrospector}.
+ *
+ * @version $Id$
+ */
+class SecureIntrospectorTest
+{
+    @Mock
+    private Logger logger;
+
+    class CustomFile extends File
+    {
+        public CustomFile(String s)
+        {
+            super(s);
+        }
+    }
+
+    @Test
+    void checkObjectExecutePermissionWithClass()
+    {
+        SecureIntrospector secureIntrospector = new SecureIntrospector(new String[] {}, new String[] {}, this.logger);
+        assertTrue(secureIntrospector.checkObjectExecutePermission(Class.class, "isLocalClass"));
+    }
+
+    @Test
+    void checkObjectExecutePermissionWithFile()
+    {
+        SecureIntrospector secureIntrospector = new SecureIntrospector(new String[] {}, new String[] {}, this.logger);
+        assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "toString"));
+        assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "mkdir"));
+
+        assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "tostring"));
+        assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "renameto"));
+        assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "renameTo"));
+
+        assertTrue(secureIntrospector.checkObjectExecutePermission(CustomFile.class, "toString"));
+        assertFalse(secureIntrospector.checkObjectExecutePermission(CustomFile.class, "mkdir"));
+    }
+
+    @Test
+    void checkObjectExecutePermissionBlacklistedClass()
+    {
+        SecureIntrospector secureIntrospector = new SecureIntrospector(
+            new String[] { "java.util.ArrayList" }, new String[] {}, this.logger);
+        assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "toString"));
+        assertFalse(secureIntrospector.checkObjectExecutePermission(ArrayList.class, "toString"));
+    }
+}

Vulnerable code:     private final Set<String> secureClassMethods = new HashSet<>();
        this.secureClassMethods.add("getname");
        this.secureClassMethods.add("getName");
        this.secureClassMethods.add("getsimpleName");
        this.secureClassMethods.add("getSimpleName");
        this.secureClassMethods.add("isarray");
        this.secureClassMethods.add("isArray");
        this.secureClassMethods.add("isassignablefrom");
        this.secureClassMethods.add("isAssignableFrom");
        this.secureClassMethods.add("isenum");
        this.secureClassMethods.add("isEnum");
        this.secureClassMethods.add("isinstance");
        this.secureClassMethods.add("isInstance");
        this.secureClassMethods.add("isinterface");
        this.secureClassMethods.add("isInterface");
        this.secureClassMethods.add("islocalClass");
        this.secureClassMethods.add("isLocalClass");
        this.secureClassMethods.add("ismemberclass");
        this.secureClassMethods.add("isMemberClass");
        this.secureClassMethods.add("isprimitive");
        this.secureClassMethods.add("isPrimitive");
        this.secureClassMethods.add("issynthetic");
        this.secureClassMethods.add("isSynthetic");
        this.secureClassMethods.add("getEnumConstants");
        // TODO: add more when needed
        if (Class.class.isAssignableFrom(clazz) && methodName != null && this.secureClassMethods.contains(methodName)) {
            return true;
        } else {
            return super.checkObjectExecutePermission(clazz, methodName);
Fixed code: import java.io.File;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
    private static final String GETNAME = "getname";
    private final Map<Class, Set<String>> whitelistedMethods;
        this.whitelistedMethods = new HashMap<>();
        this.prepareWhitelistClass();
        this.prepareWhiteListFile();
    }
    private void prepareWhitelistClass()
    {
        Set<String> whitelist = new HashSet<>(Arrays.asList(
            GETNAME,
            "getsimpleName",
            "isarray",
            "isassignablefrom",
            "isenum",
            "isinstance",
            "isinterface",
            "islocalclass",
            "ismemberclass",
            "isprimitive",
            "issynthetic",
            "getenumconstants"
        ));
        this.whitelistedMethods.put(Class.class, whitelist);
    }
    private void prepareWhiteListFile()
    {
        Set<String> whitelist = new HashSet<>(Arrays.asList(
            "canexecute",
            "canread",
            "canwrite",
            "compareto",
            "createtempfile",
            "equals",
            "getabsolutefile",
            "getabsolutepath",
            "getcanonicalfile",
            "getcanonicalpath",
            "getfreespace",
            GETNAME,
            "getparent",
            "getparentfile",
            "getpath",
            "gettotalspace",
            "getusablespace",
            "hashcode",
            "isabsolute",
            "isdirectory",
            "isfile",
            "ishidden",
            "lastmodified",
            "length",
            "topath",
            "tostring",
            "touri",
            "tourl",
            "getclass"
        ));
        this.whitelistedMethods.put(File.class, whitelist);
        Boolean result = null;
        if (methodName != null) {
            for (Map.Entry<Class, Set<String>> classSetEntry : this.whitelistedMethods.entrySet()) {
                if (classSetEntry.getKey().isAssignableFrom(clazz)) {
                    result = classSetEntry.getValue().contains(methodName.toLowerCase());
                    break;
                }
            }
        }

        if (result == null) {
            result = super.checkObjectExecutePermission(clazz, methodName);
        return result;
/*
 * See the NOTICE file distributed with this work for additional
 * information regarding copyright ownership.
 *
 * This is free software; you can redistribute it and/or modify it
 * under the terms of the GNU Lesser General Public License as
 * published by the Free Software Foundation; either version 2.1 of
 * the License, or (at your option) any later version.
 *
 * This software is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
 * Lesser General Public License for more details.
 *
 * You should have received a copy of the GNU Lesser General Public
 * License along with this software; if not, write to the Free
 * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
 * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
 */
package org.xwiki.velocity.introspection;

import java.io.File;
import java.util.ArrayList;

import org.junit.jupiter.api.Test;
import org.mockito.Mock;
import org.slf4j.Logger;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

/**
 * Tests for {@link SecureIntrospector}.
 *
 * @version $Id$
 */
class SecureIntrospectorTest
{
    @Mock
    private Logger logger;

    class CustomFile extends File
    {
        public CustomFile(String s)
        {
            super(s);
        }
    }

    @Test
    void checkObjectExecutePermissionWithClass()
    {
        SecureIntrospector secureIntrospector = new SecureIntrospector(new String[] {}, new String[] {}, this.logger);
        assertTrue(secureIntrospector.checkObjectExecutePermission(Class.class, "isLocalClass"));
    }

    @Test
    void checkObjectExecutePermissionWithFile()
    {
        SecureIntrospector secureIntrospector = new SecureIntrospector(new String[] {}, new String[] {}, this.logger);
        assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "toString"));
        assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "mkdir"));

        assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "tostring"));
        assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "renameto"));
        assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "renameTo"));

        assertTrue(secureIntrospector.checkObjectExecutePermission(CustomFile.class, "toString"));
        assertFalse(secureIntrospector.checkObjectExecutePermission(CustomFile.class, "mkdir"));
    }

    @Test
    void checkObjectExecutePermissionBlacklistedClass()
    {
        SecureIntrospector secureIntrospector = new SecureIntrospector(
            new String[] { "java.util.ArrayList" }, new String[] {}, this.logger);
        assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "toString"));
        assertFalse(secureIntrospector.checkObjectExecutePermission(ArrayList.class, "toString"));
    }
}