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..dcfb19277f 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -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){ + 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)) { @@ -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; } @@ -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 */ @@ -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)); } /** diff --git a/core/src/test/java/org/apache/struts2/ognl/OgnlUtilTest.java b/core/src/test/java/org/apache/struts2/ognl/OgnlUtilTest.java index 9c526ffcee..921f238871 100644 --- a/core/src/test/java/org/apache/struts2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/OgnlUtilTest.java @@ -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. 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..c1cf2f7310 100644 --- a/core/src/test/java/org/apache/struts2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/OgnlValueStackTest.java @@ -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; @@ -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()); } @@ -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); @@ -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; diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java index e3d3fa589a..b0bcd65cd3 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java @@ -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; @@ -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; @@ -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); @@ -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()); @@ -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()); @@ -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 @@ -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"); @@ -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"); @@ -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(); @@ -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(); @@ -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(); @@ -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)); @@ -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()); @@ -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())); @@ -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}; } } diff --git a/core/src/test/java/org/apache/struts2/ognl/ToBeInstanced.java b/core/src/test/java/org/apache/struts2/ognl/ToBeInstanced.java new file mode 100644 index 0000000000..20919e86d1 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/ToBeInstanced.java @@ -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; + } +}