diff --git a/core/src/main/java/com/opensymphony/xwork2/inject/ContainerImpl.java b/core/src/main/java/com/opensymphony/xwork2/inject/ContainerImpl.java
index 9505c64ef..7b45097c3 100644
--- a/core/src/main/java/com/opensymphony/xwork2/inject/ContainerImpl.java
+++ b/core/src/main/java/com/opensymphony/xwork2/inject/ContainerImpl.java
@@ -155,6 +155,11 @@ class ContainerImpl implements Container {
return Modifier.isStatic(member.getModifiers());
}
+ private static boolean isNotPublic(Member member) {
+ return !Modifier.isPublic(member.getModifiers()) ||
+ !Modifier.isPublic(member.getDeclaringClass().getModifiers());
+ }
+
static class FieldInjector implements Injector {
final Field field;
@@ -164,7 +169,7 @@ class ContainerImpl implements Container {
public FieldInjector(ContainerImpl container, Field field, String name)
throws MissingDependencyException {
this.field = field;
- if (!field.isAccessible()) {
+ if (isNotPublic(field) && !field.isAccessible()) {
SecurityManager sm = System.getSecurityManager();
try {
if (sm != null) {
@@ -256,7 +261,7 @@ class ContainerImpl implements Container {
public MethodInjector(ContainerImpl container, Method method, String name) throws MissingDependencyException {
this.method = method;
- if (!method.isAccessible()) {
+ if (isNotPublic(method) && !method.isAccessible()) {
SecurityManager sm = System.getSecurityManager();
try {
if (sm != null) {
@@ -306,7 +311,7 @@ class ContainerImpl implements Container {
this.implementation = implementation;
constructor = findConstructorIn(implementation);
- if (!constructor.isAccessible()) {
+ if (isNotPublic(constructor) && !constructor.isAccessible()) {
SecurityManager sm = System.getSecurityManager();
try {
if (sm != null) {
diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
index d5720e910..01d5374d9 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
@@ -189,6 +189,25 @@ public class OgnlUtil {
this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess);
}
+ /**
+ * @param maxLength Injects the Struts OGNL expression maximum length.
+ */
+ @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH, required = false)
+ protected void applyExpressionMaxLength(String maxLength) {
+ try {
+ if (maxLength == null || maxLength.isEmpty()) {
+ Ognl.applyExpressionMaxLength(null);
+ LOG.info("OGNL Expression Max Length disabled.");
+ } else {
+ Ognl.applyExpressionMaxLength(Integer.parseInt(maxLength));
+ LOG.info("OGNL Expression Max Length enabled with {}.", maxLength);
+ }
+ } catch (Exception ex) {
+ LOG.error("Unable to set OGNL Expression Max Length {}.", maxLength); // Help configuration debugging.
+ throw ex;
+ }
+ }
+
public boolean isDisallowProxyMemberAccess() {
return disallowProxyMemberAccess;
}
@@ -754,6 +773,9 @@ public class OgnlUtil {
setValue(name, context, o, value);
} catch (OgnlException e) {
Throwable reason = e.getReason();
+ if (reason instanceof SecurityException) {
+ LOG.error("Could not evaluate this expression due to security constraints: [{}]", name, e);
+ }
String msg = "Caught OgnlException while setting property '" + name + "' on type '" + o.getClass().getName() + "'.";
Throwable exception = (reason == null) ? e : reason;
diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
index 9fc786858..c610c4269 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
@@ -203,6 +203,9 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS
}
protected void handleOgnlException(String expr, Object value, boolean throwExceptionOnFailure, OgnlException e) {
+ if (e != null && e.getReason() instanceof SecurityException) {
+ LOG.error("Could not evaluate this expression due to security constraints: [{}]", expr, e);
+ }
boolean shouldLog = shouldLogMissingPropertyWarning(e);
String msg = null;
if (throwExceptionOnFailure || shouldLog) {
@@ -325,7 +328,12 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS
}
protected Object handleOgnlException(String expr, boolean throwExceptionOnFailure, OgnlException e) {
- Object ret = findInContext(expr);
+ Object ret = null;
+ if (e != null && e.getReason() instanceof SecurityException) {
+ LOG.error("Could not evaluate this expression due to security constraints: [{}]", expr, e);
+ } else {
+ ret = findInContext(expr);
+ }
if (ret == null) {
if (shouldLogMissingPropertyWarning(e)) {
LOG.warn("Could not find property [{}]!", expr, e);
diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java
index 5a47ddb9f..3f9da83a0 100644
--- a/core/src/main/java/org/apache/struts2/StrutsConstants.java
+++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java
@@ -264,6 +264,9 @@ public final class StrutsConstants {
/** Enables evaluation of OGNL expressions */
public static final String STRUTS_ENABLE_OGNL_EVAL_EXPRESSION = "struts.ognl.enableOGNLEvalExpression";
+ /** The maximum length of an expression (OGNL) */
+ public static final String STRUTS_OGNL_EXPRESSION_MAX_LENGTH = "struts.ognl.expressionMaxLength";
+
/** Disables {@link org.apache.struts2.dispatcher.StrutsRequestWrapper} request attribute value stack lookup (JSTL accessibility) */
public static final String STRUTS_DISABLE_REQUEST_ATTRIBUTE_VALUE_STACK_LOOKUP = "struts.disableRequestAttributeValueStackLookup";
diff --git a/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java b/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java
index 9f6e8513a..e77665210 100644
--- a/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java
+++ b/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java
@@ -23,12 +23,16 @@ import ognl.OgnlException;
import com.opensymphony.xwork2.inject.Inject;
import com.opensymphony.xwork2.ognl.OgnlUtil;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
/**
* FIXME: remove?
*/
public class OgnlTool {
+ private static final Logger LOG = LogManager.getLogger(OgnlTool.class);
+
private OgnlUtil ognlUtil;
public OgnlTool() {
@@ -43,6 +47,9 @@ public class OgnlTool {
try {
return ognlUtil.getValue(expr, ActionContext.getContext().getContextMap(), context);
} catch (OgnlException e) {
+ if (e.getReason() instanceof SecurityException) {
+ LOG.error("Could not evaluate this expression due to security constraints: [{}]", expr, e);
+ }
return null;
}
}
diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties
index a441e77ba..3fe298a97 100644
--- a/core/src/main/resources/org/apache/struts2/default.properties
+++ b/core/src/main/resources/org/apache/struts2/default.properties
@@ -223,4 +223,15 @@ struts.ognl.enableExpressionCache=true
### Indicates if Dispatcher should handle unexpected exceptions by calling sendError()
### or simply rethrow it as a ServletException to allow future processing by other frameworks like Spring Security
struts.handle.exception=true
+
+### Applies maximum length allowed on OGNL expressions for security enhancement (optional)
+###
+### **WARNING**: If developers enable this option (by configuration) they should make sure that they understand the implications of setting
+### struts.ognl.expressionMaxLength. They must choose a value large enough to permit ALL valid OGNL expressions used within the application.
+### Values larger than the 200-400 range have diminishing security value (at which point it is really only a "style guard" for long OGNL
+### expressions in an application. Setting a value of null or "" will also disable the feature.
+###
+### NOTE: The sample line below is *INTENTIONALLY* commented out, as this feature is disabled by default.
+# struts.ognl.expressionMaxLength=256
+
### END SNIPPET: complete_file
diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml
index 7575b27c1..ed850bdc5 100644
--- a/core/src/main/resources/struts-default.xml
+++ b/core/src/main/resources/struts-default.xml
@@ -45,6 +45,7 @@
java.lang.ClassLoader,
java.lang.Shutdown,
java.lang.ProcessBuilder,
+ sun.misc.Unsafe,
com.opensymphony.xwork2.ActionContext" />
@@ -55,13 +56,21 @@
diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
index 6662ad201..5efccbf74 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
@@ -1247,6 +1247,66 @@ public class OgnlUtilTest extends XWorkTestCase {
}
}
+ /**
+ * Test OGNL Expression Max Length feature setting via OgnlUtil is disabled by default (in default.properties).
+ *
+ * @since 2.5.21
+ */
+ public void testDefaultExpressionMaxLengthDisabled() {
+ final String LONG_OGNL_EXPRESSION = "true == ThisIsAReallyLongOGNLExpressionOfRepeatedGarbageText." + new String(new char[65535]).replace('\0', 'A'); // Expression larger than 64KB.
+ try {
+ Object compileResult = ognlUtil.compile(LONG_OGNL_EXPRESSION);
+ assertNotNull("Long OGNL expression compilation produced a null result ?", compileResult);
+ } catch (OgnlException oex) {
+ if (oex.getReason() instanceof SecurityException) {
+ fail ("Unable to compile expression (unexpected). 'struts.ognl.expressionMaxLength' may have accidentally been enabled by default. Exception: " + oex);
+ } else {
+ fail ("Unable to compile expression (unexpected). Exception: " + oex);
+ }
+ } catch (Exception ex) {
+ fail ("Unable to compile expression (unexpected). Exception: " + ex);
+ }
+ }
+
+ /**
+ * Test OGNL Expression Max Length feature setting via OgnlUtil.
+ *
+ * @since 2.5.21
+ */
+ public void testApplyExpressionMaxLength() {
+ try {
+ try {
+ ognlUtil.applyExpressionMaxLength(null);
+ } catch (Exception ex) {
+ fail ("applyExpressionMaxLength did not accept null maxlength string (disable feature) ?");
+ }
+ try {
+ ognlUtil.applyExpressionMaxLength("");
+ } catch (Exception ex) {
+ fail ("applyExpressionMaxLength did not accept empty maxlength string (disable feature) ?");
+ }
+ try {
+ ognlUtil.applyExpressionMaxLength("-1");
+ fail ("applyExpressionMaxLength accepted negative maxlength string ?");
+ } catch (IllegalArgumentException iae) {
+ // Expected rejection of -ive length.
+ }
+ try {
+ ognlUtil.applyExpressionMaxLength("0");
+ } catch (Exception ex) {
+ fail ("applyExpressionMaxLength did not accept maxlength string 0 ?");
+ }
+ try {
+ ognlUtil.applyExpressionMaxLength(Integer.toString(Integer.MAX_VALUE, 10));
+ } catch (Exception ex) {
+ fail ("applyExpressionMaxLength did not accept MAX_VALUE maxlength string ?");
+ }
+ } finally {
+ // Reset expressionMaxLength value to default (disabled)
+ ognlUtil.applyExpressionMaxLength(null);
+ }
+ }
+
private void internalTestInitialEmptyOgnlUtilExclusions(OgnlUtil ognlUtilParam) throws Exception {
Set> excludedClasses = ognlUtilParam.getExcludedClasses();
assertNotNull("parameter (default) exluded classes null?", excludedClasses);
diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
index 05b3b1a0e..8fd8cfbf4 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
@@ -29,6 +29,7 @@ import com.opensymphony.xwork2.util.*;
import com.opensymphony.xwork2.util.Foo;
import com.opensymphony.xwork2.util.location.LocatableProperties;
import com.opensymphony.xwork2.util.reflection.ReflectionContextState;
+import ognl.OgnlException;
import ognl.PropertyAccessor;
import java.io.*;
@@ -38,12 +39,15 @@ import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
+import ognl.ParseException;
+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.config.DefaultPropertiesProvider;
/**
@@ -348,6 +352,96 @@ public class OgnlValueStackTest extends XWorkTestCase {
}
}
+ public void testFailOnTooLongExpressionLongerThan192_ViaOverriddenProperty() {
+ try {
+ loadConfigurationProviders(new StubConfigurationProvider() {
+ @Override
+ public void register(ContainerBuilder builder,
+ LocatableProperties props) throws ConfigurationException {
+ props.setProperty(StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH, "192");
+ }
+ });
+ Integer repeat = Integer.parseInt(
+ container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH));
+
+ OgnlValueStack vs = createValueStack();
+ try {
+ vs.findValue(StringUtils.repeat('.', repeat + 1), true);
+ fail("Failed to throw exception on too long expression");
+ } catch (Exception ex) {
+ assertTrue(ex.getCause() instanceof OgnlException);
+ assertTrue(((OgnlException) ex.getCause()).getReason() instanceof SecurityException);
+ }
+ } finally {
+ // Reset expressionMaxLength value to default (disabled)
+ ognlUtil.applyExpressionMaxLength(null);
+ }
+ }
+
+ public void testNotFailOnTooLongExpressionWithDefaultProperties() {
+ loadConfigurationProviders(new DefaultPropertiesProvider());
+
+ Object defaultMaxLengthFromConfiguration = container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH);
+ if (defaultMaxLengthFromConfiguration != null) {
+ assertTrue("non-null defaultMaxLengthFromConfiguration not a String ?", defaultMaxLengthFromConfiguration instanceof String);
+ assertTrue("non-null defaultMaxLengthFromConfiguration not empty string by default ?", ((String) defaultMaxLengthFromConfiguration).length() == 0);
+ } else {
+ assertNull("defaultMaxLengthFromConfiguration not null ?", defaultMaxLengthFromConfiguration);
+ }
+ // Original test logic was to confirm failure of exceeding the default value. Now the feature should be disabled by default,
+ // so this test's expectations are now changed.
+ Integer repeat = Integer.valueOf(256); // Since maxlength is disabled by default, just choose an arbitrary value for test
+
+ OgnlValueStack vs = createValueStack();
+ try {
+ vs.findValue(StringUtils.repeat('.', repeat + 1), true);
+ fail("findValue did not throw any exception (should either fail as invalid expression syntax or security exception) ?");
+ } catch (Exception ex) {
+ // If STRUTS_OGNL_EXPRESSION_MAX_LENGTH feature is disabled (default), the parse should fail due to a reason of invalid expression syntax
+ // with ParseException. Previously when it was enabled the reason for the failure would have been SecurityException.
+ assertTrue(ex.getCause() instanceof OgnlException);
+ assertTrue(((OgnlException) ex.getCause()).getReason() instanceof ParseException);
+ }
+ }
+
+ public void testNotFailOnTooLongValueWithDefaultProperties() {
+ try {
+ loadConfigurationProviders(new DefaultPropertiesProvider());
+
+ Object defaultMaxLengthFromConfiguration = container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH);
+ if (defaultMaxLengthFromConfiguration != null) {
+ assertTrue("non-null defaultMaxLengthFromConfiguration not a String ?", defaultMaxLengthFromConfiguration instanceof String);
+ assertTrue("non-null defaultMaxLengthFromConfiguration not empty string by default ?", ((String) defaultMaxLengthFromConfiguration).length() == 0);
+ } else {
+ assertNull("defaultMaxLengthFromConfiguration not null ?", defaultMaxLengthFromConfiguration);
+ }
+ // Original test logic is unchanged (testing that values can be larger than maximum expression length), but since the feature is disabled by
+ // default we will now have to enable it with an arbitrary value, test, and reset it to disabled.
+ Integer repeat = Integer.valueOf(256); // Since maxlength is disabled by default, just choose an arbitrary value for test
+
+ // Apply a non-default value for expressionMaxLength (as it should be disabled by default)
+ try {
+ ognlUtil.applyExpressionMaxLength(repeat.toString());
+ } catch (Exception ex) {
+ fail ("applyExpressionMaxLength did not accept maxlength string " + repeat.toString() + " ?");
+ }
+
+ OgnlValueStack vs = createValueStack();
+
+ Dog dog = new Dog();
+ vs.push(dog);
+
+ String value = StringUtils.repeat('.', repeat + 1);
+
+ vs.setValue("name", value);
+
+ assertEquals(value, dog.getName());
+ } finally {
+ // Reset expressionMaxLength value to default (disabled)
+ ognlUtil.applyExpressionMaxLength(null);
+ }
+ }
+
public void testFailsOnMethodThatThrowsException() {
SimpleAction action = new SimpleAction();
OgnlValueStack stack = createValueStack();
diff --git a/core/src/test/java/org/apache/struts2/result/StreamResultTest.java b/core/src/test/java/org/apache/struts2/result/StreamResultTest.java
index 5a147fd32..1b46d0bd9 100644
--- a/core/src/test/java/org/apache/struts2/result/StreamResultTest.java
+++ b/core/src/test/java/org/apache/struts2/result/StreamResultTest.java
@@ -246,12 +246,19 @@ public class StreamResultTest extends StrutsInternalTestCase {
public class MyImageAction implements Action {
- public InputStream getStreamForImage() throws Exception {
+ FileInputStream streamForImage;
+ long contentLength;
+
+ public MyImageAction() throws Exception {
// just use src/test/log4j2.xml as test file
URL url = ClassLoaderUtil.getResource("log4j2.xml", StreamResultTest.class);
File file = new File(new URI(url.toString()));
- FileInputStream fis = new FileInputStream(file);
- return fis;
+ streamForImage = new FileInputStream(file);
+ contentLength = file.length();
+ }
+
+ public InputStream getStreamForImage() throws Exception {
+ return streamForImage;
}
public String execute() throws Exception {
@@ -259,9 +266,7 @@ public class StreamResultTest extends StrutsInternalTestCase {
}
public long getContentLength() throws Exception {
- URL url = ClassLoaderUtil.getResource("log4j2.xml", StreamResultTest.class);
- File file = new File(new URI(url.toString()));
- return file.length();
+ return contentLength;
}
public String getStreamForImageAsString() {
diff --git a/pom.xml b/pom.xml
index e64d2472e..840ca36f2 100644
--- a/pom.xml
+++ b/pom.xml
@@ -102,7 +102,7 @@
7.2
2.10.1
2.12.1
- 3.2.10
+ 3.2.12
1.7.29
4.3.25.RELEASE
3.0.8