Skip to content
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

WW-5525 Prevent NPE when using new in OGNL #1200

Closed
wants to merge 2 commits into from
Closed

WW-5525 Prevent NPE when using new in OGNL #1200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b72b63
test(add tests for NPE in SecurityMemberAccess) #WW-5525
Jan 31, 2025
e0794c7
fix (NPE in SecurityMemberAccess#isAccessible) check nullity of param…
FBibonne Feb 1, 2025
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
14 changes: 11 additions & 3 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 @@ -141,6 +141,10 @@ 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){
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If member is null, it can cause others NPE (for example at line 151) and according the contract of the method a null member is a non sense

return false;
}

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 All @@ -158,7 +162,7 @@ public boolean isAccessible(Map context, Object target, Member member, String pr
}

if (!checkProxyObjectAccess(target)) {
LOG.warn("Access to proxy is blocked! Target [{}], proxy class [{}]", target, target.getClass().getName());
LOG.warn("Access to proxy is blocked! Target [{}], proxy class [{}]", target, getClassName(target));
return false;
}

Expand Down Expand Up @@ -201,6 +205,10 @@ public boolean isAccessible(Map context, Object target, Member member, String pr
return true;
}

private static String getClassName(Object target) {
return target==null?null:target.getClass().getName();
}

/**
* @return {@code true} if member access is allowed
*/
Expand Down Expand Up @@ -322,14 +330,14 @@ protected boolean checkDefaultPackageAccess(Object target, Member member) {
* @return {@code true} if proxy object access is allowed
*/
protected boolean checkProxyObjectAccess(Object target) {
return !(disallowProxyObjectAccess && ProxyUtil.isProxy(target));
return !(disallowProxyObjectAccess && target!=null && ProxyUtil.isProxy(target));
}

/**
* @return {@code true} if proxy member access is allowed
*/
protected boolean checkProxyMemberAccess(Object target, Member member) {
return !(disallowProxyMemberAccess && ProxyUtil.isProxyMember(member, target));
return !(disallowProxyMemberAccess && target!=null && ProxyUtil.isProxyMember(member, target));
}

/**
Expand Down
18 changes: 18 additions & 0 deletions core/src/test/java/org/apache/struts2/ognl/OgnlUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1663,6 +1663,24 @@ public void testCompilationErrorsCached() throws Exception {
assertSame(e, e2); // Exception is cached
}

public void testGetValueWithNewWhenDisallowProxyAccesses_shouldNotRaiseNPE() throws OgnlException {
Exception expected = null;
try {
resetOgnlUtil(Map.of(StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, Boolean.TRUE.toString(),
StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, Boolean.TRUE.toString()
));

var root=new Object();

String value = "test";
String result = (String) ognlUtil.getValue("new org.apache.struts2.ognl.ToBeInstanced('" + value + "').getValue()", ognlUtil.createDefaultContext(root), root, String.class);
assertEquals(value, result);
} catch (NullPointerException e) {
expected = e;
}
assertNull(expected);
}

/**
* Generate a new OgnlUtil instance (not configured by the {@link ContainerBuilder}) that can be used for
* basic tests, with its Expression and BeanInfo factories set to LRU mode.
Expand Down
63 changes: 31 additions & 32 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,46 +18,30 @@
*/
package org.apache.struts2.ognl;

import org.apache.struts2.SimpleAction;
import org.apache.struts2.TestBean;
import org.apache.struts2.text.TextProvider;
import org.apache.struts2.XWorkTestCase;
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.*;
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.util.Bar;
import org.apache.struts2.util.BarJunior;
import org.apache.struts2.util.Cat;
import org.apache.struts2.util.Dog;
import org.apache.struts2.text.TextProvider;
import org.apache.struts2.util.Foo;
import org.apache.struts2.util.ValueStackFactory;
import org.apache.struts2.util.*;
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;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.*;
import java.math.RoundingMode;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.*;

import static org.apache.struts2.ognl.SecurityMemberAccessTest.reflectField;

Expand Down Expand Up @@ -251,11 +235,11 @@ private void testLogMissingProperties(boolean logMissingProperties) {
if (logMissingProperties) {
assertEquals(3, testAppender.logEvents.size());
assertEquals("Error setting value [missingProp1Value] with expression [missingProp1]",
testAppender.logEvents.get(0).getMessage().getFormattedMessage());
testAppender.logEvents.get(0).getMessage().getFormattedMessage());
assertEquals("Could not find property [missingProp2]!",
testAppender.logEvents.get(1).getMessage().getFormattedMessage());
testAppender.logEvents.get(1).getMessage().getFormattedMessage());
assertEquals("Could not find property [missingProp3]!",
testAppender.logEvents.get(2).getMessage().getFormattedMessage());
testAppender.logEvents.get(2).getMessage().getFormattedMessage());
} else {
assertEquals(0, testAppender.logEvents.size());
}
Expand Down Expand Up @@ -332,7 +316,7 @@ public void register(ContainerBuilder builder,
}
});
int repeat = Integer.parseInt(
container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH));
container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH));

