From 3c856c92a1bb7785ddd79bc95feaa7a70653cce3 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 6 Feb 2025 12:02:40 +1100 Subject: [PATCH] WW-5525 Fix NPE in ProxyUtil for SecurityMemberAccess originating static members --- .../struts2/ognl/SecurityMemberAccess.java | 17 ++++--- .../org/apache/struts2/util/ProxyUtil.java | 9 ++-- .../struts2/ognl/OgnlValueStackTest.java | 47 +++++++++++++++---- .../struts2/spring/SpringProxyUtilTest.java | 4 +- 4 files changed, 55 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index 9c266645ba..64a8fa5a44 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -18,13 +18,13 @@ */ package org.apache.struts2.ognl; -import org.apache.struts2.inject.Inject; -import org.apache.struts2.util.ProxyUtil; import ognl.MemberAccess; import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; +import org.apache.struts2.inject.Inject; +import org.apache.struts2.util.ProxyUtil; import java.lang.reflect.AccessibleObject; import java.lang.reflect.Constructor; @@ -38,6 +38,10 @@ import java.util.regex.Pattern; import java.util.stream.IntStream; +import static java.text.MessageFormat.format; +import static java.util.Collections.emptySet; +import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES; +import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES; import static org.apache.struts2.util.ConfigParseUtil.toClassObjectsSet; import static org.apache.struts2.util.ConfigParseUtil.toClassesSet; import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet; @@ -45,10 +49,6 @@ import static org.apache.struts2.util.ConfigParseUtil.toNewPatternsSet; import static org.apache.struts2.util.ConfigParseUtil.toPackageNamesSet; import static org.apache.struts2.util.DebugUtils.logWarningForFirstOccurrence; -import static java.text.MessageFormat.format; -import static java.util.Collections.emptySet; -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. @@ -141,6 +141,9 @@ 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); + if (member == null) { + throw new IllegalArgumentException("Member cannot be null!"); + } if (target != null) { // Special case: Target is a Class object but not Class.class if (Class.class.equals(target.getClass()) && !Class.class.equals(target)) { @@ -209,7 +212,7 @@ protected boolean checkAllowlist(Object target, Member member) { return true; } - if (!disallowProxyObjectAccess && target != null && ProxyUtil.isProxy(target)) { + if (!disallowProxyObjectAccess && 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 diff --git a/core/src/main/java/org/apache/struts2/util/ProxyUtil.java b/core/src/main/java/org/apache/struts2/util/ProxyUtil.java index 392bb9a0b2..71823b47e5 100644 --- a/core/src/main/java/org/apache/struts2/util/ProxyUtil.java +++ b/core/src/main/java/org/apache/struts2/util/ProxyUtil.java @@ -18,12 +18,12 @@ */ package org.apache.struts2.util; -import org.apache.struts2.ognl.DefaultOgnlCacheFactory; -import org.apache.struts2.ognl.OgnlCache; -import org.apache.struts2.ognl.OgnlCacheFactory; import org.apache.commons.lang3.reflect.ConstructorUtils; import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.commons.lang3.reflect.MethodUtils; +import org.apache.struts2.ognl.DefaultOgnlCacheFactory; +import org.apache.struts2.ognl.OgnlCache; +import org.apache.struts2.ognl.OgnlCacheFactory; import org.hibernate.Hibernate; import org.hibernate.proxy.HibernateProxy; @@ -81,6 +81,7 @@ public static Class ultimateTargetClass(Object candidate) { * @param object the object to check */ public static boolean isProxy(Object object) { + if (object == null) return false; Class clazz = object.getClass(); Boolean flag = isProxyCache.get(clazz); if (flag != null) { @@ -121,7 +122,7 @@ public static boolean isProxyMember(Member member, Object object) { */ public static boolean isHibernateProxy(Object object) { try { - return HibernateProxy.class.isAssignableFrom(object.getClass()); + return object != null && HibernateProxy.class.isAssignableFrom(object.getClass()); } catch (NoClassDefFoundError ignored) { return false; } diff --git a/core/src/test/java/org/apache/struts2/ognl/OgnlValueStackTest.java b/core/src/test/java/org/apache/struts2/ognl/OgnlValueStackTest.java index d19ca812b5..2edcb3eeef 100644 --- a/core/src/test/java/org/apache/struts2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/OgnlValueStackTest.java @@ -18,17 +18,26 @@ */ package org.apache.struts2.ognl; +import ognl.OgnlException; +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.core.appender.AbstractAppender; import org.apache.struts2.SimpleAction; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.StrutsException; import org.apache.struts2.TestBean; -import org.apache.struts2.text.TextProvider; import org.apache.struts2.XWorkTestCase; import org.apache.struts2.config.ConfigurationException; +import org.apache.struts2.config.DefaultPropertiesProvider; import org.apache.struts2.conversion.impl.ConversionData; import org.apache.struts2.conversion.impl.XWorkConverter; import org.apache.struts2.inject.ContainerBuilder; import org.apache.struts2.ognl.accessor.RootAccessor; import org.apache.struts2.test.StubConfigurationProvider; import org.apache.struts2.test.TestBean2; +import org.apache.struts2.text.TextProvider; import org.apache.struts2.util.Bar; import org.apache.struts2.util.BarJunior; import org.apache.struts2.util.Cat; @@ -37,15 +46,6 @@ import org.apache.struts2.util.ValueStackFactory; import org.apache.struts2.util.location.LocatableProperties; import org.apache.struts2.util.reflection.ReflectionContextState; -import ognl.OgnlException; -import org.apache.commons.lang3.StringUtils; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.core.LogEvent; -import org.apache.logging.log4j.core.Logger; -import org.apache.logging.log4j.core.appender.AbstractAppender; -import org.apache.struts2.StrutsConstants; -import org.apache.struts2.StrutsException; -import org.apache.struts2.config.DefaultPropertiesProvider; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -1234,6 +1234,33 @@ public void testOgnlValueStackFromOgnlValueStackFactoryAllStaticAccess() throws assertNull("accessed private field (result not null) ?", accessedValue); } + public void testFindValueWithConstructorAndProxyChecks() { + loadButSet(Map.of( + StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, Boolean.TRUE.toString(), + StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, Boolean.TRUE.toString())); + refreshContainerFields(); + + String value = "test"; + String ognlResult = (String) vs.findValue( + "new org.apache.struts2.ognl.OgnlValueStackTest$ValueHolder('" + value + "').value", String.class); + + assertEquals(value, ognlResult); + } + + @SuppressWarnings({"unused", "ClassCanBeRecord"}) + public static class ValueHolder { + // See testFindValueWithConstructorAndProxyChecks + private final String value; + + public ValueHolder(String value) { + this.value = value; + } + + public String getValue() { + return value; + } + } + static class BadJavaBean { private int count; private int count2; diff --git a/plugins/spring/src/test/java/org/apache/struts2/spring/SpringProxyUtilTest.java b/plugins/spring/src/test/java/org/apache/struts2/spring/SpringProxyUtilTest.java index a09f7f0c45..f2e045ce8c 100644 --- a/plugins/spring/src/test/java/org/apache/struts2/spring/SpringProxyUtilTest.java +++ b/plugins/spring/src/test/java/org/apache/struts2/spring/SpringProxyUtilTest.java @@ -23,9 +23,9 @@ import org.apache.struts2.TestBean; import org.apache.struts2.TestSubBean; import org.apache.struts2.XWorkTestCase; +import org.apache.struts2.config.StrutsXmlConfigurationProvider; import org.apache.struts2.config.providers.XmlConfigurationProvider; import org.apache.struts2.util.ProxyUtil; -import org.apache.struts2.config.StrutsXmlConfigurationProvider; import org.springframework.context.ApplicationContext; /** @@ -46,6 +46,8 @@ public void setUp() throws Exception { } public void testIsProxy() throws Exception { + assertFalse(ProxyUtil.isProxy(null)); + Object simpleAction = appContext.getBean("simple-action"); assertFalse(ProxyUtil.isProxy(simpleAction));