CVE-2012-0392

@@ -80,7 +80,7 @@ protected Object getOverrideExpr(ActionInvocation invocation, Object value) {
         try {
             stack.push(value);
 
-            return "'" + stack.findValue("top", String.class) + "'";
+            return escape(stack.findString("top"));
         } finally {
             stack.pop();
         }

@@ -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/>
@@ -50,4 +50,4 @@
 <#include "/${parameters.templateDir}/simple/scripting-events.ftl" />
 <#include "/${parameters.templateDir}/simple/common-attributes.ftl" />
 <#include "/${parameters.templateDir}/simple/dynamic-attributes.ftl" />
-/>
\ No newline at end of file
+/>

@@ -21,19 +21,17 @@
 
 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;
 import com.opensymphony.xwork2.ActionContext;
 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 StrutsConversionErrorInterceptorTest extends StrutsTestCase {
 
     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 void testEmptyValuesDoNotSetFieldErrors() throws Exception {
     }
 
     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 @@ protected void setUp() throws Exception {
         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);

@@ -20,6 +20,7 @@
 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 extends AbstractInterceptor {
     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

@@ -22,58 +22,59 @@
 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 -->
- * 
+ *
  * &lt;!-- myJspPage.jsp --&gt;
  * &lt;ww:form action="someAction" method="POST"&gt;
  *   ....
- *   &lt;ww:textfield 
+ *   &lt;ww:textfield
  *       label="My Integer Field"
  *       name="myIntegerField" /&gt;
  *   ....
- *   &lt;ww:submit /&gt;       
+ *   &lt;ww:submit /&gt;
  * &lt;/ww:form&gt;
- * 
+ *
  * <!-- END SNIPPET: exampleJspPage -->
  * </pre>
- * 
+ *
  * <pre>
  * <!-- START SNIPPET: exampleXwork -->
- * 
+ *
  * &lt;!-- xwork.xml --&gt;
  * &lt;xwork&gt;
  * &lt;include file="xwork-default.xml" /&gt;
@@ -88,31 +89,31 @@
  * &lt;/package&gt;
  * ....
  * &lt;/xwork&gt;
- * 
+ *
  * <!-- END SNIPPET:exampleXwork -->
  * </pre>
- * 
- * 
+ *
+ *
  * <pre>
  * <!-- START SNIPPET: exampleJava -->
- * 
+ *
  * &lt;!-- MyActionSupport.java --&gt;
  * 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 -->
- * 
+ *
  * &lt;!-- MyActionSupport-someAction-validation.xml --&gt;
  * &lt;validators&gt;
  *   ...
@@ -124,24 +125,24 @@
  *   &lt;/field&gt;
  *   ...
  * &lt;/validators&gt;
- * 
+ *
  * <!-- 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() { 
+
+	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);
@@ -153,12 +154,12 @@ public void validate(Object object) throws ValidationException {
 			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)) {
@@ -170,19 +171,23 @@ public void repopulateField(Object object) throws ValidationException {
             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 {
-                    LOG.warn("value is an empty array of String or with first element in it as null [" + value + "], will not repopulate conversion error ");
+                    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 ");
+                    }
                 }
             } 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 
-                LOG.warn("conversion error value is not a String or array of String but instead is [" + value + "], will not repopulate conversion error");
+                if (LOG.isWarnEnabled()) {
+                    LOG.warn("conversion error value is not a String or array of String but instead is [" + value + "], will not repopulate conversion error");
+                }
             }
 
             if (doExprOverride) {
@@ -194,7 +199,11 @@ public void beforeResult(ActionInvocation invocation, String resultCode) {
                 });
             }
         }
-	}
-	
-	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;
+}
\ No newline at end of file

@@ -35,13 +35,13 @@ public class ConversionErrorInterceptorTest extends XWorkTestCase {
     protected ActionContext context;
     protected ActionInvocation invocation;
     protected ConversionErrorInterceptor interceptor;
-    protected Map conversionErrors;
+    protected Map<String, Object> conversionErrors;
     protected Mock mockInvocation;
     protected ValueStack stack;
 
 
     public void testFieldErrorAdded() throws Exception {
-        conversionErrors.put("foo", new Long(123));
+        conversionErrors.put("foo", 123L);
 
         SimpleAction action = new SimpleAction();
         mockInvocation.expectAndReturn("getAction", action);
@@ -69,19 +69,12 @@ public void testFieldErrorWithMapKeyAdded() throws Exception {
     public void testWithPreResultListener() throws Exception {
         conversionErrors.put("foo", "Hello");
 
-        ActionContext ac = new ActionContext(stack.getContext());
-        ac.setConversionErrors(conversionErrors);
-        ac.setValueStack(stack);
+        ActionContext ac = createActionContext();
+        MockActionInvocation mai = createActionInvocation(ac);
+        SimpleAction action = createAction(mai);
 
-        MockActionInvocation mai = new MockActionInvocation();
-        mai.setInvocationContext(ac);
-        mai.setStack(stack);
-        SimpleAction action = new SimpleAction();
-        action.setFoo(55);
-        mai.setAction(action);
-        stack.push(action);
         assertNull(action.getFieldErrors().get("foo"));
-        assertEquals(new Integer(55), stack.findValue("foo"));
+        assertEquals(55, stack.findValue("foo"));
 
         interceptor.intercept(mai);
 
@@ -91,6 +84,52 @@ public void testWithPreResultListener() throws Exception {
         assertEquals("Hello", stack.findValue("foo")); // assume that the original value is reset
     }
 
+    /**
+     * See WW-3668
+     *
+     * @throws Exception
+     */
+    public void testWithPreResultListenerAgainstMaliciousCode() throws Exception {
+        conversionErrors.put("foo", "\" + #root + \"");
+
+        ActionContext ac = createActionContext();
+
+        MockActionInvocation mai = createActionInvocation(ac);
+
+        SimpleAction action = createAction(mai);
+        assertNull(action.getFieldErrors().get("foo"));
+        assertEquals(55, stack.findValue("foo"));
+
+        interceptor.intercept(mai);
+
+        assertTrue(action.hasFieldErrors());
+        assertNotNull(action.getFieldErrors().get("foo"));
+
+        assertEquals("\" + #root + \"", stack.findValue("foo"));
+    }
+
+    private MockActionInvocation createActionInvocation(ActionContext ac) {
+        MockActionInvocation mai = new MockActionInvocation();
+        mai.setInvocationContext(ac);
+        mai.setStack(stack);
+        return mai;
+    }
+
+    private SimpleAction createAction(MockActionInvocation mai) {
+        SimpleAction action = new SimpleAction();
+        action.setFoo(55);
+        mai.setAction(action);
+        stack.push(action);
+        return action;
+    }
+
+    private ActionContext createActionContext() {
+        ActionContext ac = new ActionContext(stack.getContext());
+        ac.setConversionErrors(conversionErrors);
+        ac.setValueStack(stack);
+        return ac;
+    }
+
     @Override
     protected void setUp() throws Exception {
         super.setUp();
@@ -99,7 +138,7 @@ protected void setUp() throws Exception {
         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.expect("addPreResultListener", C.isA(PreResultListener.class));