try {
vs.findValue(StringUtils.repeat('.', repeat + 1), true);
Expand Down Expand Up @@ -1234,6 +1218,21 @@ public void testOgnlValueStackFromOgnlValueStackFactoryAllStaticAccess() throws
assertNull("accessed private field (result not null) ?", accessedValue);
}

public void testFindValueWithNewWhenDisallowProxyAccesses() {
enableDisallowProxyAccesses();
String value = "test";
String ognlResult;
ognlResult = (String) vs.findValue("new org.apache.struts2.ognl.ToBeInstanced('" + value + "').getValue()", String.class);

assertEquals(value, ognlResult);
}

private void enableDisallowProxyAccesses() {
loadButSet(Map.of(StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, Boolean.TRUE.toString(),
StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, Boolean.TRUE.toString()));
refreshContainerFields();
}

static class BadJavaBean {
private int count;
private int count2;
Expand Down
46 changes: 33 additions & 13 deletions core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.apache.struts2.ognl;

import com.mockobjects.util.Null;
import org.apache.struts2.TestBean;
import org.apache.struts2.config.ConfigurationException;
import org.apache.struts2.test.TestBean2;
Expand All @@ -43,6 +44,7 @@
import java.util.Objects;
import java.util.Set;

import static java.lang.Boolean.TRUE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertThrows;
Expand Down Expand Up @@ -390,7 +392,7 @@ public void testDefaultPackageExclusion() throws Exception {

@Test
public void testDefaultPackageExclusionSetting() throws Exception {
sma.useDisallowDefaultPackageAccess(Boolean.TRUE.toString());
sma.useDisallowDefaultPackageAccess(TRUE.toString());

Class<?> clazz = Class.forName("PackagelessAction");
boolean actual = sma.isAccessible(null, clazz.getConstructor().newInstance(), clazz.getMethod("execute"), null);
Expand Down Expand Up @@ -685,7 +687,7 @@ public void testBlockAccessIfClassIsExcluded() throws Exception {
assertFalse("Access to method of excluded class isn't blocked!", actual);
}

@Test
@Test
public void testBlockAccessIfClassIsExcluded_2() throws Exception {
// given
sma.useExcludedClasses(ClassLoader.class.getName());
Expand All @@ -712,7 +714,7 @@ public void testAllowAccessIfClassIsNotExcluded() throws Exception {
assertTrue("Invalid test! Access to method of non-excluded class is blocked!", actual);
}

@Test
@Test
public void testIllegalArgumentExceptionExpectedForTargetMemberMismatch() throws Exception {
// given
sma.useExcludedClasses(Class.class.getName());
Expand Down Expand Up @@ -847,6 +849,24 @@ public void testAccessMemberAccessIsBlocked() throws Exception {
assertFalse(accessible);
}

@Test
public void testAccessConstructorWhenDisallowProxyAccesses() {
sma.useDisallowProxyMemberAccess(TRUE.toString());
sma.useDisallowProxyObjectAccess(TRUE.toString());
boolean accessible = false;
boolean exceptionOccured = false;
try {
accessible = sma.isAccessible(context,
ToBeInstanced.class,
ToBeInstanced.class.getConstructors()[0],
null);
} catch (NullPointerException npe) {
exceptionOccured=true;
}
assertFalse("Invalid test ! NPE occured!", exceptionOccured);
assertTrue("Invalid test ! constructor of ToBeInstanced class should be accessible", accessible);
}

@Test
public void testPackageNameExclusionAsCommaDelimited() {
// given
Expand All @@ -864,7 +884,7 @@ public void testPackageNameExclusionAsCommaDelimited() {
*/
@Test
public void classInclusion() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useEnforceAllowlistEnabled(TRUE.toString());

TestBean2 bean = new TestBean2();
Method method = TestBean2.class.getMethod("getData");
Expand All @@ -881,7 +901,7 @@ public void classInclusion() throws Exception {
*/
@Test
public void packageInclusion() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useEnforceAllowlistEnabled(TRUE.toString());

TestBean2 bean = new TestBean2();
Method method = TestBean2.class.getMethod("getData");
Expand All @@ -898,7 +918,7 @@ public void packageInclusion() throws Exception {
*/
@Test
public void classInclusion_subclass() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useEnforceAllowlistEnabled(TRUE.toString());
sma.useAllowlistClasses(TestBean2.class.getName());

TestBean2 bean = new TestBean2();
Expand All @@ -912,7 +932,7 @@ public void classInclusion_subclass() throws Exception {
*/
@Test
public void classInclusion_subclass_both() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useEnforceAllowlistEnabled(TRUE.toString());
sma.useAllowlistClasses(String.join(",", TestBean.class.getName(), TestBean2.class.getName()));

TestBean2 bean = new TestBean2();
Expand All @@ -927,7 +947,7 @@ public void classInclusion_subclass_both() throws Exception {
*/
@Test
public void packageInclusion_subclass() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useEnforceAllowlistEnabled(TRUE.toString());
sma.useAllowlistPackageNames(TestBean2.class.getPackage().getName());

TestBean2 bean = new TestBean2();
Expand All @@ -944,8 +964,8 @@ public void classInclusion_hibernateProxy_disallowProxyObjectAccess() throws Exc
FooBarInterface proxyObject = mockHibernateProxy(new FooBar(), FooBarInterface.class);
Method proxyMethod = proxyObject.getClass().getMethod("fooLogic");

sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString());
sma.useEnforceAllowlistEnabled(TRUE.toString());
sma.useDisallowProxyObjectAccess(TRUE.toString());
sma.useAllowlistClasses(FooBar.class.getName());

assertFalse(sma.checkAllowlist(proxyObject, proxyMethod));
Expand All @@ -960,7 +980,7 @@ public void classInclusion_hibernateProxy_allowProxyObjectAccess() throws Except
FooBarInterface proxyObject = mockHibernateProxy(new FooBar(), FooBarInterface.class);
Method proxyMethod = proxyObject.getClass().getMethod("fooLogic");

sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useEnforceAllowlistEnabled(TRUE.toString());
sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString());
sma.useAllowlistClasses(FooBar.class.getName());

Expand All @@ -969,7 +989,7 @@ public void classInclusion_hibernateProxy_allowProxyObjectAccess() throws Except

@Test
public void packageInclusion_subclass_both() throws Exception {
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useEnforceAllowlistEnabled(TRUE.toString());
sma.useAllowlistPackageNames(String.join(",",
TestBean.class.getPackage().getName(),
TestBean2.class.getPackage().getName()));
Expand Down Expand Up @@ -1083,7 +1103,7 @@ enum MyValues {
ONE, TWO, THREE;

public static MyValues[] values(String notUsed) {
return new MyValues[] {ONE, TWO, THREE};
return new MyValues[]{ONE, TWO, THREE};
}
}

Expand Down
14 changes: 14 additions & 0 deletions core/src/test/java/org/apache/struts2/ognl/ToBeInstanced.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.apache.struts2.ognl;

public class ToBeInstanced {

private final String value;

public ToBeInstanced(String value) {
this.value = value;
}

public String getValue() {
return value;
}
}