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.

Sign up for GitHub

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-5428 Allowlist capability should resolve Hibernate proxies when disableProxyObjects is not set #967

Merged
merged 6 commits into from
Jul 8, 2024
Merged

WW-5428 Allowlist capability should resolve Hibernate proxies when disableProxyObjects is not set #967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2f81418
WW-5428 Allowlist capability should resolve Hibernate proxies when di…
kusalk Jun 17, 2024
abf03fd
WW-5428 Clean up SecurityMemberAccessProxyTest
kusalk Jul 8, 2024
c965812
WW-5428 Add unit test coverage for Hibernate proxy resolution
kusalk Jul 8, 2024
c6f394a
WW-5428 Add log warning for Hibernate entities
kusalk Jul 8, 2024
8555dc2
WW-5428 Add log warning for allowlist disabled
kusalk Jul 8, 2024
05680d7
WW-5428 Amend log warning for missing allowlist entry
kusalk Jul 8, 2024
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
70 changes: 65 additions & 5 deletions core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@
import static java.util.Collections.emptySet;
import static java.util.Collections.singletonList;
import static java.util.Collections.unmodifiableSet;
import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES;
import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES;

/**
* Allows access decisions to be made on the basis of whether a member is static or not.
Expand All @@ -77,16 +79,23 @@ public class SecurityMemberAccess implements MemberAccess {

private final ProviderAllowlist providerAllowlist;
private final ThreadAllowlist threadAllowlist;

private boolean allowStaticFieldAccess = true;

private Set<Pattern> excludeProperties = emptySet();
private Set<Pattern> acceptProperties = emptySet();

private Set<String> excludedClasses = unmodifiableSet(new HashSet<>(singletonList(Object.class.getName())));
private Set<Pattern> excludedPackageNamePatterns = emptySet();
private Set<String> excludedPackageNames = emptySet();
private Set<String> excludedPackageExemptClasses = emptySet();

private boolean isDevMode;

private boolean enforceAllowlistEnabled = false;
private Set<Class<?>> allowlistClasses = emptySet();
private Set<String> allowlistPackageNames = emptySet();

private boolean disallowProxyObjectAccess = false;
private boolean disallowProxyMemberAccess = false;
private boolean disallowDefaultPackageAccess = false;
Expand Down Expand Up @@ -209,25 +218,71 @@ public boolean isAccessible(Map context, Object target, Member member, String pr
* @return {@code true} if member access is allowed
*/
protected boolean checkAllowlist(Object target, Member member) {
Class<?> memberClass = member.getDeclaringClass();
if (!enforceAllowlistEnabled) {
logAllowlistDisabled();
return true;
}

if (!disallowProxyObjectAccess && target != null && ProxyUtil.isProxy(target)) {
// If `disallowProxyObjectAccess` is not set, allow resolving Hibernate entities to their underlying
// classes/members. This allows the allowlist capability to continue working and offer some level of
// protection in applications where the developer has accepted the risk of allowing OGNL access to Hibernate
// entities. This is preferred to having to disable the allowlist capability entirely.
Comment on lines +227 to +230
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't be good to log this info? Maybe even in WARN level if struts.devMode is enabled, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yeah doesn't hurt to add some logging - will do

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Object newTarget = ProxyUtil.getHibernateProxyTarget(target);
if (newTarget != target) {
logAllowlistHibernateEntity(target, newTarget);
target = newTarget;
member = ProxyUtil.resolveTargetMember(member, newTarget);
}
}

Class<?> memberClass = member.getDeclaringClass();
if (!isClassAllowlisted(memberClass)) {
LOG.warn(format("Declaring class [{0}] of member type [{1}] is not allowlisted!", memberClass, member));
LOG.warn("Declaring class [{}] of member type [{}] is not allowlisted! Add to '{}' or '{}' configuration.",
memberClass, member, STRUTS_ALLOWLIST_CLASSES, STRUTS_ALLOWLIST_PACKAGE_NAMES);
return false;
}
if (target == null || target.getClass() == memberClass) {
return true;
}
Class<?> targetClass = target.getClass();
if (!isClassAllowlisted(targetClass)) {
LOG.warn(format("Target class [{0}] of target [{1}] is not allowlisted!", targetClass, target));
LOG.warn("Target class [{}] of target [{}] is not allowlisted! Add to '{}' or '{}' configuration.",
targetClass, target, STRUTS_ALLOWLIST_CLASSES, STRUTS_ALLOWLIST_PACKAGE_NAMES);
return false;
}
return true;
}

