Skip to content

Commit

Permalink
Merge pull request apache#736 from apache/WW-5337-exclusion-performance
Browse files Browse the repository at this point in the history 
WW-5337 Make SecurityMemberAccess exclusion checking more performant
  • Loading branch information
@kusalk
kusalk authored Aug 22, 2023
2 parents 0ef086c + 783063c commit e75f8d8
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 95 deletions.
26 changes: 14 additions & 12 deletions core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import com.opensymphony.xwork2.inject.Inject;
import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor;
import com.opensymphony.xwork2.util.CompoundRoot;
import com.opensymphony.xwork2.util.TextParseUtil;
import com.opensymphony.xwork2.util.reflection.ReflectionException;
import ognl.ClassResolver;
import ognl.Ognl;
Expand All @@ -51,6 +50,11 @@
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;

import static com.opensymphony.xwork2.util.TextParseUtil.commaDelimitedStringToSet;
import static java.util.stream.Collectors.toSet;
import static org.apache.commons.lang3.StringUtils.strip;


/**
Expand Down Expand Up @@ -187,17 +191,15 @@ protected void setDevModeExcludedClasses(String commaDelimitedClasses) {
}

private Set<Class<?>> parseClasses(String commaDelimitedClasses) {
Set<String> classNames = TextParseUtil.commaDelimitedStringToSet(commaDelimitedClasses);
Set<String> classNames = commaDelimitedStringToSet(commaDelimitedClasses);
Set<Class<?>> classes = new HashSet<>();

for (String className : classNames) {
try {
classes.add(Class.forName(className));
} catch (ClassNotFoundException e) {
throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e);
}
}

return classes;
}

Expand All @@ -218,14 +220,13 @@ protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackag
}

private Set<Pattern> parseExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) {
Set<String> packagePatterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackagePatterns);
Set<Pattern> packageNamePatterns = new HashSet<>();

for (String pattern : packagePatterns) {
packageNamePatterns.add(Pattern.compile(pattern));
try {
return commaDelimitedStringToSet(commaDelimitedPackagePatterns)
.stream().map(Pattern::compile).collect(toSet());
} catch (PatternSyntaxException e) {
throw new ConfigurationException(
"Excluded package name patterns could not be parsed due to invalid regex: " + commaDelimitedPackagePatterns, e);
}

return packageNamePatterns;
}

@Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false)
Expand Down Expand Up @@ -261,7 +262,8 @@ public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses)
}

private Set<String> parseExcludedPackageNames(String commaDelimitedPackageNames) {
Set<String> parsedSet = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackageNames);
Set<String> parsedSet = commaDelimitedStringToSet(commaDelimitedPackageNames)
.stream().map(s -> strip(s, ".")).collect(toSet());
if (parsedSet.stream().anyMatch(s -> s.matches("(.*?)\\s(.*?)"))) {
throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + commaDelimitedPackageNames);
}
Expand Down
50 changes: 27 additions & 23 deletions core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@
import java.lang.reflect.Field;
import java.lang.reflect.Member;
import java.lang.reflect.Modifier;
import java.util.Collections;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static java.util.Collections.emptySet;
import static java.util.Collections.unmodifiableSet;

/**
* Allows access decisions to be made on the basis of whether a member is static or not.
* Also blocks or allows access to properties.
Expand All @@ -43,12 +47,12 @@ public class SecurityMemberAccess implements MemberAccess {
private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class);

private final boolean allowStaticFieldAccess;
private Set<Pattern> excludeProperties = Collections.emptySet();
private Set<Pattern> acceptProperties = Collections.emptySet();
private Set<Class<?>> excludedClasses = Collections.emptySet();
private Set<Pattern> excludedPackageNamePatterns = Collections.emptySet();
private Set<String> excludedPackageNames = Collections.emptySet();
private Set<Class<?>> excludedPackageExemptClasses = Collections.emptySet();
private Set<Pattern> excludeProperties = emptySet();
private Set<Pattern> acceptProperties = emptySet();
private Set<Class<?>> excludedClasses = emptySet();
private Set<Pattern> excludedPackageNamePatterns = emptySet();
private Set<String> excludedPackageNames = emptySet();
private Set<Class<?>> excludedPackageExemptClasses = emptySet();
private boolean disallowProxyMemberAccess;

/**
Expand All @@ -60,6 +64,7 @@ public class SecurityMemberAccess implements MemberAccess {
*/
public SecurityMemberAccess(boolean allowStaticFieldAccess) {
this.allowStaticFieldAccess = allowStaticFieldAccess;
useExcludedClasses(excludedClasses); // Initialise default exclusions
}

@Override
Expand Down Expand Up @@ -237,33 +242,27 @@ protected String toPackageName(Class<?> clazz) {

protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
String packageName = toPackageName(clazz);
for (Pattern pattern : excludedPackageNamePatterns) {
if (pattern.matcher(packageName).matches()) {
return true;
}
}
return false;
return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(packageName).matches());
}

