From ff19dfee6f99f1650fc0c2484407d4ed75c2635e Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 1 Mar 2023 23:56:15 +1100 Subject: [PATCH] WW-5288 Make excluded package exemption logic more strict --- .../xwork2/ognl/SecurityMemberAccess.java | 87 ++- .../org/apache/struts2/StrutsConstants.java | 5 +- .../conversion/impl/StringConverterTest.java | 2 - .../xwork2/ognl/SecurityMemberAccessTest.java | 96 ++- .../com/test/SecurityMemberAccessTest.java | 651 ------------------ .../com/test/TestSecurityMemberAccess.java | 33 - .../SecurityMemberAccessInServletsTest.java | 8 +- 7 files changed, 142 insertions(+), 740 deletions(-) delete mode 100644 core/src/test/java/com/test/SecurityMemberAccessTest.java delete mode 100644 core/src/test/java/com/test/TestSecurityMemberAccess.java 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 0b03f9226..384d6cf24 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -28,6 +28,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Modifier; import java.util.Collections; +import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.regex.Matcher; @@ -95,6 +96,12 @@ public class SecurityMemberAccess implements MemberAccess { LOG.debug("Checking access for [target: {}, member: {}, property: {}]", target, member, propertyName); final int memberModifiers = member.getModifiers(); + 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 (!memberClass.isAssignableFrom(targetClass)) { + throw new IllegalArgumentException("Target does not match member!"); + } if (!checkPublicMemberAccess(memberModifiers)) { LOG.warn("Access to non-public [{}] is blocked!", member); @@ -117,10 +124,6 @@ public class SecurityMemberAccess implements MemberAccess { return false; } - 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; @@ -131,10 +134,18 @@ public class SecurityMemberAccess implements MemberAccess { return false; } - 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); + if (targetClass.getPackage() == null || memberClass.getPackage() == null) { + LOG.warn("The use of the default (unnamed) package is discouraged!"); + } + + if (isPackageExcluded(targetClass, memberClass)) { + LOG.warn( + "Package [{}] of target class [{}] of target [{}] or package [{}] of member [{}] are excluded!", + targetClass.getPackage(), + targetClass, + target, + memberClass.getPackage(), + member); return false; } @@ -197,29 +208,49 @@ public class SecurityMemberAccess implements MemberAccess { return false; } - protected boolean isPackageExcluded(Package targetPackage, Package memberPackage) { - if (targetPackage == null || memberPackage == null) { - LOG.warn("The use of the default (unnamed) package is discouraged!"); + protected boolean isPackageExcluded(Class targetClass, Class memberClass) { + if (targetClass == null || memberClass == null) { + throw new IllegalArgumentException( + "Parameters should never be null - if member is static, targetClass should be the same as memberClass."); } - String targetPackageName = targetPackage == null ? "" : targetPackage.getName(); - String memberPackageName = memberPackage == null ? "" : memberPackage.getName(); + Set> classesToCheck = new HashSet<>(); + classesToCheck.add(targetClass); + classesToCheck.add(memberClass); + for (Class clazz : classesToCheck) { + if (!isExcludedPackageExempt(clazz) && (isExcludedPackageNamePatterns(clazz) || isExcludedPackageNames(clazz))) { + return true; + } + } + return false; + } + + protected String toPackageName(Class clazz) { + if (clazz.getPackage() == null) { + return ""; + } else { + return clazz.getPackage().getName(); + } + } + + protected boolean isExcludedPackageNamePatterns(Class clazz) { + String packageName = toPackageName(clazz); for (Pattern pattern : excludedPackageNamePatterns) { - if (pattern.matcher(targetPackageName).matches() || pattern.matcher(memberPackageName).matches()) { + if (pattern.matcher(packageName).matches()) { return true; } } + return false; + } - targetPackageName = targetPackageName + "."; - memberPackageName = memberPackageName + "."; - - for (String packageName : excludedPackageNames) { - if (targetPackageName.startsWith(packageName) || memberPackageName.startsWith(packageName)) { + protected boolean isExcludedPackageNames(Class clazz) { + String suffixedPackageName = toPackageName(clazz) + "."; + for (String excludedPackageName : excludedPackageNames) { + if (suffixedPackageName.startsWith(excludedPackageName)) { return true; } } - return false; } @@ -227,21 +258,11 @@ public class SecurityMemberAccess implements MemberAccess { if (clazz == Object.class || (clazz == Class.class && !allowStaticFieldAccess)) { return true; } - for (Class excludedClass : excludedClasses) { - if (clazz.isAssignableFrom(excludedClass)) { - return true; - } - } - return false; + return excludedClasses.stream().anyMatch(clazz::isAssignableFrom); } - protected boolean isClassExcludedPackageExempt(Class clazz) { - for (Class excludedPackageExemptClass : excludedPackageExemptClasses) { - if (clazz.isAssignableFrom(excludedPackageExemptClass)) { - return true; - } - } - return false; + protected boolean isExcludedPackageExempt(Class clazz) { + return excludedPackageExemptClasses.stream().anyMatch(clazz::equals); } protected boolean isAcceptableProperty(String name) { diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index dc5720b71..c842986cd 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -413,10 +413,13 @@ public final class StrutsConstants { /** Allows override default DispatcherErrorHandler */ public static final String STRUTS_DISPATCHER_ERROR_HANDLER = "struts.dispatcher.errorHandler"; - /** Comma delimited set of excluded classes and package names which cannot be accessed via expressions */ + /** Comma delimited set of excluded classes which cannot be accessed via OGNL expressions. Matching is done on both target and member classes of OGNL expression. Note that superclasses of listed classes are also used for matching. */ public static final String STRUTS_EXCLUDED_CLASSES = "struts.excludedClasses"; + /** Comma delimited set of RegEx to match against package names of target and member classes of OGNL expressions. If matched, they cannot be accessed. */ public static final String STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS = "struts.excludedPackageNamePatterns"; + /** Comma delimited set of package names, of which all its classes, and all classes in its subpackages, cannot be accessed via OGNL expressions. Matching is done on both target and member classes of OGNL expression. */ public static final String STRUTS_EXCLUDED_PACKAGE_NAMES = "struts.excludedPackageNames"; + /** Comma delimited set of exempt classes from matching against excludedPackageNames and excludedPackageNamePatterns. As matching for excluded packages is done on both target and member classes of OGNL expression, an exemption must exist for each match. */ 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 */ diff --git a/core/src/test/java/com/opensymphony/xwork2/conversion/impl/StringConverterTest.java b/core/src/test/java/com/opensymphony/xwork2/conversion/impl/StringConverterTest.java index 8eef6d1ab..92d0814d6 100644 --- a/core/src/test/java/com/opensymphony/xwork2/conversion/impl/StringConverterTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/conversion/impl/StringConverterTest.java @@ -18,12 +18,10 @@ */ package com.opensymphony.xwork2.conversion.impl; -import com.opensymphony.xwork2.ActionContext; import org.apache.commons.lang3.StringUtils; import org.apache.struts2.StrutsInternalTestCase; import java.math.BigDecimal; -import java.util.HashMap; import java.util.Locale; import java.util.Map; 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 6bb9dcf94..fe956ebba 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -19,7 +19,8 @@ package com.opensymphony.xwork2.ognl; import com.opensymphony.xwork2.util.TextParseUtil; -import junit.framework.TestCase; +import org.junit.Before; +import org.junit.Test; import java.lang.reflect.Field; import java.lang.reflect.Member; @@ -31,17 +32,21 @@ import java.util.Map; import java.util.Set; import java.util.regex.Pattern; -public class SecurityMemberAccessTest extends TestCase { +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class SecurityMemberAccessTest { private Map context; private FooBar target; - @Override + @Before public void setUp() throws Exception { - context = new HashMap(); + context = new HashMap<>(); target = new FooBar(); } + @Test public void testWithoutClassExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -56,6 +61,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue(accessible); } + @Test public void testClassExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -74,6 +80,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse(accessible); } + @Test public void testObjectClassExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -88,6 +95,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("toString() from Object is accessible!!!", accessible); } + @Test public void testObjectOverwrittenMethodsExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -102,6 +110,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("hashCode() from FooBar isn't accessible!!!", accessible); } + @Test public void testInterfaceInheritanceExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -120,6 +129,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("barLogic() from BarInterface is accessible!!!", accessible); } + @Test public void testMiddleOfInheritanceExclusion1() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -138,6 +148,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("fooLogic() from FooInterface isn't accessible!!!", accessible); } + @Test public void testMiddleOfInheritanceExclusion2() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -156,6 +167,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("barLogic() from BarInterface is accessible!!!", accessible); } + @Test public void testMiddleOfInheritanceExclusion3() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -174,6 +186,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("barLogic() from BarInterface isn't accessible!!!", accessible); } + @Test public void testMiddleOfInheritanceExclusion4() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -192,6 +205,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("barLogic() from BarInterface is accessible!!!", accessible); } + @Test public void testPackageExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -210,6 +224,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("stringField is accessible!", actual); } + @Test public void testPackageExclusionExemption() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(false); @@ -232,6 +247,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("stringField isn't accessible!", actual); } + @Test public void testPackageNameExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -250,7 +266,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("stringField is accessible!", actual); } - + @Test public void testPackageNameExclusionExemption() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(false); @@ -273,7 +289,8 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("stringField isn't accessible!", actual); } - public void testPackageNameExclusionExemptionInheritance() throws Exception { + @Test + public void testPackageNameExclusionExemption2() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(false); @@ -281,6 +298,7 @@ public class SecurityMemberAccessTest extends TestCase { excluded.add(FooBar.class.getPackage().getName()); sma.setExcludedPackageNames(excluded); + // Exemption must exist for both classes (target and member) if they both match a banned package Set> allowed = new HashSet<>(); allowed.add(BarInterface.class); sma.setExcludedPackageExemptClasses(allowed); @@ -291,11 +309,37 @@ public class SecurityMemberAccessTest extends TestCase { // when boolean actual = sma.isAccessible(context, target, member, propertyName); + // then + assertFalse("barLogic is accessible!", actual); + } + + @Test + public void testPackageNameExclusionExemption3() throws Exception { + // given + SecurityMemberAccess sma = new SecurityMemberAccess(false); + + Set excluded = new HashSet<>(); + excluded.add(FooBar.class.getPackage().getName()); + sma.setExcludedPackageNames(excluded); + + // Exemption must exist for both classes (target and member) if they both match a banned package + Set> allowed = new HashSet<>(); + allowed.add(BarInterface.class); + allowed.add(FooBar.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() { + @Test + public void testDefaultPackageExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -303,14 +347,17 @@ public class SecurityMemberAccessTest extends TestCase { excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); sma.setExcludedPackageNamePatterns(excluded); + Class clazz = Class.forName("PackagelessAction"); + // when - boolean actual = sma.isPackageExcluded(null, null); + boolean actual = sma.isPackageExcluded(clazz, clazz); // then assertFalse("default package is excluded!", actual); } - public void testDefaultPackageExclusion2() { + @Test + public void testDefaultPackageExclusion2() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -318,13 +365,16 @@ public class SecurityMemberAccessTest extends TestCase { excluded.add(Pattern.compile("^$")); sma.setExcludedPackageNamePatterns(excluded); + Class clazz = Class.forName("PackagelessAction"); + // when - boolean actual = sma.isPackageExcluded(null, null); + boolean actual = sma.isPackageExcluded(clazz, clazz); // then assertTrue("default package isn't excluded!", actual); } + @Test public void testAccessEnum() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -337,6 +387,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("Access to enums is blocked!", actual); } + @Test public void testAccessStaticMethod() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -350,6 +401,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("Access to static method is not blocked!", actual); } + @Test public void testAccessStaticField() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -363,6 +415,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("Access to static field is blocked!", actual); } + @Test public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -460,6 +513,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("Access to private final static field is allowed?", actual); } + @Test public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -473,6 +527,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("Access to static field isn't blocked!", actual); } + @Test public void testBlockStaticMethodAccess() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -486,6 +541,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("Access to static isn't blocked!", actual); } + @Test public void testBlockStaticAccessIfClassIsExcluded() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -499,6 +555,7 @@ public class SecurityMemberAccessTest extends TestCase { assertFalse("Access to static method of excluded class isn't blocked!", actual); } + @Test public void testAllowStaticAccessIfClassIsNotExcluded() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -512,6 +569,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue("Invalid test! Access to static method of excluded class is blocked!", actual); } + @Test public void testAccessPrimitiveInt() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -527,6 +585,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue(accessible); } + @Test public void testAccessPrimitiveDoubleWithNames() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -542,10 +601,11 @@ public class SecurityMemberAccessTest extends TestCase { sma.setExcludedClasses(excluded); String propertyName = "doubleValue"; + double myDouble = 1; Member member = Double.class.getMethod(propertyName); // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); + boolean accessible = sma.isAccessible(context, myDouble, member, propertyName); // then assertTrue(accessible); @@ -579,6 +639,7 @@ public class SecurityMemberAccessTest extends TestCase { assertTrue(accessible); } + @Test public void testAccessPrimitiveDoubleWithPackageRegExs() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -587,15 +648,17 @@ public class SecurityMemberAccessTest extends TestCase { sma.setExcludedPackageNamePatterns(patterns); String propertyName = "doubleValue"; + double myDouble = 1; Member member = Double.class.getMethod(propertyName); // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); + boolean accessible = sma.isAccessible(context, myDouble, member, propertyName); // then assertTrue(accessible); } + @Test public void testAccessMemberAccessIsAccessible() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -608,12 +671,13 @@ public class SecurityMemberAccessTest extends TestCase { Member member = SecurityMemberAccess.class.getMethod(setter, Set.class); // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); + boolean accessible = sma.isAccessible(context, sma, member, propertyName); // then assertTrue(accessible); } + @Test public void testAccessMemberAccessIsBlocked() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -626,12 +690,13 @@ public class SecurityMemberAccessTest extends TestCase { Member member = SecurityMemberAccess.class.getMethod(setter, Set.class); // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); + boolean accessible = sma.isAccessible(context, sma, member, propertyName); // then assertFalse(accessible); } + @Test public void testPackageNameExclusionAsCommaDelimited() { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); @@ -639,8 +704,7 @@ public class SecurityMemberAccessTest extends TestCase { sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang.")); // when - boolean actual = sma.isPackageExcluded(String.class.getPackage(), null); - actual &= sma.isPackageExcluded(null, String.class.getPackage()); + boolean actual = sma.isPackageExcluded(String.class, String.class); // then assertTrue("package java.lang. is accessible!", actual); diff --git a/core/src/test/java/com/test/SecurityMemberAccessTest.java b/core/src/test/java/com/test/SecurityMemberAccessTest.java deleted file mode 100644 index e823cda72..000000000 --- a/core/src/test/java/com/test/SecurityMemberAccessTest.java +++ /dev/null @@ -1,651 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package com.test; - -import com.opensymphony.xwork2.ognl.SecurityMemberAccess; -import com.opensymphony.xwork2.util.TextParseUtil; -import junit.framework.TestCase; - -import java.lang.reflect.Field; -import java.lang.reflect.Member; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; -import java.util.regex.Pattern; - -public class SecurityMemberAccessTest extends TestCase { - - private Map context; - private FooBar target; - - @Override - public void setUp() throws Exception { - context = new HashMap(); - target = new FooBar(); - } - - public void testWithoutClassExclusion() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - String propertyName = "stringField"; - Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertTrue(accessible); - } - - public void testClassExclusion() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - String propertyName = "stringField"; - Member member = FooBar.class.getDeclaredMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); - - Set> excluded = new HashSet<>(); - excluded.add(FooBar.class); - sma.setExcludedClasses(excluded); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertFalse(accessible); - } - - public void testObjectClassExclusion() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - String propertyName = "toString"; - Member member = FooBar.class.getMethod(propertyName); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertFalse("toString() from Object is accessible!!!", accessible); - } - - public void testObjectOverwrittenMethodsExclusion() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - String propertyName = "hashCode"; - Member member = FooBar.class.getMethod(propertyName); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertTrue("hashCode() from FooBar isn't accessible!!!", accessible); - } - - public void testInterfaceInheritanceExclusion() 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 testMiddleOfInheritanceExclusion1() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - String propertyName = "fooLogic"; - Member member = FooBar.class.getMethod(propertyName); - - Set> excluded = new HashSet<>(); - excluded.add(BarInterface.class); - sma.setExcludedClasses(excluded); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertTrue("fooLogic() from FooInterface isn't accessible!!!", accessible); - } - - public void testMiddleOfInheritanceExclusion3() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - String propertyName = "barLogic"; - Member member = BarInterface.class.getMethod(propertyName); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertTrue("barLogic() from BarInterface isn't accessible!!!", accessible); - } - - public void testMiddleOfInheritanceExclusion4() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - String propertyName = "barLogic"; - Member member = BarInterface.class.getMethod(propertyName); - - Set> excluded = new HashSet<>(); - excluded.add(FooBarInterface.class); - sma.setExcludedClasses(excluded); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertFalse("barLogic() from BarInterface is accessible!!!", accessible); - } - - public void testPackageExclusion() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - Set excluded = new HashSet<>(); - excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); - sma.setExcludedPackageNamePatterns(excluded); - - String propertyName = "stringField"; - Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); - - // when - boolean actual = sma.isAccessible(context, target, member, propertyName); - - // then - assertFalse("stringField is accessible!", actual); - } - - public void testPackageNameExclusion() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - Set excluded = new HashSet<>(); - excluded.add(FooBar.class.getPackage().getName()); - sma.setExcludedPackageNames(excluded); - - String propertyName = "stringField"; - Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); - - // when - boolean actual = sma.isAccessible(context, target, member, propertyName); - - // then - assertFalse("stringField is accessible!", actual); - } - - public void testDefaultPackageExclusion() { - // given - TestSecurityMemberAccess sma = new TestSecurityMemberAccess(true); - - Set excluded = new HashSet<>(); - excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); - sma.setExcludedPackageNamePatterns(excluded); - - // when - boolean actual = sma.isPackageExcluded(null, null); - - // then - assertFalse("default package is excluded!", actual); - } - - public void testDefaultPackageExclusion2() { - // given - TestSecurityMemberAccess sma = new TestSecurityMemberAccess(true); - - Set excluded = new HashSet<>(); - excluded.add(Pattern.compile("^$")); - sma.setExcludedPackageNamePatterns(excluded); - - // when - boolean actual = sma.isPackageExcluded(null, null); - - // then - assertTrue("default package isn't excluded!", actual); - } - - public void testAccessEnum() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - // when - Member values = MyValues.class.getMethod("values"); - boolean actual = sma.isAccessible(context, MyValues.class, values, null); - - // then - assertTrue("Access to enums is blocked!", actual); - } - - public void testAccessStaticMethod() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - Member method = StaticTester.class.getMethod("sayHello"); - boolean actual = sma.isAccessible(context, Class.class, method, null); - - // then - assertFalse("Access to static method is not blocked!", actual); - } - - public void testAccessStaticField() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - Member method = StaticTester.class.getField("MAX_VALUE"); - boolean actual = sma.isAccessible(context, null, method, null); - - // then - assertTrue("Access to static field is blocked!", actual); - } - - public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - Member method = StaticTester.class.getField("MAX_VALUE"); - boolean actual = sma.isAccessible(context, null, method, null); - - // then - assertTrue("Access to public static field is blocked?", actual); - - // public static final test - // given - sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - method = StaticTester.class.getField("MIN_VALUE"); - actual = sma.isAccessible(context, null, method, null); - - // then - assertTrue("Access to public final static field is blocked?", actual); - - // package static test - // given - sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - method = StaticTester.getFieldByName("PACKAGE_STRING"); - actual = sma.isAccessible(context, null, method, null); - - // then - assertFalse("Access to package static field is allowed?", actual); - - // package final static test - // given - sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - method = StaticTester.getFieldByName("FINAL_PACKAGE_STRING"); - actual = sma.isAccessible(context, null, method, null); - - // then - assertFalse("Access to package final static field is allowed?", actual); - - // protected static test - // given - sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - method = StaticTester.getFieldByName("PROTECTED_STRING"); - actual = sma.isAccessible(context, null, method, null); - - // then - assertFalse("Access to protected static field is allowed?", actual); - - // protected final static test - // given - sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - method = StaticTester.getFieldByName("FINAL_PROTECTED_STRING"); - actual = sma.isAccessible(context, null, method, null); - - // then - assertFalse("Access to protected final static field is allowed?", actual); - - // private static test - // given - sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - method = StaticTester.getFieldByName("PRIVATE_STRING"); - actual = sma.isAccessible(context, null, method, null); - - // then - assertFalse("Access to private static field is allowed?", actual); - - // private final static test - // given - sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - method = StaticTester.getFieldByName("FINAL_PRIVATE_STRING"); - actual = sma.isAccessible(context, null, method, null); - - // then - assertFalse("Access to private final static field is allowed?", actual); - } - - public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Arrays.asList(Class.class, StaticTester.class))); - - // when - Member method = StaticTester.class.getField("MAX_VALUE"); - boolean actual = sma.isAccessible(context, null, method, null); - - // then - assertFalse("Access to static field isn't blocked!", actual); - } - - public void testBlockStaticAccess() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - Member method = StaticTester.class.getMethod("sayHello"); - boolean actual = sma.isAccessible(context, Class.class, method, null); - - // then - assertFalse("Access to static isn't blocked!", actual); - } - - public void testBlockStaticAccessIfClassIsExcluded() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); - - // when - Member method = Class.class.getMethod("getClassLoader"); - boolean actual = sma.isAccessible(context, Class.class, method, null); - - // then - assertFalse("Access to static method of excluded class isn't blocked!", actual); - } - - public void testAllowStaticAccessIfClassIsNotExcluded() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedClasses(new HashSet<>(Collections.singletonList(ClassLoader.class))); - - // when - Member method = Class.class.getMethod("getClassLoader"); - boolean actual = sma.isAccessible(context, Class.class, method, null); - - // then - assertTrue("Invalid test! Access to static method of excluded class is blocked!", actual); - } - - public void testAccessPrimitiveInt() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - 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)); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertTrue(accessible); - } - - public void testAccessPrimitiveDoubleWithNames() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("ognl.,javax.")); - - - Set> excluded = new HashSet<>(); - excluded.add(Object.class); - excluded.add(Runtime.class); - excluded.add(System.class); - excluded.add(Class.class); - excluded.add(ClassLoader.class); - sma.setExcludedClasses(excluded); - - String propertyName = "doubleValue"; - Member member = Double.class.getMethod(propertyName); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertTrue(accessible); - - // given - propertyName = "exit"; - member = System.class.getMethod(propertyName, int.class); - - // when - accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertFalse(accessible); - - // given - propertyName = "intField"; - member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); - - // when - accessible = sma.isAccessible(context, target, member, propertyName); - // then - assertTrue(accessible); - - // given - propertyName = "doubleField"; - member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); - - // when - accessible = sma.isAccessible(context, target, member, propertyName); - // then - assertTrue(accessible); - } - - public void testAccessPrimitiveDoubleWithPackageRegExs() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - Set patterns = new HashSet<>(); - patterns.add(Pattern.compile("^java\\.lang\\..*")); - sma.setExcludedPackageNamePatterns(patterns); - - String propertyName = "doubleValue"; - Member member = Double.class.getMethod(propertyName); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertTrue(accessible); - } - - public void testAccessMemberAccessIsAccessible() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - Set> excluded = new HashSet<>(); - excluded.add(ognl.MemberAccess.class); - sma.setExcludedClasses(excluded); - - String propertyName = "excludedClasses"; - String setter = "setExcludedClasses"; - Member member = SecurityMemberAccess.class.getMethod(setter, Set.class); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertTrue(accessible); - } - - public void testAccessMemberAccessIsBlocked() throws Exception { - // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); - Set> excluded = new HashSet<>(); - excluded.add(SecurityMemberAccess.class); - sma.setExcludedClasses(excluded); - - String propertyName = "excludedClasses"; - String setter = "setExcludedClasses"; - Member member = SecurityMemberAccess.class.getMethod(setter, Set.class); - - // when - boolean accessible = sma.isAccessible(context, target, member, propertyName); - - // then - assertFalse(accessible); - } - - public void testPackageNameExclusionAsCommaDelimited() { - // given - TestSecurityMemberAccess sma = new TestSecurityMemberAccess(true); - - sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang.")); - - // when - boolean actual = sma.isPackageExcluded(String.class.getPackage(), null); - actual &= sma.isPackageExcluded(null, String.class.getPackage()); - - // then - assertTrue("package java.lang. is accessible!", actual); - } - -} - -class FooBar implements FooBarInterface { - - private String stringField; - - private int intField; - - private Double doubleField; - - public String getStringField() { - return stringField; - } - - public void setStringField(String stringField) { - this.stringField = stringField; - } - - public String fooLogic() { - return "fooLogic"; - } - - public String barLogic() { - return "barLogic"; - } - - @Override - public int hashCode() { - return 1; - } - - public int getIntField() { - return intField; - } - - public void setIntField(int intField) { - this.intField = intField; - } - - public Double getDoubleField() { - return doubleField; - } - - public void setDoubleField(Double doubleField) { - this.doubleField = doubleField; - } -} - -interface FooInterface { - - String fooLogic(); - -} - -interface BarInterface { - - String barLogic(); - -} - -interface FooBarInterface extends FooInterface, BarInterface { - -} - -enum MyValues { - ONE, TWO, THREE -} - -class StaticTester { - - public static int MAX_VALUE = 0; - public static final int MIN_VALUE = 0; - static String PACKAGE_STRING = "package_string"; - static final String FINAL_PACKAGE_STRING = "final_package_string"; - static String PROTECTED_STRING = "protected_string"; - static final String FINAL_PROTECTED_STRING = "final_protected_string"; - static String PRIVATE_STRING = "private_string"; - static final String FINAL_PRIVATE_STRING = "final_private_string"; - - public static String sayHello() { - return "Hello"; - } - - protected static Field getFieldByName(String fieldName) throws NoSuchFieldException { - if (fieldName != null && fieldName.length() > 0) { - return StaticTester.class.getDeclaredField(fieldName); - } else { - throw new NoSuchFieldException("field: " + fieldName + " does not exist"); - } - } -} diff --git a/core/src/test/java/com/test/TestSecurityMemberAccess.java b/core/src/test/java/com/test/TestSecurityMemberAccess.java deleted file mode 100644 index 95d8efacd..000000000 --- a/core/src/test/java/com/test/TestSecurityMemberAccess.java +++ /dev/null @@ -1,33 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package com.test; - -import com.opensymphony.xwork2.ognl.SecurityMemberAccess; - -class TestSecurityMemberAccess extends SecurityMemberAccess { - - TestSecurityMemberAccess(boolean allowStaticFieldAccess) { - super(allowStaticFieldAccess); - } - - @Override - public boolean isPackageExcluded(Package targetPackage, Package memberPackage) { - return super.isPackageExcluded(targetPackage, memberPackage); - } -} diff --git a/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java b/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java index b5251a5e7..0e89e2a3c 100644 --- a/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java +++ b/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java @@ -20,7 +20,7 @@ package org.apache.struts2.util; import com.opensymphony.xwork2.ognl.SecurityMemberAccess; import org.apache.struts2.StrutsInternalTestCase; -import org.apache.struts2.TestAction; +import org.apache.struts2.views.jsp.ActionTag; import javax.servlet.jsp.tagext.TagSupport; import java.lang.reflect.Member; @@ -51,7 +51,7 @@ public class SecurityMemberAccessInServletsTest extends StrutsInternalTestCase { Member member = TagSupport.class.getMethod("doStartTag"); // when - boolean actual = sma.isAccessible(context, new TestAction(), member, propertyName); + boolean actual = sma.isAccessible(context, new ActionTag(), member, propertyName); // then assertTrue("javax.servlet package isn't accessible!", actual); @@ -61,7 +61,7 @@ public class SecurityMemberAccessInServletsTest extends StrutsInternalTestCase { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); - Set excluded = new HashSet(); + Set excluded = new HashSet<>(); excluded.add(Pattern.compile("^javax\\..+")); sma.setExcludedPackageNamePatterns(excluded); @@ -69,7 +69,7 @@ public class SecurityMemberAccessInServletsTest extends StrutsInternalTestCase { Member member = TagSupport.class.getMethod("doStartTag"); // when - boolean actual = sma.isAccessible(context, new TestAction(), member, propertyName); + boolean actual = sma.isAccessible(context, new ActionTag(), member, propertyName); // then assertFalse("javax.servlet package is accessible!", actual);