Skip to content

Commit

Permalink
Merge pull request #780 from apache/WW-5350-allowlist
Browse files Browse the repository at this point in the history 
WW-5350 Refactor SecurityMemberAccess
  • Loading branch information
@kusalk
kusalk authored Nov 12, 2023
2 parents f511034 + e324138 commit 9d6fe74
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 91 deletions.
201 changes: 115 additions & 86 deletions core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -104,126 +105,169 @@ 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).
*
* <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());
}

/**
* Check access for static field (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 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) {
Expand Down Expand Up @@ -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);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
22 changes: 18 additions & 4 deletions core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,14 +351,24 @@ 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
sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName())));

// 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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 9d6fe74

Please sign in to comment.