private void logAllowlistDisabled() {
if (!isDevMode && !LOG.isDebugEnabled()) {
return;
}
String msg = "OGNL allowlist is disabled!" +
" We strongly recommend keeping it enabled to protect against critical vulnerabilities." +
" Set the configuration `{0}=true` to enable it.";
Object[] args = {StrutsConstants.STRUTS_ALLOWLIST_ENABLE};
if (isDevMode) {
LOG.warn(msg, args);
} else {
LOG.debug(msg, args);
}
}

private void logAllowlistHibernateEntity(Object original, Object resolved) {
if (!isDevMode && !LOG.isDebugEnabled()) {
return;
}
String msg = "Hibernate entity [{}] resolved to [{}] for purpose of OGNL allowlisting." +
" We don't recommend executing OGNL expressions against Hibernate entities, you may disallow this behaviour using the configuration `{}=true`.";
Object[] args = {original, resolved, StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS};
if (isDevMode) {
LOG.warn(msg, args);
} else {
LOG.debug(msg, args);
}
}

protected boolean isClassAllowlisted(Class<?> clazz) {
return allowlistClasses.contains(clazz)
|| ALLOWLIST_REQUIRED_CLASSES.contains(clazz)
Expand Down Expand Up @@ -436,12 +491,12 @@ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) {
this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled);
}

@Inject(value = StrutsConstants.STRUTS_ALLOWLIST_CLASSES, required = false)
@Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false)
public void useAllowlistClasses(String commaDelimitedClasses) {
this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses);
}

@Inject(value = StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false)
@Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false)
public void useAllowlistPackageNames(String commaDelimitedPackageNames) {
this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames);
}
Expand All @@ -460,4 +515,9 @@ public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) {
public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) {
this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess);
}

@Inject(StrutsConstants.STRUTS_DEVMODE)
protected void useDevMode(String devMode) {
this.isDevMode = BooleanUtils.toBoolean(devMode);
}
}
33 changes: 33 additions & 0 deletions core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.apache.commons.lang3.reflect.ConstructorUtils;
import org.apache.commons.lang3.reflect.FieldUtils;
import org.apache.commons.lang3.reflect.MethodUtils;
import org.hibernate.Hibernate;
import org.hibernate.proxy.HibernateProxy;

import java.lang.reflect.Constructor;
Expand All @@ -33,6 +34,8 @@
import java.lang.reflect.Modifier;
import java.lang.reflect.Proxy;

import static java.lang.reflect.Modifier.isPublic;

/**
* <code>ProxyUtil</code>
* <p>
Expand Down Expand Up @@ -255,4 +258,34 @@ private static boolean hasMember(Class<?> clazz, Member member) {

return false;
}

/**
* @return the target instance of the given object if it is a Hibernate proxy object, otherwise the given object
*/
public static Object getHibernateProxyTarget(Object object) {
try {
return Hibernate.unproxy(object);
} catch (NoClassDefFoundError ignored) {
return object;
}
}

/**
* @return matching member on target object if one exists, otherwise the same member
*/
public static Member resolveTargetMember(Member proxyMember, Object target) {
int mod = proxyMember.getModifiers();
if (proxyMember instanceof Method) {
if (isPublic(mod)) {
return MethodUtils.getMatchingAccessibleMethod(target.getClass(), proxyMember.getName(), ((Method) proxyMember).getParameterTypes());
} else {
return MethodUtils.getMatchingMethod(target.getClass(), proxyMember.getName(), ((Method) proxyMember).getParameterTypes());
}
} else if (proxyMember instanceof Field) {
return FieldUtils.getField(target.getClass(), proxyMember.getName(), isPublic(mod));
} else if (proxyMember instanceof Constructor && isPublic(mod)) {
return ConstructorUtils.getMatchingAccessibleConstructor(target.getClass(), ((Constructor<?>) proxyMember).getParameterTypes());
}
return proxyMember;
}
}
81 changes: 79 additions & 2 deletions core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,16 @@
import org.apache.commons.lang3.reflect.FieldUtils;
import org.apache.struts2.ognl.ProviderAllowlist;
import org.apache.struts2.ognl.ThreadAllowlist;
import org.hibernate.proxy.HibernateProxy;
import org.hibernate.proxy.LazyInitializer;
import org.junit.Before;
import org.junit.Test;

import java.lang.reflect.Field;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Member;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
Expand Down Expand Up @@ -853,9 +857,11 @@ public void testPackageNameExclusionAsCommaDelimited() {
assertTrue("package java.lang. is accessible!", actual);
}

