Skip to content

Commit

Permalink
WW-4146 Caches only valid Ognl expressions to avoid cache attack
Browse files Browse the repository at this point in the history
  • Loading branch information
@lukaszlenart
lukaszlenart committed Mar 27, 2014
1 parent 5fb27ba commit 86813c1
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 36 deletions.
85 changes: 61 additions & 24 deletions xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,12 +226,16 @@ public void setValue(String name, Map<String, Object> context, Object root, Obje
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, context);
if (!evalName && isEvalExpression(tree, context)) {
throw new OgnlException("Eval expression cannot be used as parameter name");
}
Ognl.setValue(tree, context, root, value);
protected void setValue(String name, final Map<String, Object> context, final Object root, final Object value, final boolean evalName) throws OgnlException {
compileAndExecute(name, context, new OgnlTask<Void>() {
public Void execute(Object tree) throws OgnlException {
if (!evalName && isEvalExpression(tree, context)) {
throw new OgnlException("Eval expression cannot be used as parameter name");
}
Ognl.setValue(tree, context, root, value);
return null;
}
});
}

private boolean isEvalExpression(Object tree, Map<String, Object> context) throws OgnlException {
Expand All @@ -247,20 +251,28 @@ private boolean isEvalExpression(Object tree, Map<String, Object> context) throw
return false;
}

