Skip to content

Commit

Permalink
Merge pull request #967 from apache/WW-5428-allowlist-hibernate
Browse files Browse the repository at this point in the history 
WW-5428 Allowlist capability should resolve Hibernate proxies when disableProxyObjects is not set
  • Loading branch information
@kusalk
kusalk committed Jul 8, 2024
2 parents 82b364d + 05680d7 commit 7f57e89
Show file tree
Hide file tree
Showing 4 changed files with 221 additions and 55 deletions.
70 changes: 65 additions & 5 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 @@ -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.
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
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
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) {
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

0 comments on commit 7f57e89

Please sign in to comment.