Skip to content

Commit

Permalink
WW-3668 - Vulnerability: User input is evaluated as an OGNL expressio…
Browse files Browse the repository at this point in the history 
…n when there's a conversion error

git-svn-id: https://svn.apache.org/repos/asf/struts/struts2/trunk@1157009 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@mcucchiara
mcucchiara committed Aug 12, 2011
1 parent 6137ac2 commit b4265d3
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 85 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/resources/template/simple/text.ftl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/>
Expand Down
18 changes: 8 additions & 10 deletions core/src/test/java/org/apache/struts2/interceptor/StrutsConversionErrorInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;


/**
Expand All @@ -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[]{""});

Expand All @@ -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);
Expand All @@ -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);
Expand Down
7 changes: 6 additions & 1 deletion xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
141 changes: 73 additions & 68 deletions ...nsymphony/xwork2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
* ...
Expand All @@ -124,43 +125,43 @@
* &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() {
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);

Expand All @@ -170,18 +171,18 @@ 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 {
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()) {
Expand All @@ -198,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;
}
9 changes: 5 additions & 4 deletions ...ore/src/test/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public class ConversionErrorInterceptorTest extends XWorkTestCase {
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;

Expand Down Expand Up @@ -86,10 +86,11 @@ public void testWithPreResultListener() throws Exception {

/**
* See WW-3668
*
* @throws Exception
*/
public void testWithPreResultListenerAgainstMaliciousCode() throws Exception {
conversionErrors.put("foo", "' + #root + '");
conversionErrors.put("foo", "\" + #root + \"");

ActionContext ac = createActionContext();

Expand All @@ -104,7 +105,7 @@ public void testWithPreResultListenerAgainstMaliciousCode() throws Exception {
assertTrue(action.hasFieldErrors());
assertNotNull(action.getFieldErrors().get("foo"));

assertEquals("' + #root + '", stack.findValue("foo"));
assertEquals("\" + #root + \"", stack.findValue("foo"));
}

private MockActionInvocation createActionInvocation(ActionContext ac) {
Expand Down Expand Up @@ -137,7 +138,7 @@ protected void setUp() throws Exception {
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));
Expand Down

0 comments on commit b4265d3

Please sign in to comment.