From 5fcee890d593d6eb5034e836bb3cf9e565d7061d Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Feb 2023 00:34:42 +1100 Subject: [PATCH] WW-5268 Implement ability to specify exempt classes for package exclusions --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 49 +++++++- .../xwork2/ognl/OgnlValueStack.java | 8 +- .../xwork2/ognl/SecurityMemberAccess.java | 30 +++-- .../org/apache/struts2/StrutsConstants.java | 3 + .../xwork2/ognl/SecurityMemberAccessTest.java | 107 ++++++++++++++++-- 5 files changed, 174 insertions(+), 23 deletions(-) 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 c5430491b..827cda11a 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -26,7 +26,13 @@ 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.*; +import ognl.ClassResolver; +import ognl.Ognl; +import ognl.OgnlContext; +import ognl.OgnlException; +import ognl.OgnlRuntime; +import ognl.SimpleNode; +import ognl.TypeConverter; import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -37,7 +43,12 @@ import java.beans.IntrospectionException; import java.beans.Introspector; import java.beans.PropertyDescriptor; import java.lang.reflect.Method; -import java.util.*; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.regex.Pattern; @@ -66,10 +77,12 @@ public class OgnlUtil { private Set> excludedClasses; private Set excludedPackageNamePatterns; private Set excludedPackageNames; + private Set> excludedPackageExemptClasses; private Set> devModeExcludedClasses; private Set devModeExcludedPackageNamePatterns; private Set devModeExcludedPackageNames; + private Set> devModeExcludedPackageExemptClasses; private Container container; private boolean allowStaticFieldAccess = true; @@ -112,10 +125,12 @@ public class OgnlUtil { excludedClasses = Collections.unmodifiableSet(new HashSet<>()); excludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>()); excludedPackageNames = Collections.unmodifiableSet(new HashSet<>()); + excludedPackageExemptClasses = Collections.unmodifiableSet(new HashSet<>()); devModeExcludedClasses = Collections.unmodifiableSet(new HashSet<>()); devModeExcludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>()); devModeExcludedPackageNames = Collections.unmodifiableSet(new HashSet<>()); + devModeExcludedPackageExemptClasses = Collections.unmodifiableSet(new HashSet<>()); this.expressionCache = ognlExpressionCacheFactory.buildOgnlCache(); this.beanInfoCache = ognlBeanInfoCacheFactory.buildOgnlCache(); @@ -159,7 +174,7 @@ public class OgnlUtil { protected void setExcludedClasses(String commaDelimitedClasses) { Set> excludedClasses = new HashSet<>(); excludedClasses.addAll(this.excludedClasses); - excludedClasses.addAll(parseExcludedClasses(commaDelimitedClasses)); + excludedClasses.addAll(parseClasses(commaDelimitedClasses)); this.excludedClasses = Collections.unmodifiableSet(excludedClasses); } @@ -167,11 +182,11 @@ public class OgnlUtil { protected void setDevModeExcludedClasses(String commaDelimitedClasses) { Set> excludedClasses = new HashSet<>(); excludedClasses.addAll(this.devModeExcludedClasses); - excludedClasses.addAll(parseExcludedClasses(commaDelimitedClasses)); + excludedClasses.addAll(parseClasses(commaDelimitedClasses)); this.devModeExcludedClasses = Collections.unmodifiableSet(excludedClasses); } - private Set> parseExcludedClasses(String commaDelimitedClasses) { + private Set> parseClasses(String commaDelimitedClasses) { Set classNames = TextParseUtil.commaDelimitedStringToSet(commaDelimitedClasses); Set> classes = new HashSet<>(); @@ -179,7 +194,7 @@ public class OgnlUtil { try { classes.add(Class.forName(className)); } catch (ClassNotFoundException e) { - throw new ConfigurationException("Cannot load excluded class: " + className, e); + throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e); } } @@ -229,6 +244,22 @@ public class OgnlUtil { this.devModeExcludedPackageNames = Collections.unmodifiableSet(excludedPackageNames); } + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + public void setExcludedPackageExemptClasses(String commaDelimitedClasses) { + Set> excludedPackageExemptClasses = new HashSet<>(); + excludedPackageExemptClasses.addAll(this.excludedPackageExemptClasses); + excludedPackageExemptClasses.addAll(parseClasses(commaDelimitedClasses)); + this.excludedPackageExemptClasses = Collections.unmodifiableSet(excludedPackageExemptClasses); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { + Set> excludedPackageExemptClasses = new HashSet<>(); + excludedPackageExemptClasses.addAll(this.devModeExcludedPackageExemptClasses); + excludedPackageExemptClasses.addAll(parseClasses(commaDelimitedClasses)); + this.devModeExcludedPackageExemptClasses = Collections.unmodifiableSet(excludedPackageExemptClasses); + } + private Set parseExcludedPackageNames(String commaDelimitedPackageNames) { return TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackageNames); } @@ -245,6 +276,10 @@ public class OgnlUtil { return excludedPackageNames; } + public Set> getExcludedPackageExemptClasses() { + return excludedPackageExemptClasses; + } + @Inject protected void setContainer(Container container) { this.container = container; @@ -847,10 +882,12 @@ public class OgnlUtil { memberAccess.setExcludedClasses(devModeExcludedClasses); memberAccess.setExcludedPackageNamePatterns(devModeExcludedPackageNamePatterns); memberAccess.setExcludedPackageNames(devModeExcludedPackageNames); + memberAccess.setExcludedPackageExemptClasses(devModeExcludedPackageExemptClasses); } else { memberAccess.setExcludedClasses(excludedClasses); memberAccess.setExcludedPackageNamePatterns(excludedPackageNamePatterns); memberAccess.setExcludedPackageNames(excludedPackageNames); + memberAccess.setExcludedPackageExemptClasses(excludedPackageExemptClasses); } return Ognl.createDefaultContext(root, memberAccess, resolver, defaultConverter); diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 1b874fc44..938ed477a 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -29,7 +29,12 @@ import com.opensymphony.xwork2.util.CompoundRoot; import com.opensymphony.xwork2.util.MemberAccessValueStack; import com.opensymphony.xwork2.util.ValueStack; import com.opensymphony.xwork2.util.reflection.ReflectionContextState; -import ognl.*; +import ognl.MethodFailedException; +import ognl.NoSuchPropertyException; +import ognl.Ognl; +import ognl.OgnlContext; +import ognl.OgnlException; +import ognl.PropertyAccessor; import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -87,6 +92,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS securityMemberAccess.setExcludedClasses(ognlUtil.getExcludedClasses()); securityMemberAccess.setExcludedPackageNamePatterns(ognlUtil.getExcludedPackageNamePatterns()); securityMemberAccess.setExcludedPackageNames(ognlUtil.getExcludedPackageNames()); + securityMemberAccess.setExcludedPackageExemptClasses(ognlUtil.getExcludedPackageExemptClasses()); securityMemberAccess.setDisallowProxyMemberAccess(ognlUtil.isDisallowProxyMemberAccess()); } 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 6dcdb5a1b..0b03f9226 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -47,6 +47,7 @@ public class SecurityMemberAccess implements MemberAccess { private Set> excludedClasses = Collections.emptySet(); private Set excludedPackageNamePatterns = Collections.emptySet(); private Set excludedPackageNames = Collections.emptySet(); + private Set> excludedPackageExemptClasses = Collections.emptySet(); private boolean disallowProxyMemberAccess; /** @@ -117,23 +118,23 @@ public class SecurityMemberAccess implements MemberAccess { } final Class memberClass = member.getDeclaringClass(); + // target can be null in case of accessing static fields, since OGNL 3.2.8 + final Class targetClass = Modifier.isStatic(memberModifiers) ? memberClass : target.getClass(); if (isClassExcluded(memberClass)) { LOG.warn("Declaring class of member type [{}] is excluded!", member); return false; } - // target can be null in case of accessing static fields, since OGNL 3.2.8 - final Class targetClass = Modifier.isStatic(memberModifiers) ? memberClass : target.getClass(); - - if (isPackageExcluded(targetClass.getPackage(), memberClass.getPackage())) { - LOG.warn("Package [{}] of target class [{}] of target [{}] or package [{}] of member [{}] are excluded!", targetClass.getPackage(), targetClass, - target, memberClass.getPackage(), member); + if (isClassExcluded(targetClass)) { + LOG.warn("Target class [{}] of target [{}] is excluded!", targetClass, target); return false; } - if (isClassExcluded(targetClass)) { - LOG.warn("Target class [{}] of target [{}] is excluded!", targetClass, target); + if (!isClassExcludedPackageExempt(targetClass) && !isClassExcludedPackageExempt(memberClass) + && isPackageExcluded(targetClass.getPackage(), memberClass.getPackage())) { + LOG.warn("Package [{}] of target class [{}] of target [{}] or package [{}] of member [{}] are excluded!", + targetClass.getPackage(), targetClass, target, memberClass.getPackage(), member); return false; } @@ -234,6 +235,15 @@ public class SecurityMemberAccess implements MemberAccess { return false; } + protected boolean isClassExcludedPackageExempt(Class clazz) { + for (Class excludedPackageExemptClass : excludedPackageExemptClasses) { + if (clazz.isAssignableFrom(excludedPackageExemptClass)) { + return true; + } + } + return false; + } + protected boolean isAcceptableProperty(String name) { return name == null || ((!isExcluded(name)) && isAccepted(name)); } @@ -287,6 +297,10 @@ public class SecurityMemberAccess implements MemberAccess { this.excludedPackageNames = excludedPackageNames; } + public void setExcludedPackageExemptClasses(Set> excludedPackageExemptClasses) { + this.excludedPackageExemptClasses = excludedPackageExemptClasses; + } + public void setDisallowProxyMemberAccess(boolean disallowProxyMemberAccess) { this.disallowProxyMemberAccess = disallowProxyMemberAccess; } diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 9a1920f02..8d44b9837 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -414,11 +414,14 @@ public final class StrutsConstants { public static final String STRUTS_EXCLUDED_CLASSES = "struts.excludedClasses"; public static final String STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS = "struts.excludedPackageNamePatterns"; public static final String STRUTS_EXCLUDED_PACKAGE_NAMES = "struts.excludedPackageNames"; + public static final String STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES = "struts.excludedPackageExemptClasses"; /** Comma delimited set of excluded classes and package names which cannot be accessed via expressions in devMode */ public static final String STRUTS_DEV_MODE_EXCLUDED_CLASSES = "struts.devMode.excludedClasses"; public static final String STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS = "struts.devMode.excludedPackageNamePatterns"; public static final String STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES = "struts.devMode.excludedPackageNames"; + public static final String STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES = "struts.devMode.excludedPackageExemptClasses"; + /** Dedicated services to check if passed string is excluded/accepted */ public static final String STRUTS_EXCLUDED_PATTERNS_CHECKER = "struts.excludedPatterns.checker"; 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 19abfae09..6bb9dcf94 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -47,7 +47,7 @@ public class SecurityMemberAccessTest extends TestCase { SecurityMemberAccess sma = new SecurityMemberAccess(true); String propertyName = "stringField"; - Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + Member member = FooBar.class.getMethod(formGetterName(propertyName)); // when boolean accessible = sma.isAccessible(context, target, member, propertyName); @@ -61,7 +61,7 @@ public class SecurityMemberAccessTest extends TestCase { SecurityMemberAccess sma = new SecurityMemberAccess(true); String propertyName = "stringField"; - Member member = FooBar.class.getDeclaredMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + Member member = FooBar.class.getDeclaredMethod(formGetterName(propertyName)); Set> excluded = new HashSet<>(); excluded.add(FooBar.class); @@ -138,6 +138,24 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("fooLogic() from FooInterface isn't accessible!!!", accessible); } + public void testMiddleOfInheritanceExclusion2() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(true); + + String propertyName = "barLogic"; + Member member = BarInterface.class.getMethod(propertyName); + + Set> excluded = new HashSet<>(); + excluded.add(BarInterface.class); + sma.setExcludedClasses(excluded); + + // when + boolean accessible = sma.isAccessible(context, target, member, propertyName); + + // then + assertFalse("barLogic() from BarInterface is accessible!!!", accessible); + } + public void testMiddleOfInheritanceExclusion3() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -145,6 +163,10 @@ public class SecurityMemberAccessTest extends TestCase { String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); + Set> excluded = new HashSet<>(); + excluded.add(FooInterface.class); + sma.setExcludedClasses(excluded); + // when boolean accessible = sma.isAccessible(context, target, member, propertyName); @@ -179,7 +201,7 @@ public class SecurityMemberAccessTest extends TestCase { sma.setExcludedPackageNamePatterns(excluded); String propertyName = "stringField"; - Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + Member member = FooBar.class.getMethod(formGetterName(propertyName)); // when boolean actual = sma.isAccessible(context, target, member, propertyName); @@ -188,6 +210,28 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("stringField is accessible!", actual); } + public void testPackageExclusionExemption() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + Set excluded = new HashSet<>(); + excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); + sma.setExcludedPackageNamePatterns(excluded); + + Set> allowed = new HashSet<>(); + allowed.add(FooBar.class); + sma.setExcludedPackageExemptClasses(allowed); + + String propertyName = "stringField"; + Member member = FooBar.class.getMethod(formGetterName(propertyName)); + + // when + boolean actual = sma.isAccessible(context, target, member, propertyName); + + // then + assertTrue("stringField isn't accessible!", actual); + } + public void testPackageNameExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -197,7 +241,7 @@ public class SecurityMemberAccessTest extends TestCase { sma.setExcludedPackageNames(excluded); String propertyName = "stringField"; - Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + Member member = FooBar.class.getMethod(formGetterName(propertyName)); // when boolean actual = sma.isAccessible(context, target, member, propertyName); @@ -206,6 +250,51 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("stringField is accessible!", actual); } + + public void testPackageNameExclusionExemption() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + Set excluded = new HashSet<>(); + excluded.add(FooBar.class.getPackage().getName()); + sma.setExcludedPackageNames(excluded); + + Set> allowed = new HashSet<>(); + allowed.add(FooBar.class); + sma.setExcludedPackageExemptClasses(allowed); + + String propertyName = "stringField"; + Member member = FooBar.class.getMethod(formGetterName(propertyName)); + + // when + boolean actual = sma.isAccessible(context, target, member, propertyName); + + // then + assertTrue("stringField isn't accessible!", actual); + } + + public void testPackageNameExclusionExemptionInheritance() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + Set excluded = new HashSet<>(); + excluded.add(FooBar.class.getPackage().getName()); + sma.setExcludedPackageNames(excluded); + + Set> allowed = new HashSet<>(); + allowed.add(BarInterface.class); + sma.setExcludedPackageExemptClasses(allowed); + + String propertyName = "barLogic"; + Member member = BarInterface.class.getMethod(propertyName); + + // when + boolean actual = sma.isAccessible(context, target, member, propertyName); + + // then + assertTrue("barLogic isn't accessible!", actual); + } + public void testDefaultPackageExclusion() { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -429,7 +518,7 @@ public class SecurityMemberAccessTest extends TestCase { sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang.,ognl,javax")); String propertyName = "intField"; - Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + Member member = FooBar.class.getMethod(formGetterName(propertyName)); // when boolean accessible = sma.isAccessible(context, target, member, propertyName); @@ -473,7 +562,7 @@ public class SecurityMemberAccessTest extends TestCase { // given propertyName = "intField"; - member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + member = FooBar.class.getMethod(formGetterName(propertyName)); // when accessible = sma.isAccessible(context, target, member, propertyName); @@ -482,7 +571,7 @@ public class SecurityMemberAccessTest extends TestCase { // given propertyName = "doubleField"; - member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); + member = FooBar.class.getMethod(formGetterName(propertyName)); // when accessible = sma.isAccessible(context, target, member, propertyName); @@ -547,7 +636,6 @@ public class SecurityMemberAccessTest extends TestCase { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang.")); // when @@ -558,6 +646,9 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("package java.lang. is accessible!", actual); } + private static String formGetterName(String propertyName) { + return "get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1); + } } class FooBar implements FooBarInterface {