Skip to content

Commit 215951c

Browse files
authored
XWIKI-5168: Don't allow some methods in velocity introspector (#127)
1 parent 75aadf3 commit 215951c

File tree

2 files changed

+155
-29
lines changed

2 files changed

+155
-29
lines changed

Diff for: xwiki-commons-core/xwiki-commons-velocity/src/main/java/org/xwiki/velocity/introspection/SecureIntrospector.java

+75-29
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,11 @@
1919
*/
2020
package org.xwiki.velocity.introspection;
2121

22+
import java.io.File;
23+
import java.util.Arrays;
24+
import java.util.HashMap;
2225
import java.util.HashSet;
26+
import java.util.Map;
2327
import java.util.Set;
2428

2529
import org.apache.velocity.util.introspection.SecureIntrospectorImpl;
@@ -33,7 +37,8 @@
3337
*/
3438
public class SecureIntrospector extends SecureIntrospectorImpl
3539
{
36-
private final Set<String> secureClassMethods = new HashSet<>();
40+
private static final String GETNAME = "getname";
41+
private final Map<Class, Set<String>> whitelistedMethods;
3742

3843
/**
3944
* @param badClasses forbidden classes
@@ -44,41 +49,82 @@ public SecureIntrospector(String[] badClasses, String[] badPackages, Logger log)
4449
{
4550
super(badClasses, badPackages, log);
4651

47-
this.secureClassMethods.add("getname");
48-
this.secureClassMethods.add("getName");
49-
this.secureClassMethods.add("getsimpleName");
50-
this.secureClassMethods.add("getSimpleName");
52+
this.whitelistedMethods = new HashMap<>();
53+
this.prepareWhitelistClass();
54+
this.prepareWhiteListFile();
55+
}
5156

52-
this.secureClassMethods.add("isarray");
53-
this.secureClassMethods.add("isArray");
54-
this.secureClassMethods.add("isassignablefrom");
55-
this.secureClassMethods.add("isAssignableFrom");
56-
this.secureClassMethods.add("isenum");
57-
this.secureClassMethods.add("isEnum");
58-
this.secureClassMethods.add("isinstance");
59-
this.secureClassMethods.add("isInstance");
60-
this.secureClassMethods.add("isinterface");
61-
this.secureClassMethods.add("isInterface");
62-
this.secureClassMethods.add("islocalClass");
63-
this.secureClassMethods.add("isLocalClass");
64-
this.secureClassMethods.add("ismemberclass");
65-
this.secureClassMethods.add("isMemberClass");
66-
this.secureClassMethods.add("isprimitive");
67-
this.secureClassMethods.add("isPrimitive");
68-
this.secureClassMethods.add("issynthetic");
69-
this.secureClassMethods.add("isSynthetic");
70-
this.secureClassMethods.add("getEnumConstants");
57+
private void prepareWhitelistClass()
58+
{
59+
Set<String> whitelist = new HashSet<>(Arrays.asList(
60+
GETNAME,
61+
"getsimpleName",
62+
"isarray",
63+
"isassignablefrom",
64+
"isenum",
65+
"isinstance",
66+
"isinterface",
67+
"islocalclass",
68+
"ismemberclass",
69+
"isprimitive",
70+
"issynthetic",
71+
"getenumconstants"
72+
));
73+
this.whitelistedMethods.put(Class.class, whitelist);
74+
}
7175

72-
// TODO: add more when needed
76+
private void prepareWhiteListFile()
77+
{
78+
Set<String> whitelist = new HashSet<>(Arrays.asList(
79+
"canexecute",
80+
"canread",
81+
"canwrite",
82+
"compareto",
83+
"createtempfile",
84+
"equals",
85+
"getabsolutefile",
86+
"getabsolutepath",
87+
"getcanonicalfile",
88+
"getcanonicalpath",
89+
"getfreespace",
90+
GETNAME,
91+
"getparent",
92+
"getparentfile",
93+
"getpath",
94+
"gettotalspace",
95+
"getusablespace",
96+
"hashcode",
97+
"isabsolute",
98+
"isdirectory",
99+
"isfile",
100+
"ishidden",
101+
"lastmodified",
102+
"length",
103+
"topath",
104+
"tostring",
105+
"touri",
106+
"tourl",
107+
"getclass"
108+
));
109+
this.whitelistedMethods.put(File.class, whitelist);
73110
}
74111

75112
@Override
76113
public boolean checkObjectExecutePermission(Class clazz, String methodName)
77114
{
78-
if (Class.class.isAssignableFrom(clazz) && methodName != null && this.secureClassMethods.contains(methodName)) {
79-
return true;
80-
} else {
81-
return super.checkObjectExecutePermission(clazz, methodName);
115+
Boolean result = null;
116+
if (methodName != null) {
117+
for (Map.Entry<Class, Set<String>> classSetEntry : this.whitelistedMethods.entrySet()) {
118+
if (classSetEntry.getKey().isAssignableFrom(clazz)) {
119+
result = classSetEntry.getValue().contains(methodName.toLowerCase());
120+
break;
121+
}
122+
}
123+
}
124+
125+
if (result == null) {
126+
result = super.checkObjectExecutePermission(clazz, methodName);
82127
}
128+
return result;
83129
}
84130
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/*
2+
* See the NOTICE file distributed with this work for additional
3+
* information regarding copyright ownership.
4+
*
5+
* This is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU Lesser General Public License as
7+
* published by the Free Software Foundation; either version 2.1 of
8+
* the License, or (at your option) any later version.
9+
*
10+
* This software is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
* Lesser General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Lesser General Public
16+
* License along with this software; if not, write to the Free
17+
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
18+
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
19+
*/
20+
package org.xwiki.velocity.introspection;
21+
22+
import java.io.File;
23+
import java.util.ArrayList;
24+
25+
import org.junit.jupiter.api.Test;
26+
import org.mockito.Mock;
27+
import org.slf4j.Logger;
28+
29+
import static org.junit.jupiter.api.Assertions.assertFalse;
30+
import static org.junit.jupiter.api.Assertions.assertTrue;
31+
32+
/**
33+
* Tests for {@link SecureIntrospector}.
34+
*
35+
* @version $Id$
36+
*/
37+
class SecureIntrospectorTest
38+
{
39+
@Mock
40+
private Logger logger;
41+
42+
class CustomFile extends File
43+
{
44+
public CustomFile(String s)
45+
{
46+
super(s);
47+
}
48+
}
49+
50+
@Test
51+
void checkObjectExecutePermissionWithClass()
52+
{
53+
SecureIntrospector secureIntrospector = new SecureIntrospector(new String[] {}, new String[] {}, this.logger);
54+
assertTrue(secureIntrospector.checkObjectExecutePermission(Class.class, "isLocalClass"));
55+
}
56+
57+
@Test
58+
void checkObjectExecutePermissionWithFile()
59+
{
60+
SecureIntrospector secureIntrospector = new SecureIntrospector(new String[] {}, new String[] {}, this.logger);
61+
assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "toString"));
62+
assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "mkdir"));
63+
64+
assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "tostring"));
65+
assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "renameto"));
66+
assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "renameTo"));
67+
68+
assertTrue(secureIntrospector.checkObjectExecutePermission(CustomFile.class, "toString"));
69+
assertFalse(secureIntrospector.checkObjectExecutePermission(CustomFile.class, "mkdir"));
70+
}
71+
72+
@Test
73+
void checkObjectExecutePermissionBlacklistedClass()
74+
{
75+
SecureIntrospector secureIntrospector = new SecureIntrospector(
76+
new String[] { "java.util.ArrayList" }, new String[] {}, this.logger);
77+
assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "toString"));
78+
assertFalse(secureIntrospector.checkObjectExecutePermission(ArrayList.class, "toString"));
79+
}
80+
}

0 commit comments

Comments
 (0)