diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index c6169afa9..005d17eba 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -24,7 +24,6 @@ import com.opensymphony.xwork2.inject.Container; 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; @@ -51,6 +50,11 @@ import java.util.Map; 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; /** @@ -187,9 +191,8 @@ public class OgnlUtil { } private Set> parseClasses(String commaDelimitedClasses) { - Set classNames = TextParseUtil.commaDelimitedStringToSet(commaDelimitedClasses); + Set classNames = commaDelimitedStringToSet(commaDelimitedClasses); Set> classes = new HashSet<>(); - for (String className : classNames) { try { classes.add(Class.forName(className)); @@ -197,7 +200,6 @@ public class OgnlUtil { throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e); } } - return classes; } @@ -218,14 +220,13 @@ public class OgnlUtil { } private Set parseExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - Set packagePatterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackagePatterns); - Set 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) @@ -261,7 +262,8 @@ public class OgnlUtil { } private Set parseExcludedPackageNames(String commaDelimitedPackageNames) { - Set parsedSet = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackageNames); + Set 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); } diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 18acc675c..5db52639e 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -27,13 +27,17 @@ import java.lang.reflect.AccessibleObject; 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. @@ -43,12 +47,12 @@ public class SecurityMemberAccess implements MemberAccess { private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class); private final boolean allowStaticFieldAccess; - private Set excludeProperties = Collections.emptySet(); - private Set acceptProperties = Collections.emptySet(); - private Set> excludedClasses = Collections.emptySet(); - private Set excludedPackageNamePatterns = Collections.emptySet(); - private Set excludedPackageNames = Collections.emptySet(); - private Set> excludedPackageExemptClasses = Collections.emptySet(); + private Set excludeProperties = emptySet(); + private Set acceptProperties = emptySet(); + private Set> excludedClasses = emptySet(); + private Set excludedPackageNamePatterns = emptySet(); + private Set excludedPackageNames = emptySet(); + private Set> excludedPackageExemptClasses = emptySet(); private boolean disallowProxyMemberAccess; /** @@ -60,6 +64,7 @@ public class SecurityMemberAccess implements MemberAccess { */ public SecurityMemberAccess(boolean allowStaticFieldAccess) { this.allowStaticFieldAccess = allowStaticFieldAccess; + useExcludedClasses(excludedClasses); // Initialise default exclusions } @Override @@ -237,18 +242,15 @@ public class SecurityMemberAccess implements MemberAccess { 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 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; } } @@ -256,14 +258,11 @@ public class SecurityMemberAccess implements MemberAccess { } 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) { @@ -328,11 +327,16 @@ public class SecurityMemberAccess implements MemberAccess { */ @Deprecated public void setExcludedClasses(Set> excludedClasses) { - this.excludedClasses = excludedClasses; + useExcludedClasses(excludedClasses); } public void useExcludedClasses(Set> excludedClasses) { - this.excludedClasses = excludedClasses; + Set> newExcludedClasses = new HashSet<>(excludedClasses); + newExcludedClasses.add(Object.class); + if (!allowStaticFieldAccess) { + newExcludedClasses.add(Class.class); + } + this.excludedClasses = unmodifiableSet(newExcludedClasses); } /** diff --git a/core/src/main/resources/struts-excluded-classes.xml b/core/src/main/resources/struts-excluded-classes.xml index 6e4957273..72df88c89 100644 --- a/core/src/main/resources/struts-excluded-classes.xml +++ b/core/src/main/resources/struts-excluded-classes.xml @@ -55,52 +55,52 @@ - + + 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"/> diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java index 5a5bc6ae0..1a6d9697d 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -178,23 +178,6 @@ public class SecurityMemberAccessTest { 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> 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 @@ -790,7 +773,7 @@ public class SecurityMemberAccessTest { @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);