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 f882b2c583..0c16ca1a6e 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -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. @@ -77,16 +79,23 @@ public class SecurityMemberAccess implements MemberAccess { private final ProviderAllowlist providerAllowlist; private final ThreadAllowlist threadAllowlist; + private boolean allowStaticFieldAccess = true; + private Set excludeProperties = emptySet(); private Set acceptProperties = emptySet(); + private Set excludedClasses = unmodifiableSet(new HashSet<>(singletonList(Object.class.getName()))); private Set excludedPackageNamePatterns = emptySet(); private Set excludedPackageNames = emptySet(); private Set excludedPackageExemptClasses = emptySet(); + + private boolean isDevMode; + private boolean enforceAllowlistEnabled = false; private Set> allowlistClasses = emptySet(); private Set allowlistPackageNames = emptySet(); + private boolean disallowProxyObjectAccess = false; private boolean disallowProxyMemberAccess = false; private boolean disallowDefaultPackageAccess = false; @@ -209,12 +218,28 @@ 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) { @@ -222,12 +247,42 @@ protected boolean checkAllowlist(Object target, Member member) { } 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) @@ -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); } @@ -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); + } } diff --git a/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java b/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java index c169af20b0..895cfb7eec 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java @@ -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; @@ -33,6 +34,8 @@ import java.lang.reflect.Modifier; import java.lang.reflect.Proxy; +import static java.lang.reflect.Modifier.isPublic; + /** * ProxyUtil *

@@ -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; + } } 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 381b7d0ad0..d508ef99d0 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -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; @@ -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(); @@ -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()); @@ -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()); @@ -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()); @@ -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()); @@ -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()); @@ -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 mockHibernateProxy(T originalObject, Class proxyInterface) { + return (T) Proxy.newProxyInstance( + proxyInterface.getClassLoader(), + new Class[]{proxyInterface, HibernateProxy.class}, + new DummyHibernateProxyHandler(originalObject) + ); + } } class FooBar implements FooBarInterface { @@ -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); + } +} diff --git a/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java b/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java index 3838ca9aea..885665a120 100644 --- a/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java +++ b/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java @@ -19,76 +19,72 @@ package com.opensymphony.xwork2.ognl; import com.opensymphony.xwork2.ActionProxy; -import com.opensymphony.xwork2.XWorkTestCase; +import com.opensymphony.xwork2.XWorkJUnit4TestCase; import com.opensymphony.xwork2.config.providers.XmlConfigurationProvider; import org.apache.struts2.config.StrutsXmlConfigurationProvider; +import org.junit.Before; +import org.junit.Test; import java.lang.reflect.Member; +import java.util.Arrays; import java.util.HashMap; import java.util.Map; -public class SecurityMemberAccessProxyTest extends XWorkTestCase { +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class SecurityMemberAccessProxyTest extends XWorkJUnit4TestCase { + + private static final String PROXY_MEMBER_METHOD = "isExposeProxy"; + private static final String TEST_SUB_BEAN_CLASS_METHOD = "getIssueId"; + private Map context; private ActionProxy proxy; - private Map members; - private final SecurityMemberAccess sma = new SecurityMemberAccess(true); - private final String PROXY_MEMBER_METHOD = "isExposeProxy"; - private final String TEST_SUB_BEAN_CLASS_METHOD = "setIssueId"; + private final SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + + private Member proxyObjectProxyMember; + private Member proxyObjectNonProxyMember; + @Before @Override public void setUp() throws Exception { - super.setUp(); - - context = new HashMap<>(); - // Set up XWork XmlConfigurationProvider provider = new StrutsXmlConfigurationProvider("com/opensymphony/xwork2/spring/actionContext-xwork.xml"); - container.inject(provider); loadConfigurationProviders(provider); - // Setup proxy object - setupProxy(); + context = new HashMap<>(); + proxy = actionProxyFactory.createActionProxy(null, "chaintoAOPedTestSubBeanAction", null, context); + proxyObjectProxyMember = proxy.getAction().getClass().getMethod(PROXY_MEMBER_METHOD); + proxyObjectNonProxyMember = proxy.getAction().getClass().getMethod(TEST_SUB_BEAN_CLASS_METHOD); } - public void testProxyAccessIsBlocked() throws Exception { - members.values().forEach(member -> { - // When disallowProxyObjectAccess is set to true, and disallowProxyMemberAccess is set to false, the proxy access is blocked - sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); - sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); - assertFalse(sma.isAccessible(context, proxy.getAction(), member, "")); - - // When disallowProxyObjectAccess is set to true, and disallowProxyMemberAccess is set to true, the proxy access is blocked - sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); - sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); - assertFalse(sma.isAccessible(context, proxy.getAction(), member, "")); - }); - - // When disallowProxyObjectAccess is set to false, and disallowProxyMemberAccess is set to true, the proxy member access is blocked - sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); - sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); - assertFalse(sma.isAccessible(context, proxy.getAction(), members.get(PROXY_MEMBER_METHOD), "")); + /** + * When {@code disallowProxyObjectAccess} is {@code true}, proxy access is blocked irrespective of + * {@code disallowProxyMemberAccess} value and irrespective of whether the member itself originates from the proxy. + */ + @Test + public void disallowProxyObjectAccess() { + sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); + Arrays.asList(proxyObjectProxyMember, proxyObjectNonProxyMember).forEach(member -> + Arrays.asList(Boolean.TRUE, Boolean.FALSE).forEach(disallowProxyMemberAccess -> { + sma.useDisallowProxyMemberAccess(disallowProxyMemberAccess.toString()); + assertFalse(sma.isAccessible(context, proxy.getAction(), member, "")); + }) + ); } - public void testProxyAccessIsAccessible() throws Exception { - members.values().forEach(member -> { - // When disallowProxyObjectAccess is set to false, and disallowProxyMemberAccess is set to false, the proxy access is allowed - sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); - sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); - assertTrue(sma.isAccessible(context, proxy.getAction(), member, "")); - }); - - // When disallowProxyObjectAccess is set to false, and disallowProxyMemberAccess is set to true, the original class member access is allowed + @Test + public void disallowProxyMemberAccess() { sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); - assertTrue(sma.isAccessible(context, proxy.getAction(), members.get(TEST_SUB_BEAN_CLASS_METHOD), "")); + assertFalse(sma.isAccessible(context, proxy.getAction(), proxyObjectProxyMember, "")); + assertTrue(sma.isAccessible(context, proxy.getAction(), proxyObjectNonProxyMember, "")); } - private void setupProxy() throws NoSuchMethodException { - proxy = actionProxyFactory.createActionProxy(null, "chaintoAOPedTestSubBeanAction", null, context); - - members = new HashMap<>(); - // method is proxy member - members.put(PROXY_MEMBER_METHOD, proxy.getAction().getClass().getMethod(PROXY_MEMBER_METHOD)); - // method is not proxy member but from POJO class - members.put(TEST_SUB_BEAN_CLASS_METHOD, proxy.getAction().getClass().getMethod(TEST_SUB_BEAN_CLASS_METHOD, String.class)); + @Test + public void allowAllProxyAccess() { + sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); + assertTrue(sma.isAccessible(context, proxy.getAction(), proxyObjectProxyMember, "")); + assertTrue(sma.isAccessible(context, proxy.getAction(), proxyObjectNonProxyMember, "")); } }