Author: mcucchiara
Date: Fri Aug 12 08:38:39 2011
New Revision: 1157009
URL: http://svn.apache.org/viewvc?rev=1157009&view=rev
Log:
WW-3668 - Vulnerability: User input is evaluated as an OGNL expression when
there's a conversion error
Modified:
struts/struts2/trunk/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
struts/struts2/trunk/core/src/main/resources/template/simple/text.ftl
struts/struts2/trunk/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
Modified:
struts/struts2/trunk/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java?rev=1157009&r1=1157008&r2=1157009&view=diff
==============================================================================
---
struts/struts2/trunk/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
(original)
+++
struts/struts2/trunk/core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
Fri Aug 12 08:38:39 2011
@@ -80,7 +80,7 @@ public class StrutsConversionErrorInterc
try {
stack.push(value);
- return "'" + stack.findValue("top", String.class) + "'";
+ return escape(stack.findString("top"));
} finally {
stack.pop();
}
Modified: struts/struts2/trunk/core/src/main/resources/template/simple/text.ftl
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/core/src/main/resources/template/simple/text.ftl?rev=1157009&r1=1157008&r2=1157009&view=diff
==============================================================================
--- struts/struts2/trunk/core/src/main/resources/template/simple/text.ftl
(original)
+++ struts/struts2/trunk/core/src/main/resources/template/simple/text.ftl Fri
Aug 12 08:38:39 2011
@@ -29,7 +29,7 @@
maxlength="${parameters.maxlength?html}"<#rt/>
</#if>
<#if parameters.nameValue??>
- value="<@s.property value="parameters.nameValue"/>"<#rt/>
+ value="${parameters.nameValue?html}"<#rt/>
</#if>
<#if parameters.disabled?default(false)>
disabled="disabled"<#rt/>
Modified:
struts/struts2/trunk/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java?rev=1157009&r1=1157008&r2=1157009&view=diff
==============================================================================
---
struts/struts2/trunk/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
(original)
+++
struts/struts2/trunk/core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
Fri Aug 12 08:38:39 2011
@@ -21,11 +21,6 @@
package org.apache.struts2.interceptor;
-import java.util.HashMap;
-import java.util.Map;
-
-import org.apache.struts2.StrutsTestCase;
-
import com.mockobjects.dynamic.C;
import com.mockobjects.dynamic.Mock;
import com.opensymphony.xwork2.Action;
@@ -33,7 +28,10 @@ import com.opensymphony.xwork2.ActionCon
import com.opensymphony.xwork2.ActionInvocation;
import com.opensymphony.xwork2.ActionSupport;
import com.opensymphony.xwork2.util.ValueStack;
-import com.opensymphony.xwork2.util.ValueStackFactory;
+import org.apache.struts2.StrutsTestCase;
+
+import java.util.HashMap;
+import java.util.Map;
/**
@@ -44,14 +42,14 @@ public class StrutsConversionErrorInterc
protected ActionContext context;
protected ActionInvocation invocation;
- protected Map conversionErrors;
+ protected Map<String, Object> conversionErrors;
protected Mock mockInvocation;
protected ValueStack stack;
protected StrutsConversionErrorInterceptor interceptor;
public void testEmptyValuesDoNotSetFieldErrors() throws Exception {
- conversionErrors.put("foo", new Long(123));
+ conversionErrors.put("foo", 123L);
conversionErrors.put("bar", "");
conversionErrors.put("baz", new String[]{""});
@@ -70,7 +68,7 @@ public class StrutsConversionErrorInterc
}
public void testFieldErrorAdded() throws Exception {
- conversionErrors.put("foo", new Long(123));
+ conversionErrors.put("foo", 123L);
ActionSupport action = new ActionSupport();
mockInvocation.expectAndReturn("getAction", action);
@@ -89,7 +87,7 @@ public class StrutsConversionErrorInterc
invocation = (ActionInvocation) mockInvocation.proxy();
stack = ActionContext.getContext().getValueStack();
context = new ActionContext(stack.getContext());
- conversionErrors = new HashMap();
+ conversionErrors = new HashMap<String, Object>();
context.setConversionErrors(conversionErrors);
mockInvocation.matchAndReturn("getInvocationContext", context);
mockInvocation.expectAndReturn("invoke", Action.SUCCESS);
Modified:
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java?rev=1157009&r1=1157008&r2=1157009&view=diff
==============================================================================
---
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
(original)
+++
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
Fri Aug 12 08:38:39 2011
@@ -20,6 +20,7 @@ import com.opensymphony.xwork2.ActionInv
import com.opensymphony.xwork2.ValidationAware;
import com.opensymphony.xwork2.conversion.impl.XWorkConverter;
import com.opensymphony.xwork2.util.ValueStack;
+import org.apache.commons.lang.StringEscapeUtils;
import java.util.HashMap;
import java.util.Map;
@@ -84,7 +85,11 @@ public class ConversionErrorInterceptor
public static final String ORIGINAL_PROPERTY_OVERRIDE =
"original.property.override";
protected Object getOverrideExpr(ActionInvocation invocation, Object
value) {
- return "'" + value + "'";
+ return escape(value);
+ }
+
+ protected String escape(Object value) {
+ return "\"" + StringEscapeUtils.escapeJava(String.valueOf(value)) +
"\"";
}
@Override
Modified:
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java?rev=1157009&r1=1157008&r2=1157009&view=diff
==============================================================================
---
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
(original)
+++
struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
Fri Aug 12 08:38:39 2011
@@ -22,58 +22,59 @@ import com.opensymphony.xwork2.util.Valu
import com.opensymphony.xwork2.util.logging.Logger;
import com.opensymphony.xwork2.util.logging.LoggerFactory;
import com.opensymphony.xwork2.validator.ValidationException;
+import org.apache.commons.lang.StringEscapeUtils;
import java.util.LinkedHashMap;
import java.util.Map;
/**
- *
- *
+ *
+ *
* An abstract base class that adds in the capability to populate the stack
with
* a fake parameter map when a conversion error has occurred and the
'repopulateField'
* property is set to "true".
- *
+ *
* <p/>
- *
+ *
*
* <!-- START SNIPPET: javadoc -->
*
- * The capability of auto-repopulating the stack with a fake parameter map
when
- * a conversion error has occurred can be done with 'repopulateField' property
- * set to "true".
+ * The capability of auto-repopulating the stack with a fake parameter map when
+ * a conversion error has occurred can be done with 'repopulateField' property
+ * set to "true".
*
* <p/>
*
- * This is typically usefull when one wants to repopulate the field with the
original value
- * when a conversion error occurred. Eg. with a textfield that only allows an
Integer
+ * This is typically usefull when one wants to repopulate the field with the
original value
+ * when a conversion error occurred. Eg. with a textfield that only allows an
Integer
* (the action class have an Integer field declared), upon conversion error,
the incorrectly
* entered integer (maybe a text 'one') will not appear when dispatched back.
With 'repopulateField'
- * porperty set to true, it will, meaning the textfield will have 'one' as its
value
+ * porperty set to true, it will, meaning the textfield will have 'one' as its
value
* upon conversion error.
- *
+ *
* <!-- END SNIPPET: javadoc -->
- *
+ *
* <p/>
- *
+ *
* <pre>
* <!-- START SNIPPET: exampleJspPage -->
- *
+ *
* <!-- myJspPage.jsp -->
* <ww:form action="someAction" method="POST">
* ....
- * <ww:textfield
+ * <ww:textfield
* label="My Integer Field"
* name="myIntegerField" />
* ....
- * <ww:submit />
+ * <ww:submit />
* </ww:form>
- *
+ *
* <!-- END SNIPPET: exampleJspPage -->
* </pre>
- *
+ *
* <pre>
* <!-- START SNIPPET: exampleXwork -->
- *
+ *
* <!-- xwork.xml -->
* <xwork>
* <include file="xwork-default.xml" />
@@ -88,31 +89,31 @@ import java.util.Map;
* </package>
* ....
* </xwork>
- *
+ *
* <!-- END SNIPPET:exampleXwork -->
* </pre>
- *
- *
+ *
+ *
* <pre>
* <!-- START SNIPPET: exampleJava -->
- *
+ *
* <!-- MyActionSupport.java -->
* public class MyActionSupport extends ActionSupport {
* private Integer myIntegerField;
- *
+ *
* public Integer getMyIntegerField() { return this.myIntegerField; }
- * public void setMyIntegerField(Integer myIntegerField) {
- * this.myIntegerField = myIntegerField;
+ * public void setMyIntegerField(Integer myIntegerField) {
+ * this.myIntegerField = myIntegerField;
* }
* }
- *
+ *
* <!-- END SNIPPET: exampleJava -->
* </pre>
- *
- *
+ *
+ *
* <pre>
* <!-- START SNIPPET: exampleValidation -->
- *
+ *
* <!-- MyActionSupport-someAction-validation.xml -->
* <validators>
* ...
@@ -124,43 +125,43 @@ import java.util.Map;
* </field>
* ...
* </validators>
- *
+ *
* <!-- END SNIPPET: exampleValidation -->
* </pre>
- *
+ *
* @author tm_jee
* @version $Date$ $Id$
*/
public abstract class RepopulateConversionErrorFieldValidatorSupport extends
FieldValidatorSupport {
-
- private static final Logger LOG =
LoggerFactory.getLogger(RepopulateConversionErrorFieldValidatorSupport.class);
-
- private String repopulateFieldAsString = "false";
- private boolean repopulateFieldAsBoolean = false;
-
- public String getRepopulateField() {
- return repopulateFieldAsString;
- }
-
- public void setRepopulateField(String repopulateField) {
- this.repopulateFieldAsString = repopulateField == null ?
repopulateField : repopulateField.trim();
- this.repopulateFieldAsBoolean =
"true".equalsIgnoreCase(this.repopulateFieldAsString) ? (true) : (false);
- }
-
- public void validate(Object object) throws ValidationException {
- doValidate(object);
- if (repopulateFieldAsBoolean) {
- repopulateField(object);
- }
- }
-
- public void repopulateField(Object object) throws ValidationException {
-
- ActionInvocation invocation =
ActionContext.getContext().getActionInvocation();
- Map<String, Object> conversionErrors =
ActionContext.getContext().getConversionErrors();
-
- String fieldName = getFieldName();
- String fullFieldName =
getValidatorContext().getFullFieldName(fieldName);
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RepopulateConversionErrorFieldValidatorSupport.class);
+
+ private String repopulateFieldAsString = "false";
+ private boolean repopulateFieldAsBoolean = false;
+
+ public String getRepopulateField() {
+ return repopulateFieldAsString;
+ }
+
+ public void setRepopulateField(String repopulateField) {
+ this.repopulateFieldAsString = repopulateField == null ?
repopulateField : repopulateField.trim();
+ this.repopulateFieldAsBoolean =
"true".equalsIgnoreCase(this.repopulateFieldAsString) ? (true) : (false);
+ }
+
+ public void validate(Object object) throws ValidationException {
+ doValidate(object);
+ if (repopulateFieldAsBoolean) {
+ repopulateField(object);
+ }
+ }
+
+ public void repopulateField(Object object) throws ValidationException {
+
+ ActionInvocation invocation =
ActionContext.getContext().getActionInvocation();
+ Map<String, Object> conversionErrors =
ActionContext.getContext().getConversionErrors();
+
+ String fieldName = getFieldName();
+ String fullFieldName =
getValidatorContext().getFullFieldName(fieldName);
if (conversionErrors.containsKey(fullFieldName)) {
Object value = conversionErrors.get(fullFieldName);
@@ -170,18 +171,18 @@ public abstract class RepopulateConversi
if (value instanceof String[]) {
// take the first element, if possible
String[] tmpValue = (String[]) value;
- if (tmpValue != null && (tmpValue.length > 0)) {
+ if ((tmpValue.length > 0)) {
doExprOverride = true;
- fakeParams.put(fullFieldName, "'" + tmpValue[0] + "'");
+ fakeParams.put(fullFieldName, escape(tmpValue[0]));
} else {
if (LOG.isWarnEnabled()) {
- LOG.warn("value is an empty array of String or with
first element in it as null [" + value + "], will not repopulate conversion
error ");
+ LOG.warn("value is an empty array of String or with
first element in it as null [" + value + "], will not repopulate conversion
error ");
}
}
} else if (value instanceof String) {
String tmpValue = (String) value;
doExprOverride = true;
- fakeParams.put(fullFieldName, "'" + tmpValue + "'");
+ fakeParams.put(fullFieldName, escape(tmpValue));
} else {
// opps... it should be
if (LOG.isWarnEnabled()) {
@@ -198,7 +199,11 @@ public abstract class RepopulateConversi
});
}
}
- }
-
- protected abstract void doValidate(Object object) throws
ValidationException;
+ }
+
+ protected String escape(String value) {
+ return "\"" + StringEscapeUtils.escapeJava(value) + "\"";
+ }
+
+ protected abstract void doValidate(Object object) throws
ValidationException;
}
Modified:
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
URL:
http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java?rev=1157009&r1=1157008&r2=1157009&view=diff
==============================================================================
---
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
(original)
+++
struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
Fri Aug 12 08:38:39 2011
@@ -35,7 +35,7 @@ public class ConversionErrorInterceptorT
protected ActionContext context;
protected ActionInvocation invocation;
protected ConversionErrorInterceptor interceptor;
- protected Map<String,Object> conversionErrors;
+ protected Map<String, Object> conversionErrors;
protected Mock mockInvocation;
protected ValueStack stack;
@@ -86,10 +86,11 @@ public class ConversionErrorInterceptorT
/**
* See WW-3668
+ *
* @throws Exception
*/
public void testWithPreResultListenerAgainstMaliciousCode() throws
Exception {
- conversionErrors.put("foo", "' + #root + '");
+ conversionErrors.put("foo", "\" + #root + \"");
ActionContext ac = createActionContext();
@@ -104,7 +105,7 @@ public class ConversionErrorInterceptorT
assertTrue(action.hasFieldErrors());
assertNotNull(action.getFieldErrors().get("foo"));
- assertEquals("' + #root + '", stack.findValue("foo"));
+ assertEquals("\" + #root + \"", stack.findValue("foo"));
}
private MockActionInvocation createActionInvocation(ActionContext ac) {
@@ -137,7 +138,7 @@ public class ConversionErrorInterceptorT
invocation = (ActionInvocation) mockInvocation.proxy();
stack = ActionContext.getContext().getValueStack();
context = new ActionContext(stack.getContext());
- conversionErrors = new HashMap<String,Object>();
+ conversionErrors = new HashMap<String, Object>();
context.setConversionErrors(conversionErrors);
mockInvocation.matchAndReturn("getInvocationContext", context);
mockInvocation.expect("addPreResultListener",
C.isA(PreResultListener.class));
