From bb68ce6ae5a3cff8f35844f4533c4007370c30e8 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:31:31 +1000 Subject: [PATCH 01/11] WW-5337 Catch PatternSyntaxException and ensure ConfigurationException thrown --- .../com/opensymphony/xwork2/ognl/OgnlUtil.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 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 c6169afa9..be738be93 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -51,6 +51,9 @@ 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; /** @@ -218,14 +221,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) From 393eb8c0f89f802dffb7c6d0d20da4bdfd73fa82 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:32:04 +1000 Subject: [PATCH 02/11] WW-5337 Minor clean up OgnlUtil --- .../src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 5 +---- 1 file changed, 1 insertion(+), 4 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 be738be93..21ebd5fea 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; @@ -190,9 +189,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)); @@ -200,7 +198,6 @@ public class OgnlUtil { throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e); } } - return classes; } From 841705cadfd38b5460915375b5e977131f3988eb Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:33:30 +1000 Subject: [PATCH 03/11] WW-5337 Strip trailing periods from package names provided as not needed --- .../src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 21ebd5fea..005d17eba 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -53,6 +53,8 @@ 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; /** @@ -260,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); } From 4145d1af14a1b39dd0970159171f24058d845834 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:33:52 +1000 Subject: [PATCH 04/11] WW-5337 Make #isExcludedPackageNamePatterns more succinct --- .../com/opensymphony/xwork2/ognl/SecurityMemberAccess.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) 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..d90e58308 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -237,12 +237,7 @@ 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) { From e53dd7dd5e8f0f8d9aa3244180cb6394bb00ed07 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:34:47 +1000 Subject: [PATCH 05/11] WW-5337 Make #isClassExcluded (semantics changes) and #isExcludedPackageExempt constant time --- .../com/opensymphony/xwork2/ognl/SecurityMemberAccess.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 d90e58308..3b870dedd 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -251,14 +251,14 @@ public class SecurityMemberAccess implements MemberAccess { } protected boolean isClassExcluded(Class clazz) { - if (clazz == Object.class || (clazz == Class.class && !allowStaticFieldAccess)) { + if (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) { From 746c7541326ba809f16bceef8fea4215793b5d62 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:37:32 +1000 Subject: [PATCH 06/11] WW-5337 Make #isExcludedPackageNames runtime proportional to no. of package parts rather than no. of excluded packages --- .../opensymphony/xwork2/ognl/SecurityMemberAccess.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) 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 3b870dedd..29d15ebf9 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -27,8 +27,10 @@ import java.lang.reflect.AccessibleObject; import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Modifier; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.regex.Matcher; @@ -241,9 +243,11 @@ public class SecurityMemberAccess implements MemberAccess { } 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; } } From 72a5d213327ab612029e5f58d1131ae116098c1f Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:38:15 +1000 Subject: [PATCH 07/11] WW-5337 Update struts-excluded-classes.xml to not have trailing periods --- .../resources/struts-excluded-classes.xml | 84 +++++++++---------- 1 file changed, 42 insertions(+), 42 deletions(-) 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"/> From b0f1ef1f8b97a7fc5df129949472d01b1f02cf1d Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:58:24 +1000 Subject: [PATCH 08/11] WW-5337 Revert Object special handling --- .../java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 29d15ebf9..01be7a0c4 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -255,7 +255,7 @@ public class SecurityMemberAccess implements MemberAccess { } protected boolean isClassExcluded(Class clazz) { - if (clazz == Class.class && !allowStaticFieldAccess) { + if (clazz == Object.class || clazz == Class.class && !allowStaticFieldAccess) { return true; } return excludedClasses.contains(clazz); From a228b14a487514caffa7b6850516a9d4e100f6fd Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:58:42 +1000 Subject: [PATCH 09/11] WW-5337 Drop superinterface/superclass banning test --- .../xwork2/ognl/SecurityMemberAccessTest.java | 17 ----------------- 1 file changed, 17 deletions(-) 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..70b144a1d 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 From 270ec4ad71a2fdbc41e2b01ad59615f31eebc2a9 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 23:59:00 +1000 Subject: [PATCH 10/11] WW-5337 Fix #testPackageNameExclusionAsCommaDelimited --- .../com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 70b144a1d..1a6d9697d 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -773,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); From 783063c66f0ba759e22850cba8b9c8d09ae137a5 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 22 Aug 2023 00:29:03 +1000 Subject: [PATCH 11/11] WW-5337 Initialise default exclusions one-time in SecurityMemberAccess (more performant) --- .../xwork2/ognl/SecurityMemberAccess.java | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) 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 01be7a0c4..5db52639e 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -28,7 +28,6 @@ import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Modifier; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -36,6 +35,9 @@ 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. @@ -45,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; /** @@ -62,6 +64,7 @@ public class SecurityMemberAccess implements MemberAccess { */ public SecurityMemberAccess(boolean allowStaticFieldAccess) { this.allowStaticFieldAccess = allowStaticFieldAccess; + useExcludedClasses(excludedClasses); // Initialise default exclusions } @Override @@ -255,9 +258,6 @@ public class SecurityMemberAccess implements MemberAccess { } protected boolean isClassExcluded(Class clazz) { - if (clazz == Object.class || clazz == Class.class && !allowStaticFieldAccess) { - return true; - } return excludedClasses.contains(clazz); } @@ -327,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); } /**