From 3dfc5a4074710e66e516d3d1adcacc1c2dc025f1 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Sat, 16 Nov 2019 11:39:19 -0500 Subject: [PATCH] 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. --- .../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 bc53c758e..83e5833d0 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -197,7 +197,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 93f82425e..6e3bd0209 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -205,7 +205,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; @@ -331,7 +331,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 23f159ef4..c84452d2e 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -220,7 +220,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 997955306..701a1f814 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -1232,6 +1232,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. * @@ -1239,30 +1260,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 c601a9733..566429f9e 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -40,6 +40,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; @@ -350,36 +351,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() {