You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by ku...@apache.org on 2023/11/05 09:13:51 UTC
(struts) 01/02: WW-5350 Refactor SecurityMemberAccess
This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5350-allowlist
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 82647959b2a7db068bbb219fd07128e6b32fdc5b
Author: Kusal Kithul-Godage <gi...@kusal.io>
AuthorDate: Fri Nov 3 18:10:57 2023 +1100
WW-5350 Refactor SecurityMemberAccess
---
.../xwork2/ognl/SecurityMemberAccess.java | 168 +++++++++++++--------
.../xwork2/ognl/SecurityMemberAccessTest.java | 16 +-
2 files changed, 120 insertions(+), 64 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 232c33e5a..6301ac1a3 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 org.apache.logging.log4j.Logger;
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,130 @@ public class SecurityMemberAccess implements MemberAccess {
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 instanceof Class) { // Target may be of type Class for static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null && !member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ throw new IllegalArgumentException("Target does not match member!");
+ }
}
- 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)) {
+ 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 target class [{}] of target [{}] is excluded!",
- targetClass.getPackage(),
- targetClass,
+ 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 member [{}] are 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).
- *
+ * <p>
* 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 +236,33 @@ public class SecurityMemberAccess implements MemberAccess {
* <p>
* 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)
- * <p>
- * 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,8 +299,11 @@ public class SecurityMemberAccess implements MemberAccess {
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) {
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 f38a7dce6..830cdbb2c 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 class SecurityMemberAccessTest {
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
@@ -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 {