From 1de94b20923e349e02eb79dc45ce29245435416e Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Tue, 1 Oct 2019 14:57:46 +0330 Subject: [PATCH] WW-5041 Upgrade to OGNL 3.1.26 and adapt to its new features --- .../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 071ed71f9..8bd8b5631 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -184,6 +184,19 @@ public class OgnlUtil { this.disallowProxyMemberAccess = Boolean.parseBoolean(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; } @@ -755,6 +768,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 6bfdb31c9..bb7b4cb14 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -204,6 +204,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) { @@ -326,7 +329,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 f7863050c..371540bca 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -261,6 +261,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 952575503..d792d6d9d 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 Ognl.getValue(ognlUtil.compile(expr), 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 9cd2c6ba0..23f159ef4 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -219,4 +219,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 e85bc6aa0..f8f8bd0f8 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 72a84e9dd..c601a9733 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -30,6 +30,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.*; @@ -40,11 +41,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; /** @@ -347,6 +350,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 bb2aef47d..d55c19de5 100644 --- a/pom.xml +++ b/pom.xml @@ -98,7 +98,7 @@ UTF-8 4.3.24.RELEASE - 3.1.23 + 3.1.26 7.1 3.0.8 1.0.7