Skip to content

Commit f703177

Browse files
surlisurli
committed
XWIKI-5168: Don't allow some methods in velocity introspector
1 parent 17fbd54 commit f703177

4 files changed

Lines changed: 182 additions & 29 deletions

File tree

‎xwiki-commons-core/xwiki-commons-velocity/pom.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
<xwiki.jacoco.instructionRatio>0.74</xwiki.jacoco.instructionRatio>
3636
<!-- Name to display by the Extension Manager -->
3737
<xwiki.extension.name>Velocity API</xwiki.extension.name>
38+
<checkstyle.suppressions.location>${basedir}/src/main/checkstyle/checkstyle-suppressions.xml</checkstyle.suppressions.location>
3839
</properties>
3940
<dependencies>
4041
<dependency>

‎xwiki-commons-core/xwiki-commons-velocity/src/main/checkstyle/checkstyle-suppressions.xml‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
3+
<!--
4+
* See the NOTICE file distributed with this work for additional
5+
* information regarding copyright ownership.
6+
*
7+
* This is free software; you can redistribute it and/or modify it
8+
* under the terms of the GNU Lesser General Public License as
9+
* published by the Free Software Foundation; either version 2.1 of
10+
* the License, or (at your option) any later version.
11+
*
12+
* This software is distributed in the hope that it will be useful,
13+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
14+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
15+
* Lesser General Public License for more details.
16+
*
17+
* You should have received a copy of the GNU Lesser General Public
18+
* License along with this software; if not, write to the Free
19+
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
20+
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
21+
-->
22+
23+
<!DOCTYPE suppressions PUBLIC
24+
"-//Puppy Crawl//DTD Suppressions 1.0//EN"
25+
"http://www.puppycrawl.com/dtds/suppressions_1_0.dtd">
26+
27+
<suppressions>
28+
<!-- The default whitelists of accepted method are too long for this check -->
29+
<suppress checks="ExecutableStatementCount" files="SecureIntrospector.java"/>
30+
</suppressions>

‎xwiki-commons-core/xwiki-commons-velocity/src/main/java/org/xwiki/velocity/introspection/SecureIntrospector.java‎

Lines changed: 72 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@
1919
*/
2020
package org.xwiki.velocity.introspection;
2121

22+
import java.io.File;
23+
import java.util.HashMap;
2224
import java.util.HashSet;
25+
import java.util.Map;
2326
import java.util.Set;
2427

2528
import org.apache.velocity.util.introspection.SecureIntrospectorImpl;
@@ -33,7 +36,8 @@
3336
*/
3437
public class SecureIntrospector extends SecureIntrospectorImpl
3538
{
36-
private final Set<String> secureClassMethods = new HashSet<>();
39+
private static final String GETNAME = "getname";
40+
private final Map<Class, Set<String>> whitelistedMethods;
3741

3842
/**
3943
* @param badClasses forbidden classes
@@ -44,41 +48,80 @@ public SecureIntrospector(String[] badClasses, String[] badPackages, Logger log)
4448
{
4549
super(badClasses, badPackages, log);
4650

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

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");
56+
private void prepareWhitelistClass()
57+
{
58+
Set<String> whitelist = new HashSet<>();
59+
whitelist.add(GETNAME);
60+
whitelist.add("getsimpleName");
61+
whitelist.add("isarray");
62+
whitelist.add("isassignablefrom");
63+
whitelist.add("isenum");
64+
whitelist.add("isinstance");
65+
whitelist.add("isinterface");
66+
whitelist.add("islocalclass");
67+
whitelist.add("ismemberclass");
68+
whitelist.add("isprimitive");
69+
whitelist.add("issynthetic");
70+
whitelist.add("getenumconstants");
71+
this.whitelistedMethods.put(Class.class, whitelist);
72+
}
7173

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

75109
@Override
76110
public boolean checkObjectExecutePermission(Class clazz, String methodName)
77111
{
78-
if (Class.class.isAssignableFrom(clazz) && methodName != null && this.secureClassMethods.contains(methodName)) {
79-
return true;
80-
} else {
81-
return super.checkObjectExecutePermission(clazz, methodName);
112+
Boolean result = null;
113+
if (methodName != null) {
114+
for (Map.Entry<Class, Set<String>> classSetEntry : this.whitelistedMethods.entrySet()) {
115+
if (classSetEntry.getKey().isAssignableFrom(clazz)) {
116+
result = classSetEntry.getValue().contains(methodName.toLowerCase());
117+
break;
118+
}
119+
}
120+
}
121+
122+
if (result == null) {
123+
result = super.checkObjectExecutePermission(clazz, methodName);
82124
}
125+
return result;
83126
}
84127
}

‎xwiki-commons-core/xwiki-commons-velocity/src/test/java/org/xwiki/velocity/introspection/SecureIntrospectorTest.java‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
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+
public class SecureIntrospectorTest
38+
{
39+
@Mock
40+
private Logger logger;
41+
42+
class CustomFile extends File {
43+
public CustomFile(String s)
44+
{
45+
super(s);
46+
}
47+
}
48+
49+
@Test
50+
void checkObjectExecutePermissionWithClass()
51+
{
52+
SecureIntrospector secureIntrospector = new SecureIntrospector(new String[] {}, new String[] {}, this.logger);
53+
assertTrue(secureIntrospector.checkObjectExecutePermission(Class.class, "isLocalClass"));
54+
}
55+
56+
@Test
57+
void checkObjectExecutePermissionWithFile()
58+
{
59+
SecureIntrospector secureIntrospector = new SecureIntrospector(new String[] {}, new String[] {}, this.logger);
60+
assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "toString"));
61+
assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "mkdir"));
62+
63+
assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "tostring"));
64+
assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "renameto"));
65+
assertFalse(secureIntrospector.checkObjectExecutePermission(File.class, "renameTo"));
66+
67+
assertTrue(secureIntrospector.checkObjectExecutePermission(CustomFile.class, "toString"));
68+
assertFalse(secureIntrospector.checkObjectExecutePermission(CustomFile.class, "mkdir"));
69+
}
70+
71+
@Test
72+
void checkObjectExecutePermissionBlacklistedClass()
73+
{
74+
SecureIntrospector secureIntrospector = new SecureIntrospector(
75+
new String[] { "java.util.ArrayList" }, new String[] {}, this.logger);
76+
assertTrue(secureIntrospector.checkObjectExecutePermission(File.class, "toString"));
77+
assertFalse(secureIntrospector.checkObjectExecutePermission(ArrayList.class, "toString"));
78+
}
79+
}

0 commit comments

Comments
 (0)