Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WW-5350 Refactor SecurityMemberAccess #780

Merged
merged 6 commits into from
Nov 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to be a bit more deliberate about how we handle the arguments in case OGNL behaviour changes

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)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validate and throw exceptions if arguments are not what we expect as the following logic depends on these assumptions to be accurate

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)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract this into its own protected method for consistency

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)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to standardise around just passing the object and/or member to these protected methods

if (!checkPublicMemberAccess(member)) {
LOG.warn("Access to non-public [{}] is blocked!", member);
return false;
}

if (!checkStaticFieldAccess(member, memberModifiers)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed redundant argument

if (!checkStaticFieldAccess(member)) {
LOG.warn("Access to static field [{}] is blocked!", member);
return false;
}

// it needs to be before calling #checkStaticMethodAccess()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially an exemption to checkStaticMethodAccess so I moved it within that method - we shouldn't be relying on method call order here

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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted this into its own method

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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted all the exclusion list checks into this method

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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bypass computation if static field access is permitted

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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closed a minor hole where another static values method could also be exempted

}

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
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
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