/**
* Test that the allowlist is enforced correctly for classes.
*/
@Test
public void classInclusion() throws Exception {

sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());

TestBean2 bean = new TestBean2();
Expand All @@ -868,6 +874,9 @@ public void classInclusion() throws Exception {
assertTrue(sma.checkAllowlist(bean, method));
}

/**
* Test that the allowlist is enforced correctly for packages.
*/
@Test
public void packageInclusion() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
Expand All @@ -882,6 +891,9 @@ public void packageInclusion() throws Exception {
assertTrue(sma.checkAllowlist(bean, method));
}

/**
* Test that the allowlist doesn't allow inherited methods unless the declaring class is also allowlisted.
*/
@Test
public void classInclusion_subclass() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
Expand All @@ -893,6 +905,9 @@ public void classInclusion_subclass() throws Exception {
assertFalse(sma.checkAllowlist(bean, method));
}

/**
* Test that the allowlist allows inherited methods when both the target and declaring class are allowlisted.
*/
@Test
public void classInclusion_subclass_both() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
Expand All @@ -904,6 +919,10 @@ public void classInclusion_subclass_both() throws Exception {
assertTrue(sma.checkAllowlist(bean, method));
}

/**
* Test that the allowlist doesn't allow inherited methods unless the package of the declaring class is also
* allowlisted.
*/
@Test
public void packageInclusion_subclass() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
Expand All @@ -915,6 +934,37 @@ public void packageInclusion_subclass() throws Exception {
assertFalse(sma.checkAllowlist(bean, method));
}

/**
* When the allowlist is enabled and proxy object access is disallowed, Hibernate proxies should not be allowed.
*/
@Test
public void classInclusion_hibernateProxy_disallowProxyObjectAccess() throws Exception {
FooBarInterface proxyObject = mockHibernateProxy(new FooBar(), FooBarInterface.class);
Method proxyMethod = proxyObject.getClass().getMethod("fooLogic");

sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString());
sma.useAllowlistClasses(FooBar.class.getName());

assertFalse(sma.checkAllowlist(proxyObject, proxyMethod));
}

/**
* When the allowlist is enabled and proxy object access is allowed, Hibernate proxies should be allowlisted based
* on their underlying target object. Class allowlisting should work as expected.
*/
@Test
public void classInclusion_hibernateProxy_allowProxyObjectAccess() throws Exception {
FooBarInterface proxyObject = mockHibernateProxy(new FooBar(), FooBarInterface.class);
Method proxyMethod = proxyObject.getClass().getMethod("fooLogic");

sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString());
sma.useAllowlistClasses(FooBar.class.getName());

assertTrue(sma.checkAllowlist(proxyObject, proxyMethod));
}

@Test
public void packageInclusion_subclass_both() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
Expand All @@ -931,6 +981,15 @@ public void packageInclusion_subclass_both() throws Exception {
private static String formGetterName(String propertyName) {
return "get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1);
}

@SuppressWarnings("unchecked")
private static <T> T mockHibernateProxy(T originalObject, Class<T> proxyInterface) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Mocking a Hibernate proxy isn't perfect - integration/acceptance tests would offer better protection against regressions but I'd prefer not to complicate the tests further by introducing a Hibernate session factory etc

return (T) Proxy.newProxyInstance(
proxyInterface.getClassLoader(),
new Class<?>[]{proxyInterface, HibernateProxy.class},
new DummyHibernateProxyHandler(originalObject)
);
}
}

class FooBar implements FooBarInterface {
Expand Down Expand Up @@ -1042,10 +1101,28 @@ public static String sayHello() {
}

protected static Field getFieldByName(String fieldName) throws NoSuchFieldException {
if (fieldName != null && fieldName.length() > 0) {
if (fieldName != null && !fieldName.isEmpty()) {
return StaticTester.class.getDeclaredField(fieldName);
} else {
throw new NoSuchFieldException("field: " + fieldName + " does not exist");
}
}
}

class DummyHibernateProxyHandler implements InvocationHandler {
private final Object instance;

public DummyHibernateProxyHandler(Object instance) {
this.instance = instance;
}

@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
if (HibernateProxy.class.getMethod("getHibernateLazyInitializer").equals(method)) {
LazyInitializer initializer = mock(LazyInitializer.class);
when(initializer.getImplementation()).thenReturn(instance);
return initializer;
}
return method.invoke(instance, args);
}
}
Loading