Merge pull request #390 from yasserzamani/ognl3_2_12

WW-5050 cherry pick #371 and related commits

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) {

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;
 

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);

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";
 

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;
         }
     }

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

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" />
 
     <!-- this must be valid regex, each '.' in package name must be escaped! -->
@@ -55,13 +56,21 @@
     <constant name="struts.excludedPackageNames"
               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.,
                 org.objectweb.asm.,
+                org.springframework.context.,
+                com.opensymphony.xwork2.inject.,
                 com.opensymphony.xwork2.ognl.,
                 com.opensymphony.xwork2.security.,
                 com.opensymphony.xwork2.util." />

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<Class<?>> excludedClasses = ognlUtilParam.getExcludedClasses();
         assertNotNull("parameter (default) exluded classes null?", excludedClasses);

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();

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() {

pom.xml

@@ -102,7 +102,7 @@
         <asm.version>7.2</asm.version>
         <jackson.version>2.10.1</jackson.version>
         <log4j2.version>2.12.1</log4j2.version>
-        <ognl.version>3.2.10</ognl.version>
+        <ognl.version>3.2.12</ognl.version>
         <slf4j.version>1.7.29</slf4j.version>
         <spring.platformVersion>4.3.25.RELEASE</spring.platformVersion>
         <tiles.version>3.0.8</tiles.version>