public Object getValue(String name, Map<String, Object> context, Object root) throws OgnlException {
return Ognl.getValue(compile(name, context), context, root);
public Object getValue(final String name, final Map<String, Object> context, final Object root) throws OgnlException {
return compileAndExecute(name, context, new OgnlTask<Object>() {
public Object execute(Object tree) throws OgnlException {
return Ognl.getValue(tree, context, root);
}
});
}

public Object getValue(String name, Map<String, Object> context, Object root, Class resultType) throws OgnlException {
return Ognl.getValue(compile(name, context), context, root, resultType);
public Object getValue(final String name, final Map<String, Object> context, final Object root, final Class resultType) throws OgnlException {
return compileAndExecute(name, context, new OgnlTask<Object>() {
public Object execute(Object tree) throws OgnlException {
return Ognl.getValue(tree, context, root, resultType);
}
});
}


public Object compile(String expression) throws OgnlException {
return compile(expression, null);
}

public Object compile(String expression, Map<String, Object> context) throws OgnlException {
private <T> Object compileAndExecute(String expression, Map<String, Object> context, OgnlTask<T> task) throws OgnlException {
Object tree;
if (enableExpressionCache) {
tree = expressions.get(expression);
Expand All @@ -274,7 +286,19 @@ public Object compile(String expression, Map<String, Object> context) throws Ogn
checkEnableEvalExpression(tree, context);
}

return tree;

final T exec = task.execute(tree);
if(enableExpressionCache)
expressions.putIfAbsent(expression, tree);
return exec;
}

public Object compile(String expression, Map<String, Object> context) throws OgnlException {
return compileAndExecute(expression,context,new OgnlTask<Object>() {
public Object execute(Object tree) throws OgnlException {
return tree;
}
});
}

private void checkEnableEvalExpression(Object tree, Map<String, Object> context) throws OgnlException {
Expand All @@ -295,7 +319,7 @@ private void checkEnableEvalExpression(Object tree, Map<String, Object> context)
* @param inclusions collection of method names to included copying (can be null)
* note if exclusions AND inclusions are supplied and not null nothing will get copied.
*/
public void copy(Object from, Object to, Map<String, Object> context, Collection<String> exclusions, Collection<String> inclusions) {
public void copy(final Object from, final Object to, final 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.");
Expand All @@ -305,9 +329,9 @@ public void copy(Object from, Object to, Map<String, Object> context, Collection
}

TypeConverter conv = getTypeConverterFromContext(context);
Map contextFrom = Ognl.createDefaultContext(from);
final Map contextFrom = Ognl.createDefaultContext(from);
Ognl.setTypeConverter(contextFrom, conv);
Map contextTo = Ognl.createDefaultContext(to);
final Map contextTo = Ognl.createDefaultContext(to);
Ognl.setTypeConverter(contextTo, conv);

PropertyDescriptor[] fromPds;
Expand Down Expand Up @@ -342,9 +366,14 @@ public void copy(Object from, Object to, Map<String, Object> context, Collection
PropertyDescriptor toPd = toPdHash.get(fromPd.getName());
if ((toPd != null) && (toPd.getWriteMethod() != null)) {
try {
Object expr = compile(fromPd.getName(), context);
Object value = Ognl.getValue(expr, contextFrom, from);
Ognl.setValue(expr, contextTo, to, value);
compileAndExecute(fromPd.getName(), context, new OgnlTask<Object>() {
public Void execute(Object expr) throws OgnlException {
Object value = Ognl.getValue(expr, contextFrom, from);
Ognl.setValue(expr, contextTo, to, value);
return null;
}
});

} catch (OgnlException e) {
if (LOG.isDebugEnabled()) {
LOG.debug("Got OGNL exception", e);
Expand Down Expand Up @@ -409,16 +438,19 @@ public PropertyDescriptor[] getPropertyDescriptors(Class clazz) throws Introspec
* @throws IntrospectionException is thrown if an exception occurs during introspection.
* @throws OgnlException is thrown by OGNL if the property value could not be retrieved
*/
public Map getBeanMap(Object source) throws IntrospectionException, OgnlException {
Map beanMap = new HashMap();
Map sourceMap = Ognl.createDefaultContext(source);
public Map<String, Object> getBeanMap(final Object source) throws IntrospectionException, OgnlException {
Map<String, Object> beanMap = new HashMap<String, Object>();
final Map sourceMap = Ognl.createDefaultContext(source);
PropertyDescriptor[] propertyDescriptors = getPropertyDescriptors(source);
for (PropertyDescriptor propertyDescriptor : propertyDescriptors) {
String propertyName = propertyDescriptor.getDisplayName();
final String propertyName = propertyDescriptor.getDisplayName();
Method readMethod = propertyDescriptor.getReadMethod();
if (readMethod != null) {
Object expr = compile(propertyName);
Object value = Ognl.getValue(expr, sourceMap, source);
final Object value = compileAndExecute(propertyName, null, new OgnlTask<Object>() {
public Object execute(Object expr) throws OgnlException {
return Ognl.getValue(expr, sourceMap, source);
}
});
beanMap.put(propertyName, value);
} else {
beanMap.put(propertyName, "There is no read method for " + propertyName);
Expand Down Expand Up @@ -485,4 +517,9 @@ TypeConverter getTypeConverterFromContext(Map<String, Object> context) {
*/
return defaultConverter;
}

private interface OgnlTask<T> {
T execute(Object tree) throws OgnlException;
}

}
27 changes: 15 additions & 12 deletions xwork-core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@

import com.opensymphony.xwork2.XWorkConstants;
import com.opensymphony.xwork2.XWorkException;
import com.opensymphony.xwork2.ognl.OgnlValueStack;
import com.opensymphony.xwork2.inject.Inject;
import com.opensymphony.xwork2.ognl.OgnlValueStack;
import com.opensymphony.xwork2.util.CompoundRoot;
import com.opensymphony.xwork2.util.ValueStack;
import com.opensymphony.xwork2.util.logging.Logger;
Expand All @@ -29,6 +29,8 @@
import java.beans.PropertyDescriptor;
import java.util.*;

import static java.lang.String.format;
import static org.apache.commons.lang3.BooleanUtils.toBoolean;

/**
* A stack that is able to call methods on objects in the stack.
Expand All @@ -55,7 +57,7 @@ public String getSourceSetter(OgnlContext context, Object target, Object index)

private final static Logger LOG = LoggerFactory.getLogger(CompoundRootAccessor.class);
private final static Class[] EMPTY_CLASS_ARRAY = new Class[0];
private static Map invalidMethods = new HashMap();
private static Map<MethodCall, Boolean> invalidMethods = new HashMap<MethodCall, Boolean>();

static boolean devMode = false;

Expand All @@ -79,7 +81,8 @@ public void setProperty(Map context, Object target, Object name, Object value) t

return;
} else if (o instanceof Map) {
Map<Object, Object> map = (Map) o;
@SuppressWarnings("unchecked")
Map<Object, Object> map = (Map<Object, Object>) o;
try {
map.put(name, value);
return;
Expand All @@ -98,14 +101,14 @@ public void setProperty(Map context, Object target, Object name, Object value) t
}
}

Boolean reportError = (Boolean) context.get(ValueStack.REPORT_ERRORS_ON_NO_PROP);

final String msg = "No object in the CompoundRoot has a publicly accessible property named '" + name + "' (no setter could be found).";
boolean reportError = toBoolean((Boolean) context.get(ValueStack.REPORT_ERRORS_ON_NO_PROP));

if ((reportError != null) && (reportError.booleanValue())) {
throw new XWorkException(msg);
} else {
if (devMode) {
if (reportError || devMode) {
final String msg = format("No object in the CompoundRoot has a publicly accessible property named '%s' " +
"(no setter could be found).", name);
if (reportError) {
throw new XWorkException(msg);
} else {
LOG.warn(msg);
}
}
Expand All @@ -118,7 +121,7 @@ public Object getProperty(Map context, Object target, Object name) throws OgnlEx
if (name instanceof Integer) {
Integer index = (Integer) name;

return root.cutStack(index.intValue());
return root.cutStack(index);
} else if (name instanceof String) {
if ("top".equals(name)) {
if (root.size() > 0) {
Expand Down Expand Up @@ -234,7 +237,7 @@ public Object callMethod(Map context, Object target, String name, Object[] objec

if ((argTypes == null) || !invalidMethods.containsKey(mc)) {
try {
Object value = OgnlRuntime.callMethod((OgnlContext) context, o, name, name, objects);
Object value = OgnlRuntime.callMethod((OgnlContext) context, o, name, objects);

if (value != null) {
return value;
Expand Down

0 comments on commit 86813c1

Please sign in to comment.