Skip to content

WW-5525 Fix NPE in ProxyUtil for SecurityMemberAccess originating static members #1214

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

Merged
merged 1 commit into from
Feb 17, 2025
Merged

WW-5525 Fix NPE in ProxyUtil for SecurityMemberAccess originating static members #1214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 10 additions & 7 deletions core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,17 +38,17 @@
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;
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.
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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
Expand Down
9 changes: 5 additions & 4 deletions core/src/main/java/org/apache/struts2/util/ProxyUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand Down
47 changes: 37 additions & 10 deletions core/src/test/java/org/apache/struts2/ognl/OgnlValueStackTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion plugins/spring/src/test/java/org/apache/struts2/spring/SpringProxyUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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));

Expand Down