From 11999d148ef879683195a8327ac67001d391a0c2 Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Tue, 1 Oct 2019 14:57:46 +0330 Subject: [PATCH 1/6] WW-5041 Upgrade to OGNL 3.1.26 and adapt to its new features (cherry picked from commit 1de94b2) --- .../xwork2/inject/ContainerImpl.java | 9 +++-- .../opensymphony/xwork2/ognl/OgnlUtil.java | 16 +++++++++ .../xwork2/ognl/OgnlValueStack.java | 10 +++++- .../org/apache/struts2/StrutsConstants.java | 3 ++ .../apache/struts2/views/jsp/ui/OgnlTool.java | 7 ++++ .../org/apache/struts2/default.properties | 4 +++ core/src/main/resources/struts-default.xml | 5 +++ .../xwork2/ognl/OgnlValueStackTest.java | 35 +++++++++++++++++++ .../struts2/result/StreamResultTest.java | 17 +++++---- pom.xml | 2 +- 10 files changed, 97 insertions(+), 11 deletions(-) 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..1fa6b94b9 100644 --- a/core/src/main/java/com/opensymphony/xwork2/inject/ContainerImpl.java +++ b/core/src/main/java/com/opensymphony/xwork2/inject/ContainerImpl.java @@ -164,7 +164,8 @@ class ContainerImpl implements Container { public FieldInjector(ContainerImpl container, Field field, String name) throws MissingDependencyException { this.field = field; - if (!field.isAccessible()) { + if ((!Modifier.isPublic(field.getModifiers()) || !Modifier.isPublic(field.getDeclaringClass().getModifiers())) + && !field.isAccessible()) { SecurityManager sm = System.getSecurityManager(); try { if (sm != null) { @@ -256,7 +257,8 @@ class ContainerImpl implements Container { public MethodInjector(ContainerImpl container, Method method, String name) throws MissingDependencyException { this.method = method; - if (!method.isAccessible()) { + if ((!Modifier.isPublic(method.getModifiers()) || !Modifier.isPublic(method.getDeclaringClass().getModifiers())) + && !method.isAccessible()) { SecurityManager sm = System.getSecurityManager(); try { if (sm != null) { @@ -306,7 +308,8 @@ class ContainerImpl implements Container { this.implementation = implementation; constructor = findConstructorIn(implementation); - if (!constructor.isAccessible()) { + if ((!Modifier.isPublic(constructor.getModifiers()) || !Modifier.isPublic(constructor.getDeclaringClass().getModifiers())) + && !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..ffa8685cc 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,19 @@ public class OgnlUtil { this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } + /** + * @param maxLength Injects the Struts OGNL maximum expression length. + */ + @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH, required = false) + protected void applyExpressionMaxLength(String maxLength) { + if (maxLength == null || maxLength.isEmpty()) { + // user is going to disable this functionality + Ognl.applyExpressionMaxLength(null); + } else { + Ognl.applyExpressionMaxLength(Integer.parseInt(maxLength)); + } + } + public boolean isDisallowProxyMemberAccess() { return disallowProxyMemberAccess; } @@ -754,6 +767,9 @@ public class OgnlUtil { setValue(name, context, o, value); } catch (OgnlException e) { Throwable reason = e.getReason(); + if (reason instanceof SecurityException) { + LOG.warn("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..57bf6d1fd 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.getReason() instanceof SecurityException) { + LOG.warn("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.getReason() instanceof SecurityException) { + LOG.warn("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..2fc0325c0 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.warn("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..a78652c24 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -223,4 +223,8 @@ 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 +struts.ognl.expressionMaxLength=200 + ### END SNIPPET: complete_file diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 7575b27c1..ba9531de8 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -55,13 +55,18 @@ 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..2c30e9a79 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.*; @@ -39,11 +40,13 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +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 +351,38 @@ public class OgnlValueStackTest extends XWorkTestCase { } } + public void testFailOnTooLongExpressionWithDefaultProperties() { + loadConfigurationProviders(new DefaultPropertiesProvider()); + 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); + } + } + + public void testNotFailOnTooLongValueWithDefaultProperties() { + loadConfigurationProviders(new DefaultPropertiesProvider()); + Integer repeat = Integer.parseInt( + container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH)); + + OgnlValueStack vs = createValueStack(); + + Dog dog = new Dog(); + vs.push(dog); + + String value = StringUtils.repeat('.', repeat + 1); + + vs.setValue("name", value); + + assertEquals(value, dog.getName()); + } + 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 From 84cf17ebb1a63188bb7204636d08ae4f7544f09d Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+jcgh4164838gh792c124b5@users.noreply.github.com> Date: Sat, 2 Nov 2019 21:01:09 +0330 Subject: [PATCH 2/6] Minor follow-up changes to PR #371 - added some additional exclusions in struts-default.xml. - added log warning that specifies the value of maxLength involved if applyExpressionMaxLength(maxLength) fails. - added null guards to two handleOgnlException() methods that could result in an NPE with #371 changes (a null OgnlException parameter was permissible previously, correct or not). (cherry picked from commit e2b644a) --- .../com/opensymphony/xwork2/ognl/OgnlUtil.java | 15 ++++++++++----- .../opensymphony/xwork2/ognl/OgnlValueStack.java | 4 ++-- core/src/main/resources/struts-default.xml | 4 ++++ 3 files changed, 16 insertions(+), 7 deletions(-) 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 ffa8685cc..52437895a 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -194,11 +194,16 @@ public class OgnlUtil { */ @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH, required = false) protected void applyExpressionMaxLength(String maxLength) { - if (maxLength == null || maxLength.isEmpty()) { - // user is going to disable this functionality - Ognl.applyExpressionMaxLength(null); - } else { - Ognl.applyExpressionMaxLength(Integer.parseInt(maxLength)); + try { + if (maxLength == null || maxLength.isEmpty()) { + // user is going to disable this functionality + Ognl.applyExpressionMaxLength(null); + } else { + Ognl.applyExpressionMaxLength(Integer.parseInt(maxLength)); + } + } catch (Exception ex) { + LOG.warn("Unable to set OGNL Expression Max Length {}.", maxLength); // Help configuration debugging. + throw ex; } } 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 57bf6d1fd..c44744b59 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -203,7 +203,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS } protected void handleOgnlException(String expr, Object value, boolean throwExceptionOnFailure, OgnlException e) { - if (e.getReason() instanceof SecurityException) { + if (e != null && e.getReason() instanceof SecurityException) { LOG.warn("Could not evaluate this expression due to security constraints: [{}]", expr, e); } boolean shouldLog = shouldLogMissingPropertyWarning(e); @@ -329,7 +329,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS protected Object handleOgnlException(String expr, boolean throwExceptionOnFailure, OgnlException e) { Object ret = null; - if (e.getReason() instanceof SecurityException) { + if (e != null && e.getReason() instanceof SecurityException) { LOG.warn("Could not evaluate this expression due to security constraints: [{}]", expr, e); } else { ret = findInContext(expr); diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index ba9531de8..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" /> @@ -56,11 +57,14 @@ value=" ognl., java.io., + java.net., + java.nio., javax., freemarker.core., freemarker.template., freemarker.ext.jsp., freemarker.ext.rhino., + sun.misc., sun.reflect., javassist., org.apache.velocity., From a23db416e5b379c7209711f1b8497effc5e79be3 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+jcgh4164838gh792c124b5@users.noreply.github.com> Date: Sat, 2 Nov 2019 21:41:21 +0330 Subject: [PATCH 3/6] Additional change - added unit test (hoping to make coveralls happy). (cherry picked from commit dd6d206) --- .../xwork2/ognl/OgnlUtilTest.java | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) 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..a7462f8a6 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,40 @@ public class OgnlUtilTest extends XWorkTestCase { } } + /** + * Test OGNL Expression Max Length feature setting via OgnlUtil. + * + * @since 2.5.21 + */ + public void testApplyExpressionMaxLength() { + try { + ognlUtil.applyExpressionMaxLength(null); + } catch (Exception ex) { + fail ("applyExpressionMaxLength did not accept null maxlength string ?"); + } + try { + ognlUtil.applyExpressionMaxLength(""); + } catch (Exception ex) { + fail ("applyExpressionMaxLength did not accept empty maxlength string ?"); + } + 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 ?"); + } + } + private void internalTestInitialEmptyOgnlUtilExclusions(OgnlUtil ognlUtilParam) throws Exception { Set> excludedClasses = ognlUtilParam.getExcludedClasses(); assertNotNull("parameter (default) exluded classes null?", excludedClasses); From 28a7912acd6faec7c6a13b229ef1c15f410c1248 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+jcgh4164838gh792c124b5@users.noreply.github.com> Date: Sat, 16 Nov 2019 20:09:19 +0330 Subject: [PATCH 4/6] Disable expressionMaxLength by default for Struts 2.5.x. (#380) * Disable struts.ognl.expressionMaxLength by default for Struts 2.5.x. - Commented out struts.ognl.expressionMaxLength line in default.properties and provided in-place comments about its usage. - Changed OgnlValueStack.handleOgnlException() methods to output error instead of warn for failures to evaluate expressions due to security constraints. - Updated existing unit tests to compensate for change in default behaviour. - Added a unit test to confirm default behaviour for struts.ognl.expressionMaxLength is disabled. * Updated commit for disable struts.ognl.expressionMaxLength by default for Struts 2.5.x - Additional unit test requested by Y. Zamani for code coverage. - Corrected accidental use of wrong (static) toString method in one test. - Addition of a minimum struts.ognl.expressionMaxLength value permitted by Struts 2 (128). Any value smaller than that is likely to be a configuration error and if a user really wishes to force it they may go to OGNL directly to do so. * Updated commit for disable struts.ognl.expressionMaxLength by default for Struts 2.5.x - Removed minimum struts.ognl.expressionMaxLength (restored to previous behaviour) as requested by Y. Zamani and L. Lenart. - Updated unit tests to compensate for the above change. - Changed log output from warn to error in applyExpressionMaxLength() on exception since it will likely be considered a fatal condition. (cherry picked from commit 3dfc5a4) --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 2 +- .../xwork2/ognl/OgnlValueStack.java | 4 +- .../org/apache/struts2/default.properties | 11 ++- .../xwork2/ognl/OgnlUtilTest.java | 72 ++++++++++----- .../xwork2/ognl/OgnlValueStackTest.java | 87 ++++++++++++++++--- 5 files changed, 134 insertions(+), 42 deletions(-) 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 52437895a..27f8fbbf3 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -202,7 +202,7 @@ public class OgnlUtil { Ognl.applyExpressionMaxLength(Integer.parseInt(maxLength)); } } catch (Exception ex) { - LOG.warn("Unable to set OGNL Expression Max Length {}.", maxLength); // Help configuration debugging. + LOG.error("Unable to set OGNL Expression Max Length {}.", maxLength); // Help configuration debugging. throw ex; } } 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 c44744b59..c610c4269 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -204,7 +204,7 @@ 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.warn("Could not evaluate this expression due to security constraints: [{}]", expr, e); + LOG.error("Could not evaluate this expression due to security constraints: [{}]", expr, e); } boolean shouldLog = shouldLogMissingPropertyWarning(e); String msg = null; @@ -330,7 +330,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS protected Object handleOgnlException(String expr, boolean throwExceptionOnFailure, OgnlException e) { Object ret = null; if (e != null && e.getReason() instanceof SecurityException) { - LOG.warn("Could not evaluate this expression due to security constraints: [{}]", expr, e); + LOG.error("Could not evaluate this expression due to security constraints: [{}]", expr, e); } else { ret = findInContext(expr); } diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index a78652c24..3fe298a97 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -224,7 +224,14 @@ struts.ognl.enableExpressionCache=true ### 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 -struts.ognl.expressionMaxLength=200 +### 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/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java index a7462f8a6..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,27 @@ 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. * @@ -1254,30 +1275,35 @@ public class OgnlUtilTest extends XWorkTestCase { */ 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); - } catch (Exception ex) { - fail ("applyExpressionMaxLength did not accept null maxlength string ?"); - } - try { - ognlUtil.applyExpressionMaxLength(""); - } catch (Exception ex) { - fail ("applyExpressionMaxLength did not accept empty maxlength string ?"); - } - 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 ?"); } } 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 2c30e9a79..8fd8cfbf4 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -39,6 +39,7 @@ 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; @@ -351,36 +352,94 @@ public class OgnlValueStackTest extends XWorkTestCase { } } - public void testFailOnTooLongExpressionWithDefaultProperties() { + 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()); - Integer repeat = Integer.parseInt( - container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH)); + + 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("Failed to throw exception on too long expression"); + 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 SecurityException); + assertTrue(((OgnlException) ex.getCause()).getReason() instanceof ParseException); } } public void testNotFailOnTooLongValueWithDefaultProperties() { - loadConfigurationProviders(new DefaultPropertiesProvider()); - Integer repeat = Integer.parseInt( - container.getInstance(String.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH)); + try { + loadConfigurationProviders(new DefaultPropertiesProvider()); - OgnlValueStack vs = createValueStack(); + 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 - Dog dog = new Dog(); - vs.push(dog); + // 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() + " ?"); + } - String value = StringUtils.repeat('.', repeat + 1); + OgnlValueStack vs = createValueStack(); - vs.setValue("name", value); + Dog dog = new Dog(); + vs.push(dog); - assertEquals(value, dog.getName()); + 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() { From 7dddaf51281536bd843dce23f4a20f29f5ec8a50 Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Tue, 24 Dec 2019 10:50:46 +0330 Subject: [PATCH 5/6] refactor duplicate code. add log info. --- .../opensymphony/xwork2/inject/ContainerImpl.java | 14 ++++++++------ .../com/opensymphony/xwork2/ognl/OgnlUtil.java | 5 +++-- 2 files changed, 11 insertions(+), 8 deletions(-) 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 1fa6b94b9..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,8 +169,7 @@ class ContainerImpl implements Container { public FieldInjector(ContainerImpl container, Field field, String name) throws MissingDependencyException { this.field = field; - if ((!Modifier.isPublic(field.getModifiers()) || !Modifier.isPublic(field.getDeclaringClass().getModifiers())) - && !field.isAccessible()) { + if (isNotPublic(field) && !field.isAccessible()) { SecurityManager sm = System.getSecurityManager(); try { if (sm != null) { @@ -257,8 +261,7 @@ class ContainerImpl implements Container { public MethodInjector(ContainerImpl container, Method method, String name) throws MissingDependencyException { this.method = method; - if ((!Modifier.isPublic(method.getModifiers()) || !Modifier.isPublic(method.getDeclaringClass().getModifiers())) - && !method.isAccessible()) { + if (isNotPublic(method) && !method.isAccessible()) { SecurityManager sm = System.getSecurityManager(); try { if (sm != null) { @@ -308,8 +311,7 @@ class ContainerImpl implements Container { this.implementation = implementation; constructor = findConstructorIn(implementation); - if ((!Modifier.isPublic(constructor.getModifiers()) || !Modifier.isPublic(constructor.getDeclaringClass().getModifiers())) - && !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 27f8fbbf3..cfcd4bce5 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -190,16 +190,17 @@ public class OgnlUtil { } /** - * @param maxLength Injects the Struts OGNL maximum expression length. + * @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()) { - // user is going to disable this functionality 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. From 93f9cf6bfa2bd19996edb9616003c28d2c3bb2de Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Tue, 24 Dec 2019 10:56:30 +0330 Subject: [PATCH 6/6] increase security log levels to error --- core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 2 +- .../src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 cfcd4bce5..01d5374d9 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -774,7 +774,7 @@ public class OgnlUtil { } catch (OgnlException e) { Throwable reason = e.getReason(); if (reason instanceof SecurityException) { - LOG.warn("Could not evaluate this expression due to security constraints: [{}]", name, e); + 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/org/apache/struts2/views/jsp/ui/OgnlTool.java b/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java index 2fc0325c0..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 @@ -48,7 +48,7 @@ public class OgnlTool { return ognlUtil.getValue(expr, ActionContext.getContext().getContextMap(), context); } catch (OgnlException e) { if (e.getReason() instanceof SecurityException) { - LOG.warn("Could not evaluate this expression due to security constraints: [{}]", expr, e); + LOG.error("Could not evaluate this expression due to security constraints: [{}]", expr, e); } return null; }