You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by mc...@apache.org on 2012/01/21 01:04:44 UTC
svn commit: r1234212 - in /struts/struts2/trunk: ./
xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/
xwork-core/src/main/java/com/opensymphony/xwork2/ognl/
xwork-core/src/main/java/com/opensymphony/xwork2/util/
xwork-core/src/test/java/com...
Author: mcucchiara
Date: Sat Jan 21 00:04:43 2012
New Revision: 1234212
URL: http://svn.apache.org/viewvc?rev=1234212&view=rev
Log:
Security issue fixed (see [1] for further details)
[1] https://cwiki.apache.org/confluence/display/WW/S2-009
Modified:
struts/struts2/trunk/pom.xml
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/ValueStack.java
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/StubValueStack.java
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
Modified: struts/struts2/trunk/pom.xml
URL: http://svn.apache.org/viewvc/struts/struts2/trunk/pom.xml?rev=1234212&r1=1234211&r2=1234212&view=diff
==============================================================================
--- struts/struts2/trunk/pom.xml (original)
+++ struts/struts2/trunk/pom.xml Sat Jan 21 00:04:43 2012
@@ -85,7 +85,7 @@
<properties>
<currentVersion>${project.version}</currentVersion>
<struts2.springPlatformVersion>3.0.5.RELEASE</struts2.springPlatformVersion>
- <ognl.version>3.0.3</ognl.version>
+ <ognl.version>3.0.4</ognl.version>
<asm.version>3.3</asm.version>
<tiles.version>2.0.6</tiles.version>
</properties>
Modified: struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
URL: http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java?rev=1234212&r1=1234211&r2=1234212&view=diff
==============================================================================
--- struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java (original)
+++ struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java Sat Jan 21 00:04:43 2012
@@ -135,7 +135,7 @@ public class ParametersInterceptor exten
static boolean devMode = false;
// Allowed names of parameters
- private String acceptedParamNames = "[a-zA-Z0-9\\.\\]\\[\\(\\)_']+";
+ private String acceptedParamNames = "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['\\w+'\\])|(\\('\\w+'\\)))*";
private Pattern acceptedPattern = Pattern.compile(acceptedParamNames);
private ValueStackFactory valueStackFactory;
@@ -289,7 +289,7 @@ public class ParametersInterceptor exten
String name = entry.getKey();
Object value = entry.getValue();
try {
- newStack.setValue(name, value);
+ newStack.setParameter(name, value);
} catch (RuntimeException e) {
if (devMode) {
String developerNotification = LocalizedTextUtil.findText(ParametersInterceptor.class, "devmode.notification", ActionContext.getContext().getLocale(), "Developer Notification:\n{0}", new Object[]{
Modified: struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
URL: http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java?rev=1234212&r1=1234211&r2=1234212&view=diff
==============================================================================
--- struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java (original)
+++ struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java Sat Jan 21 00:04:43 2012
@@ -206,7 +206,23 @@ public class OgnlUtil {
* Ideally, this should be handled by OGNL directly.
*/
public void setValue(String name, Map<String, Object> context, Object root, Object value) throws OgnlException {
- Ognl.setValue(compile(name), context, root, value);
+ setValue(name, context, root, value, true);
+ }
+
+ protected void setValue(String name, Map<String, Object> context, Object root, Object value, boolean evalName) throws OgnlException {
+ Object tree = compile(name);
+ if (!evalName && isEvalExpression(tree, context)) {
+ throw new OgnlException("Eval expression cannot be used as parameter name");
+ }
+ Ognl.setValue(tree, context, root, value);
+ }
+
+ private boolean isEvalExpression(Object tree, Map<String, Object> context) throws OgnlException {
+ if (tree instanceof SimpleNode) {
+ SimpleNode node = (SimpleNode) tree;
+ return node.isEvalChain((OgnlContext) context);
+ }
+ return false;
}
public Object getValue(String name, Map<String, Object> context, Object root) throws OgnlException {
@@ -245,7 +261,7 @@ public class OgnlUtil {
public void copy(Object from, Object to, Map<String, Object> context, Collection<String> exclusions, Collection<String> inclusions) {
if (from == null || to == null) {
if (LOG.isWarnEnabled()) {
- LOG.warn("Attempting to copy from or to a null source. This is illegal and is bein skipped. This may be due to an error in an OGNL expression, action chaining, or some other event.");
+ LOG.warn("Attempting to copy from or to a null source. This is illegal and is bein skipped. This may be due to an error in an OGNL expression, action chaining, or some other event.");
}
return;
@@ -284,7 +300,7 @@ public class OgnlUtil {
copy = false;
}
- if (copy == true) {
+ if (copy) {
PropertyDescriptor toPd = toPdHash.get(fromPd.getName());
if ((toPd != null) && (toPd.getWriteMethod() != null)) {
try {
Modified: struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
URL: http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java?rev=1234212&r1=1234211&r2=1234212&view=diff
==============================================================================
--- struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java (original)
+++ struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java Sat Jan 21 00:04:43 2012
@@ -145,6 +145,15 @@ public class OgnlValueStack implements S
}
/**
+ * @see com.opensymphony.xwork2.util.ValueStack#setParameter(String, Object)
+ */
+ public void setParameter(String expr, Object value) {
+ setValue(expr, value, devMode, false);
+ }
+
+ /**
+
+ /**
* @see com.opensymphony.xwork2.util.ValueStack#setValue(java.lang.String, java.lang.Object)
*/
public void setValue(String expr, Object value) {
@@ -155,9 +164,13 @@ public class OgnlValueStack implements S
* @see com.opensymphony.xwork2.util.ValueStack#setValue(java.lang.String, java.lang.Object, boolean)
*/
public void setValue(String expr, Object value, boolean throwExceptionOnFailure) {
+ setValue(expr, value, throwExceptionOnFailure, true);
+ }
+
+ private void setValue(String expr, Object value, boolean throwExceptionOnFailure, boolean evalExpression) {
Map<String, Object> context = getContext();
try {
- trySetValue(expr, value, throwExceptionOnFailure, context);
+ trySetValue(expr, value, throwExceptionOnFailure, context, evalExpression);
} catch (OgnlException e) {
handleOgnlException(expr, value, throwExceptionOnFailure, e);
} catch (RuntimeException re) { //XW-281
@@ -167,10 +180,10 @@ public class OgnlValueStack implements S
}
}
- private void trySetValue(String expr, Object value, boolean throwExceptionOnFailure, Map<String, Object> context) throws OgnlException {
+ private void trySetValue(String expr, Object value, boolean throwExceptionOnFailure, Map<String, Object> context, boolean evalExpression) throws OgnlException {
context.put(XWorkConverter.CONVERSION_PROPERTY_FULLNAME, expr);
context.put(REPORT_ERRORS_ON_NO_PROP, (throwExceptionOnFailure) ? Boolean.TRUE : Boolean.FALSE);
- ognlUtil.setValue(expr, context, root, value);
+ ognlUtil.setValue(expr, context, root, value, evalExpression);
}
private void cleanUpContext(Map<String, Object> context) {
Modified: struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/ValueStack.java
URL: http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/ValueStack.java?rev=1234212&r1=1234211&r2=1234212&view=diff
==============================================================================
--- struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/ValueStack.java (original)
+++ struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/ValueStack.java Sat Jan 21 00:04:43 2012
@@ -76,6 +76,14 @@ public interface ValueStack {
/**
* Attempts to set a property on a bean in the stack with the given expression using the default search order.
+ * N.B.: unlike #setValue(String,Object) it doesn't allow eval expression.
+ * @param expr the expression defining the path to the property to be set.
+ * @param value the value to be set into the named property
+ */
+ void setParameter(String expr, Object value);
+
+ /**
+ * Attempts to set a property on a bean in the stack with the given expression using the default search order.
*
* @param expr the expression defining the path to the property to be set.
* @param value the value to be set into the named property
Modified: struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/StubValueStack.java
URL: http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/StubValueStack.java?rev=1234212&r1=1234211&r2=1234212&view=diff
==============================================================================
--- struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/StubValueStack.java (original)
+++ struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/StubValueStack.java Sat Jan 21 00:04:43 2012
@@ -50,6 +50,10 @@ public class StubValueStack implements V
ctx.put(expr, value);
}
+ public void setParameter(String expr, Object value) {
+ throw new UnsupportedOperationException("not implemented");
+ }
+
public void setValue(String expr, Object value, boolean throwExceptionOnFailure) {
ctx.put(expr, value);
}
Modified: struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
URL: http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java?rev=1234212&r1=1234211&r2=1234212&view=diff
==============================================================================
--- struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java (original)
+++ struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java Sat Jan 21 00:04:43 2012
@@ -34,8 +34,10 @@ import com.opensymphony.xwork2.ognl.acce
import com.opensymphony.xwork2.util.CompoundRoot;
import com.opensymphony.xwork2.util.ValueStack;
import com.opensymphony.xwork2.util.ValueStackFactory;
+import junit.framework.Assert;
import ognl.PropertyAccessor;
+import java.io.File;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
@@ -295,6 +297,32 @@ public class ParametersInterceptorTest e
assertNull(action.getTheProtectedMap().get("foo"));
}
+ /**
+ * This test demonstrates a vulnerability which allows to execute arbitrary code.
+ * For further details and explanations see https://cwiki.apache.org/confluence/display/WW/S2-009
+ * @throws Exception
+ */
+ public void testEvalExpressionAsParameterName() throws Exception {
+ Map<String, Object> params = new HashMap<String, Object>();
+ params.put("blah", "(#context[\"xwork.MethodAccessor.denyMethodExecution\"]= new " +
+ "java.lang.Boolean(false), #_memberAccess[\"allowStaticMethodAccess\"]= new java.lang.Boolean(true), " +
+ "@java.lang.Runtime@getRuntime().exec('mkdir /tmp/PWNAGE'))(meh)");
+ params.put("top['blah'](0)", "true");
+
+ HashMap<String, Object> extraContext = new HashMap<String, Object>();
+ extraContext.put(ActionContext.PARAMETERS, params);
+
+ ActionProxy proxy = actionProxyFactory.createActionProxy("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME, extraContext);
+ proxy.execute();
+ @SuppressWarnings("unused")
+ SimpleAction action = (SimpleAction) proxy.getAction();
+ File pwn = new File("/tmp/PWNAGE");
+ boolean dirExists = pwn.exists();
+ @SuppressWarnings("unused")
+ boolean deleted = pwn.delete();
+ Assert.assertFalse("Remote exploit: The PWN folder has been created", dirExists);
+ }
+
public void testParametersOverwriteField() throws Exception {
Map<String, Object> params = new LinkedHashMap<String, Object>();
params.put("existingMap.boo", "This is blah");
@@ -509,6 +537,10 @@ public class ParametersInterceptorTest e
public void setValue(String expr, Object value) {
actual.put(expr, value);
}
+ @Override
+ public void setParameter(String expr, Object value) {
+ actual.put(expr, value);
+ }
};
container.inject(stack);
return stack;