This is an automated email from the ASF dual-hosted git repository. kusal pushed a commit to branch WW-5525-proxyutil-npe in repository https://gitbox.apache.org/repos/asf/struts.git
commit eccd236131f28ff99e3194d2d90c639b5832924b Author: Kusal Kithul-Godage <g...@kusal.io> AuthorDate: Thu Feb 6 12:02:40 2025 +1100 WW-5525 Fix NPE in ProxyUtil for SecurityMemberAccess originating static members --- .../apache/struts2/ognl/SecurityMemberAccess.java | 17 ++++---- .../java/org/apache/struts2/util/ProxyUtil.java | 9 +++-- .../apache/struts2/ognl/OgnlValueStackTest.java | 47 +++++++++++++++++----- 3 files changed, 52 insertions(+), 21 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 9c266645b..64a8fa5a4 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.Matcher; 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.toNewPackageNamesSet; 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 class SecurityMemberAccess implements MemberAccess { 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 @@ public class SecurityMemberAccess implements MemberAccess { 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 392bb9a0b..71823b47e 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 class ProxyUtil { * @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 class ProxyUtil { */ 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 d19ca812b..2edcb3eee 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.Foo; 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 class OgnlValueStackTest extends XWorkTestCase { 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;