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 232c33e5aa..966c0522a2 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -26,6 +26,7 @@ import java.lang.reflect.AccessibleObject; import java.lang.reflect.Field; import java.lang.reflect.Member; +import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.Arrays; import java.util.HashSet; @@ -104,89 +105,135 @@ public void restore(Map context, Object target, Member member, String propertyNa public boolean isAccessible(Map context, Object target, Member member, String propertyName) { 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 (target != null) { + // Special case: Target is a Class object but not Class.class + if (Class.class.equals(target.getClass()) && !Class.class.equals(target)) { + if (!isStatic(member)) { + throw new IllegalArgumentException("Member expected to be static!"); + } + if (!member.getDeclaringClass().equals(target)) { + throw new IllegalArgumentException("Target class does not match static member!"); + } + target = null; // This information is not useful to us and conflicts with following logic which expects target to be null or an instance containing the member + // Standard case: Member should exist on target + } else if (!member.getDeclaringClass().isAssignableFrom(target.getClass())) { + throw new IllegalArgumentException("Member does not exist on target!"); + } } - if (disallowProxyMemberAccess && ProxyUtil.isProxyMember(member, target)) { - LOG.warn("Access to proxy is blocked! Target class [{}] of target [{}], member [{}]", targetClass, target, member); + if (!checkProxyMemberAccess(target, member)) { + LOG.warn("Access to proxy is blocked! Member class [{}] of target [{}], member [{}]", member.getDeclaringClass(), target, member); return false; } - if (!checkPublicMemberAccess(memberModifiers)) { + if (!checkPublicMemberAccess(member)) { LOG.warn("Access to non-public [{}] is blocked!", member); return false; } - if (!checkStaticFieldAccess(member, memberModifiers)) { + if (!checkStaticFieldAccess(member)) { LOG.warn("Access to static field [{}] is blocked!", member); return false; } - // it needs to be before calling #checkStaticMethodAccess() - if (checkEnumAccess(target, member)) { - LOG.trace("Allowing access to enum: target [{}], member [{}]", target, member); - return true; - } - - if (!checkStaticMethodAccess(member, memberModifiers)) { + if (!checkStaticMethodAccess(member)) { LOG.warn("Access to static method [{}] is blocked!", member); return false; } - if (isClassExcluded(memberClass)) { - LOG.warn("Declaring class of member type [{}] is excluded!", member); + if (!checkDefaultPackageAccess(target, member)) { return false; } - if (targetClass != memberClass && isClassExcluded(targetClass)) { - LOG.warn("Target class [{}] of target [{}] is excluded!", targetClass, target); + if (!checkExclusionList(target, member)) { return false; } - if (disallowDefaultPackageAccess) { - if (targetClass.getPackage() == null || targetClass.getPackage().getName().isEmpty()) { - LOG.warn("Class [{}] from the default package is excluded!", targetClass); - return false; - } - if (memberClass.getPackage() == null || memberClass.getPackage().getName().isEmpty()) { - LOG.warn("Class [{}] from the default package is excluded!", memberClass); - return false; - } + if (!isAcceptableProperty(propertyName)) { + return false; } - if (isPackageExcluded(targetClass)) { - LOG.warn("Package [{}] of target class [{}] of target [{}] is excluded!", - targetClass.getPackage(), - targetClass, + return true; + } + + /** + * @return {@code true} if member access is allowed + */ + protected boolean checkExclusionList(Object target, Member member) { + Class> memberClass = member.getDeclaringClass(); + if (isClassExcluded(memberClass)) { + LOG.warn("Declaring class of member type [{}] is excluded!", memberClass); + return false; + } + if (isPackageExcluded(memberClass)) { + LOG.warn("Package [{}] of member class [{}] of member [{}] is excluded!", + memberClass.getPackage(), + memberClass, target); return false; } + if (target == null || target.getClass() == memberClass) { + return true; + } + Class> targetClass = target.getClass(); + if (isClassExcluded(targetClass)) { + LOG.warn("Target class [{}] of target [{}] is excluded!", targetClass, target); + return false; + } + if (isPackageExcluded(targetClass)) { + LOG.warn("Package [{}] of target [{}] is excluded!", targetClass.getPackage(), member); + return false; + } + return true; + } - if (targetClass != memberClass && isPackageExcluded(memberClass)) { - LOG.warn("Package [{}] of member [{}] are excluded!", memberClass.getPackage(), member); + /** + * @return {@code true} if member access is allowed + */ + protected boolean checkDefaultPackageAccess(Object target, Member member) { + if (!disallowDefaultPackageAccess) { + return true; + } + Class> memberClass = member.getDeclaringClass(); + if (memberClass.getPackage() == null || memberClass.getPackage().getName().isEmpty()) { + LOG.warn("Class [{}] from the default package is excluded!", memberClass); return false; } + if (target == null || target.getClass() == memberClass) { + return true; + } + Class> targetClass = target.getClass(); + if (targetClass.getPackage() == null || targetClass.getPackage().getName().isEmpty()) { + LOG.warn("Class [{}] from the default package is excluded!", targetClass); + return false; + } + return true; + } - return isAcceptableProperty(propertyName); + /** + * @return {@code true} if member access is allowed + */ + protected boolean checkProxyMemberAccess(Object target, Member member) { + return !(disallowProxyMemberAccess && ProxyUtil.isProxyMember(member, target)); } /** * Check access for static method (via modifiers). - * + *
* Note: For non-static members, the result is always true. * - * @param member - * @param memberModifiers - * - * @return + * @return {@code true} if member access is allowed */ - protected boolean checkStaticMethodAccess(Member member, int memberModifiers) { - return !Modifier.isStatic(memberModifiers) || member instanceof Field; + protected boolean checkStaticMethodAccess(Member member) { + if (checkEnumAccess(member)) { + LOG.trace("Exempting Enum#values from static method check: class [{}]", member.getDeclaringClass()); + return true; + } + return member instanceof Field || !isStatic(member); + } + + private static boolean isStatic(Member member) { + return Modifier.isStatic(member.getModifiers()); } /** @@ -194,36 +241,33 @@ protected boolean checkStaticMethodAccess(Member member, int memberModifiers) { *
* Note: For non-static members, the result is always true. * - * @param member - * @param memberModifiers - * @return + * @return {@code true} if member access is allowed */ - protected boolean checkStaticFieldAccess(Member member, int memberModifiers) { - if (Modifier.isStatic(memberModifiers) && member instanceof Field) { - return allowStaticFieldAccess; - } else { + protected boolean checkStaticFieldAccess(Member member) { + if (allowStaticFieldAccess) { return true; } + return !(member instanceof Field) || !isStatic(member); } /** * Check access for public members (via modifiers) - *
- * Returns true if-and-only-if the member is public. * - * @param memberModifiers - * @return + * @return {@code true} if member access is allowed */ - protected boolean checkPublicMemberAccess(int memberModifiers) { - return Modifier.isPublic(memberModifiers); + protected boolean checkPublicMemberAccess(Member member) { + return Modifier.isPublic(member.getModifiers()); } - protected boolean checkEnumAccess(Object target, Member member) { - if (target instanceof Class) { - final Class> clazz = (Class>) target; - return Enum.class.isAssignableFrom(clazz) && member.getName().equals("values"); - } - return false; + /** + * @return {@code true} if member access is allowed + */ + protected boolean checkEnumAccess(Member member) { + return member.getDeclaringClass().isEnum() + && isStatic(member) + && member instanceof Method + && member.getName().equals("values") + && ((Method) member).getParameterCount() == 0; } protected boolean isPackageExcluded(Class> clazz) { @@ -260,37 +304,22 @@ protected boolean isClassExcluded(Class> clazz) { return excludedClasses.contains(clazz.getName()); } + /** + * @return {@code true} if member access is allowed + */ protected boolean isAcceptableProperty(String name) { - return name == null || ((!isExcluded(name)) && isAccepted(name)); + return name == null || !isExcluded(name) && isAccepted(name); } protected boolean isAccepted(String paramName) { - if (!this.acceptProperties.isEmpty()) { - for (Pattern pattern : acceptProperties) { - Matcher matcher = pattern.matcher(paramName); - if (matcher.matches()) { - return true; - } - } - - //no match, but acceptedParams is not empty - return false; + if (acceptProperties.isEmpty()) { + return true; } - - //empty acceptedParams - return true; + return acceptProperties.stream().map(pattern -> pattern.matcher(paramName)).anyMatch(Matcher::matches); } protected boolean isExcluded(String paramName) { - if (!this.excludeProperties.isEmpty()) { - for (Pattern pattern : excludeProperties) { - Matcher matcher = pattern.matcher(paramName); - if (matcher.matches()) { - return true; - } - } - } - return false; + return excludeProperties.stream().map(pattern -> pattern.matcher(paramName)).anyMatch(Matcher::matches); } /** diff --git a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index 2e7a0be160..f1d1fe9f00 100644 --- a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -350,7 +350,7 @@ public void testAccessToOgnlInternals() throws Exception { //then assertEquals("This is blah", ((SimpleAction) proxy.getAction()).getBlah()); Field field = ReflectionContextState.class.getField("DENY_METHOD_EXECUTION"); - boolean allowStaticFieldAccess = ((OgnlContext) stack.getContext()).getMemberAccess().isAccessible(stack.getContext(), proxy.getAction(), field, ""); + boolean allowStaticFieldAccess = ((OgnlContext) stack.getContext()).getMemberAccess().isAccessible(stack.getContext(), ReflectionContextState.class, field, ""); assertFalse(allowStaticFieldAccess); } 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 f38a7dce66..7ffc47b9f8 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -351,6 +351,16 @@ public void testAccessEnum() throws Exception { assertTrue("Access to enums is blocked!", actual); } + @Test + public void testAccessEnum_alternateValues() throws Exception { + // when + Member alternateValues = MyValues.class.getMethod("values", String.class); + boolean actual = sma.isAccessible(context, MyValues.class, alternateValues, null); + + // then + assertFalse("Access to unrelated #values method not blocked!", actual); + } + @Test public void testAccessStaticMethod() throws Exception { // given @@ -358,7 +368,7 @@ public void testAccessStaticMethod() throws Exception { // when Member method = StaticTester.class.getMethod("sayHello"); - boolean actual = sma.isAccessible(context, Class.class, method, null); + boolean actual = sma.isAccessible(context, StaticTester.class, method, null); // then assertFalse("Access to static method is not blocked!", actual); @@ -585,7 +595,7 @@ public void testBlockStaticMethodAccess() throws Exception { // when Member method = StaticTester.class.getMethod("sayHello"); - boolean actual = sma.isAccessible(context, Class.class, method, null); + boolean actual = sma.isAccessible(context, StaticTester.class, method, null); // then assertFalse("Access to static isn't blocked!", actual); @@ -694,7 +704,7 @@ public void testAccessPrimitiveDoubleWithNames() throws Exception { member = System.class.getMethod(propertyName, int.class); // when - accessible = sma.isAccessible(context, target, member, propertyName); + accessible = sma.isAccessible(context, System.class, member, propertyName); // then assertFalse(accessible); @@ -875,7 +885,11 @@ interface FooBarInterface extends FooInterface, BarInterface { } enum MyValues { - ONE, TWO, THREE + ONE, TWO, THREE; + + public static MyValues[] values(String notUsed) { + return new MyValues[] {ONE, TWO, THREE}; + } } class StaticTester {