protected boolean isExcludedPackageNames(Class<?> clazz) {
String suffixedPackageName = toPackageName(clazz) + ".";
for (String excludedPackageName : excludedPackageNames) {
if (suffixedPackageName.startsWith(excludedPackageName)) {
String packageName = toPackageName(clazz);
List<String> packageParts = Arrays.asList(packageName.split("\\."));
for (int i = 0; i < packageParts.size(); i++) {
String parentPackage = String.join(".", packageParts.subList(0, i + 1));
if (excludedPackageNames.contains(parentPackage)) {
return true;
}
}
return false;
}

protected boolean isClassExcluded(Class<?> clazz) {
if (clazz == Object.class || (clazz == Class.class && !allowStaticFieldAccess)) {
return true;
}
return excludedClasses.stream().anyMatch(clazz::isAssignableFrom);
return excludedClasses.contains(clazz);
}

protected boolean isExcludedPackageExempt(Class<?> clazz) {
return excludedPackageExemptClasses.stream().anyMatch(clazz::equals);
return excludedPackageExemptClasses.contains(clazz);
}

protected boolean isAcceptableProperty(String name) {
Expand Down Expand Up @@ -328,11 +327,16 @@ public void useAcceptProperties(Set<Pattern> acceptedProperties) {
*/
@Deprecated
public void setExcludedClasses(Set<Class<?>> excludedClasses) {
this.excludedClasses = excludedClasses;
useExcludedClasses(excludedClasses);
}

public void useExcludedClasses(Set<Class<?>> excludedClasses) {
this.excludedClasses = excludedClasses;
Set<Class<?>> newExcludedClasses = new HashSet<>(excludedClasses);
newExcludedClasses.add(Object.class);
if (!allowStaticFieldAccess) {
newExcludedClasses.add(Class.class);
}
this.excludedClasses = unmodifiableSet(newExcludedClasses);
}

/**
Expand Down
84 changes: 42 additions & 42 deletions core/src/main/resources/struts-excluded-classes.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,52 +55,52 @@
<!-- constant name="struts.excludedPackageNamePatterns" value="^java\.lang\..*,^ognl.*,^(?!javax\.servlet\..+)(javax\..+)" / -->
<!-- constant name="struts.devMode.excludedPackageNamePatterns" value="^java\.lang\..*,^ognl.*,^(?!javax\.servlet\..+)(javax\..+)" / -->

<!-- this is simpler version of the above used with string comparison -->
<!-- All classes within the following packages and their sub-packages are excluded -->
<constant name="struts.excludedPackageNames"
value="
ognl.,
java.io.,
java.net.,
java.nio.,
javax.,
freemarker.core.,
freemarker.template.,
freemarker.ext.jsp.,
freemarker.ext.rhino.,
sun.misc.,
sun.reflect.,
javassist.,
org.apache.velocity.,
org.objectweb.asm.,
org.springframework.context.,
com.opensymphony.xwork2.inject.,
com.opensymphony.xwork2.ognl.,
com.opensymphony.xwork2.security.,
com.opensymphony.xwork2.util.,
org.apache.tomcat.,
org.apache.catalina.core.,
org.wildfly.extension.undertow.deployment."/>
ognl,
java.io,
java.net,
java.nio,
javax,
freemarker.core,
freemarker.template,
freemarker.ext.jsp,
freemarker.ext.rhino,
sun.misc,
sun.reflect,
javassist,
org.apache.velocity,
org.objectweb.asm,
org.springframework.context,
com.opensymphony.xwork2.inject,
com.opensymphony.xwork2.ognl,
com.opensymphony.xwork2.security,
com.opensymphony.xwork2.util,
org.apache.tomcat,
org.apache.catalina.core,
org.wildfly.extension.undertow.deployment"/>

<constant name="struts.devMode.excludedPackageNames"
value="
ognl.,
java.io.,
java.net.,
java.nio.,
javax.,
freemarker.core.,
freemarker.template.,
freemarker.ext.jsp.,
freemarker.ext.rhino.,
sun.misc.,
sun.reflect.,
javassist.,
org.apache.velocity.,
org.objectweb.asm.,
org.springframework.context.,
com.opensymphony.xwork2.inject.,
com.opensymphony.xwork2.ognl.,
com.opensymphony.xwork2.security.,
com.opensymphony.xwork2.util."/>
ognl,
java.io,
java.net,
java.nio,
javax,
freemarker.core,
freemarker.template,
freemarker.ext.jsp,
freemarker.ext.rhino,
sun.misc,
sun.reflect,
javassist,
org.apache.velocity,
org.objectweb.asm,
org.springframework.context,
com.opensymphony.xwork2.inject,
com.opensymphony.xwork2.ognl,
com.opensymphony.xwork2.security,
com.opensymphony.xwork2.util"/>

</struts>
19 changes: 1 addition & 18 deletions core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,23 +178,6 @@ public void testMiddleOfInheritanceExclusion3() throws Exception {
assertTrue("barLogic() from BarInterface isn't accessible!!!", accessible);
}

@Test
public void testMiddleOfInheritanceExclusion4() throws Exception {
// given
String propertyName = "barLogic";
Member member = BarInterface.class.getMethod(propertyName);

Set<Class<?>> excluded = new HashSet<>();
excluded.add(FooBarInterface.class);
sma.useExcludedClasses(excluded);

// when
boolean accessible = sma.isAccessible(context, target, member, propertyName);

// then
assertFalse("barLogic() from BarInterface is accessible!!!", accessible);
}

@Test
public void testPackageExclusion() throws Exception {
// given
Expand Down Expand Up @@ -790,7 +773,7 @@ public void testAccessMemberAccessIsBlocked() throws Exception {
@Test
public void testPackageNameExclusionAsCommaDelimited() {
// given
sma.useExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang."));
sma.useExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang"));

// when
boolean actual = sma.isPackageExcluded(String.class, String.class);
Expand Down

0 comments on commit e75f8d8

Please sign in to comment.