diff --git a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.java b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.java new file mode 100644 index 00000000000..cc99ff46517 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.java @@ -0,0 +1,17 @@ +import ognl.Ognl; +import ognl.OgnlException; + +public void evaluate(HttpServletRequest request, Object root) throws OgnlException { + String expression = request.getParameter("expression"); + + // BAD: User provided expression is evaluated + Ognl.getValue(expression, root); + + // GOOD: The name is validated and expression is evaluated in sandbox + System.setProperty("ognl.security.manager", ""); // Or add -Dognl.security.manager to JVM args + if (isValid(expression)) { + Ognl.getValue(expression, root); + } else { + // Reject the request + } +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.qhelp b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.qhelp new file mode 100644 index 00000000000..e20d54f1d84 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.qhelp @@ -0,0 +1,33 @@ + + + +

Object-Graph Navigation Language (OGNL) is an open-source Expression Language (EL) for Java. Due +to its ability to create or change executable code, OGNL is capable of introducing critical +security flaws to any application that uses it. Evaluation of unvalidated expressions can let +attacker to modify Java objects' properties or execute arbitrary code.

+ + + +

The general recommendation is to not evaluate untrusted ONGL expressions. If user provided OGNL +expressions must be evaluated, do this in sandbox (add `-Dognl.security.manager` to JVM arguments) +and validate the expressions before evaluation.

+ + + +

In the following examples, the code accepts an OGNL expression from the user and evaluates it. +

+ +

In the first example, the user provided OGNL expression is parsed and evaluated.

+ +

The second example validates the expression and evaluates it inside the sandbox.

+ + + + + +
  • OGNL library.
    • +
  • Struts security: Proactively protect from OGNL Expression Injections attacks.
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.ql new file mode 100644 index 00000000000..e8a75591b98 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.ql @@ -0,0 +1,22 @@ +/** + * @name OGNL Expression Language statement with user-controlled input + * @description Evaluation of OGNL Expression Language statement with user-controlled input can + * lead to execution of arbitrary code. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/ognl-injection + * @tags security + * external/cwe/cwe-917 + */ + +import java +import semmle.code.java.dataflow.FlowSources +import DataFlow +import DataFlow::PathGraph +import OgnlInjectionLib + +from DataFlow::PathNode source, DataFlow::PathNode sink, OgnlInjectionFlowConfig conf +where conf.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "OGNL expression might include input from $@.", + source.getNode(), "this user input" diff --git a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll new file mode 100644 index 00000000000..569e18a29c3 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll @@ -0,0 +1,109 @@ +import java +import semmle.code.java.dataflow.FlowSources +import DataFlow +import DataFlow::PathGraph + +/** + * A taint-tracking configuration for unvalidated user input that is used in OGNL EL evaluation. + */ +class OgnlInjectionFlowConfig extends TaintTracking::Configuration { + OgnlInjectionFlowConfig() { this = "OgnlInjectionFlowConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof OgnlInjectionSink } + + override predicate isSanitizer(DataFlow::Node node) { + node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + parseCompileExpressionStep(node1, node2) + } +} + +/** The class `org.apache.commons.ognl.Ognl` or `ognl.Ognl`. */ +class TypeOgnl extends Class { + TypeOgnl() { + this.hasQualifiedName("org.apache.commons.ognl", "Ognl") or + this.hasQualifiedName("ognl", "Ognl") + } +} + +/** The interface `org.apache.commons.ognl.Node` or `ognl.Node`. */ +class TypeNode extends Interface { + TypeNode() { + this.hasQualifiedName("org.apache.commons.ognl", "Node") or + this.hasQualifiedName("ognl", "Node") + } +} + +/** The class `com.opensymphony.xwork2.ognl.OgnlUtil`. */ +class TypeOgnlUtil extends Class { + TypeOgnlUtil() { this.hasQualifiedName("com.opensymphony.xwork2.ognl", "OgnlUtil") } +} + +/** + * OGNL sink for OGNL injection vulnerabilities, i.e. 1st argument to `getValue` or `setValue` + * method from `Ognl` or `getValue` or `setValue` method from `Node`. + */ +predicate ognlSinkMethod(Method m, int index) { + ( + m.getDeclaringType() instanceof TypeOgnl + or + m.getDeclaringType().getAnAncestor*() instanceof TypeNode + ) and + ( + m.hasName("getValue") or + m.hasName("setValue") + ) and + index = 0 +} + +/** + * Struts sink for OGNL injection vulnerabilities, i.e. 1st argument to `getValue`, `setValue` or + * `callMethod` method from `OgnlUtil`. + */ +predicate strutsSinkMethod(Method m, int index) { + m.getDeclaringType() instanceof TypeOgnlUtil and + ( + m.hasName("getValue") or + m.hasName("setValue") or + m.hasName("callMethod") + ) and + index = 0 +} + +/** Holds if parameter at index `index` in method `m` is OGNL injection sink. */ +predicate ognlInjectionSinkMethod(Method m, int index) { + ognlSinkMethod(m, index) or + strutsSinkMethod(m, index) +} + +/** A data flow sink for unvalidated user input that is used in OGNL EL evaluation. */ +class OgnlInjectionSink extends DataFlow::ExprNode { + OgnlInjectionSink() { + exists(MethodAccess ma, Method m, int index | + ma.getMethod() = m and + (ma.getArgument(index) = this.getExpr() or ma.getQualifier() = this.getExpr()) and + ognlInjectionSinkMethod(m, index) + ) + } +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `Object` or `Node`, + * i.e. `Ognl.parseExpression(tainted)` or `Ognl.compileExpression(tainted)`. + */ +predicate parseCompileExpressionStep(ExprNode n1, ExprNode n2) { + exists(MethodAccess ma, Method m, int index | + n1.asExpr() = ma.getArgument(index) and + n2.asExpr() = ma and + ma.getMethod() = m and + m.getDeclaringType() instanceof TypeOgnl + | + m.hasName("parseExpression") and index = 0 + or + m.hasName("compileExpression") and index = 2 + ) +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.expected new file mode 100644 index 00000000000..e74636a2b61 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.expected @@ -0,0 +1,48 @@ +edges +| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:13:19:13:22 | tree | +| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:14:19:14:22 | tree | +| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:16:17:16:27 | (...)... : Object | +| OgnlInjection.java:16:17:16:27 | (...)... : Object | OgnlInjection.java:17:5:17:8 | node | +| OgnlInjection.java:16:17:16:27 | (...)... : Object | OgnlInjection.java:18:5:18:8 | node | +| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:23:19:23:22 | tree | +| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:24:19:24:22 | tree | +| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:26:5:26:8 | tree | +| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:27:5:27:8 | tree | +| OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:31:19:31:22 | expr | +| OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:32:19:32:22 | expr | +| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:37:19:37:22 | expr | +| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:38:19:38:22 | expr | +| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:39:31:39:34 | expr | +nodes +| OgnlInjection.java:11:39:11:63 | expr : String | semmle.label | expr : String | +| OgnlInjection.java:13:19:13:22 | tree | semmle.label | tree | +| OgnlInjection.java:14:19:14:22 | tree | semmle.label | tree | +| OgnlInjection.java:16:17:16:27 | (...)... : Object | semmle.label | (...)... : Object | +| OgnlInjection.java:17:5:17:8 | node | semmle.label | node | +| OgnlInjection.java:18:5:18:8 | node | semmle.label | node | +| OgnlInjection.java:21:41:21:65 | expr : String | semmle.label | expr : String | +| OgnlInjection.java:23:19:23:22 | tree | semmle.label | tree | +| OgnlInjection.java:24:19:24:22 | tree | semmle.label | tree | +| OgnlInjection.java:26:5:26:8 | tree | semmle.label | tree | +| OgnlInjection.java:27:5:27:8 | tree | semmle.label | tree | +| OgnlInjection.java:30:40:30:64 | expr : String | semmle.label | expr : String | +| OgnlInjection.java:31:19:31:22 | expr | semmle.label | expr | +| OgnlInjection.java:32:19:32:22 | expr | semmle.label | expr | +| OgnlInjection.java:35:26:35:50 | expr : String | semmle.label | expr : String | +| OgnlInjection.java:37:19:37:22 | expr | semmle.label | expr | +| OgnlInjection.java:38:19:38:22 | expr | semmle.label | expr | +| OgnlInjection.java:39:31:39:34 | expr | semmle.label | expr | +#select +| OgnlInjection.java:13:19:13:22 | tree | OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:13:19:13:22 | tree | OGNL expression might include input from $@. | OgnlInjection.java:11:39:11:63 | expr | this user input | +| OgnlInjection.java:14:19:14:22 | tree | OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:14:19:14:22 | tree | OGNL expression might include input from $@. | OgnlInjection.java:11:39:11:63 | expr | this user input | +| OgnlInjection.java:17:5:17:8 | node | OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:17:5:17:8 | node | OGNL expression might include input from $@. | OgnlInjection.java:11:39:11:63 | expr | this user input | +| OgnlInjection.java:18:5:18:8 | node | OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:18:5:18:8 | node | OGNL expression might include input from $@. | OgnlInjection.java:11:39:11:63 | expr | this user input | +| OgnlInjection.java:23:19:23:22 | tree | OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:23:19:23:22 | tree | OGNL expression might include input from $@. | OgnlInjection.java:21:41:21:65 | expr | this user input | +| OgnlInjection.java:24:19:24:22 | tree | OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:24:19:24:22 | tree | OGNL expression might include input from $@. | OgnlInjection.java:21:41:21:65 | expr | this user input | +| OgnlInjection.java:26:5:26:8 | tree | OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:26:5:26:8 | tree | OGNL expression might include input from $@. | OgnlInjection.java:21:41:21:65 | expr | this user input | +| OgnlInjection.java:27:5:27:8 | tree | OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:27:5:27:8 | tree | OGNL expression might include input from $@. | OgnlInjection.java:21:41:21:65 | expr | this user input | +| OgnlInjection.java:31:19:31:22 | expr | OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:31:19:31:22 | expr | OGNL expression might include input from $@. | OgnlInjection.java:30:40:30:64 | expr | this user input | +| OgnlInjection.java:32:19:32:22 | expr | OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:32:19:32:22 | expr | OGNL expression might include input from $@. | OgnlInjection.java:30:40:30:64 | expr | this user input | +| OgnlInjection.java:37:19:37:22 | expr | OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:37:19:37:22 | expr | OGNL expression might include input from $@. | OgnlInjection.java:35:26:35:50 | expr | this user input | +| OgnlInjection.java:38:19:38:22 | expr | OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:38:19:38:22 | expr | OGNL expression might include input from $@. | OgnlInjection.java:35:26:35:50 | expr | this user input | +| OgnlInjection.java:39:31:39:34 | expr | OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:39:31:39:34 | expr | OGNL expression might include input from $@. | OgnlInjection.java:35:26:35:50 | expr | this user input | diff --git a/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.java b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.java new file mode 100644 index 00000000000..300583c6fc9 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.java @@ -0,0 +1,41 @@ +import ognl.Node; +import ognl.Ognl; + +import java.util.HashMap; + +import com.opensymphony.xwork2.ognl.OgnlUtil; + +import org.springframework.web.bind.annotation.RequestParam; + +public class OgnlInjection { + public void testOgnlParseExpression(@RequestParam String expr) throws Exception { + Object tree = Ognl.parseExpression(expr); + Ognl.getValue(tree, new HashMap<>(), new Object()); + Ognl.setValue(tree, new HashMap<>(), new Object()); + + Node node = (Node) tree; + node.getValue(null, new Object()); + node.setValue(null, new Object(), new Object()); + } + + public void testOgnlCompileExpression(@RequestParam String expr) throws Exception { + Node tree = Ognl.compileExpression(null, new Object(), expr); + Ognl.getValue(tree, new HashMap<>(), new Object()); + Ognl.setValue(tree, new HashMap<>(), new Object()); + + tree.getValue(null, new Object()); + tree.setValue(null, new Object(), new Object()); + } + + public void testOgnlDirectlyToGetSet(@RequestParam String expr) throws Exception { + Ognl.getValue(expr, new Object()); + Ognl.setValue(expr, new Object(), new Object()); + } + + public void testStruts(@RequestParam String expr) throws Exception { + OgnlUtil ognl = new OgnlUtil(); + ognl.getValue(expr, new HashMap<>(), new Object()); + ognl.setValue(expr, new HashMap<>(), new Object(), new Object()); + new OgnlUtil().callMethod(expr, new HashMap<>(), new Object()); + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.qlref b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.qlref new file mode 100644 index 00000000000..668f3bf2797 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-917/OgnlInjection.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-917/options b/java/ql/test/experimental/query-tests/security/CWE-917/options new file mode 100644 index 00000000000..c5767a074fa --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-917/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.2.3:${testdir}/../../../stubs/ognl-3.2.14:${testdir}/../../../stubs/struts2-core-2.5.22 diff --git a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/JavaSource.java b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/JavaSource.java new file mode 100644 index 00000000000..0ca2ecb43a7 --- /dev/null +++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/JavaSource.java @@ -0,0 +1,3 @@ +package ognl; + +public interface JavaSource {} diff --git a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Node.java b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Node.java new file mode 100644 index 00000000000..56d58f24d07 --- /dev/null +++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Node.java @@ -0,0 +1,6 @@ +package ognl; + +public interface Node extends JavaSource { + public Object getValue(OgnlContext context, Object source) throws OgnlException; + public void setValue(OgnlContext context, Object target, Object value) throws OgnlException; +} diff --git a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Ognl.java b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Ognl.java new file mode 100644 index 00000000000..1aa67646f92 --- /dev/null +++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Ognl.java @@ -0,0 +1,26 @@ +package ognl; + +import java.util.*; + +public abstract class Ognl { + public static Object parseExpression(String expression) throws OgnlException { + return new Object(); + } + + public static Object getValue(Object tree, Map context, Object root) throws OgnlException { + return new Object(); + } + + public static void setValue(Object tree, Object root, Object value) throws OgnlException {} + + public static Node compileExpression(OgnlContext context, Object root, String expression) + throws Exception { + return null; + } + + public static Object getValue(String expression, Object root) throws OgnlException { + return new Object(); + } + + public static void setValue(String expression, Object root, Object value) throws OgnlException {} +} diff --git a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlContext.java b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlContext.java new file mode 100644 index 00000000000..fad33eb8e80 --- /dev/null +++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlContext.java @@ -0,0 +1,71 @@ +package ognl; + +import java.util.*; + +public class OgnlContext extends Object implements Map { + @Override + public int size() { + return 0; + } + + @Override + public boolean isEmpty() { + return false; + } + + @Override + public boolean containsKey(Object key) { + return true; + } + + @Override + public boolean containsValue(Object value) { + return true; + } + + @Override + public Object get(Object key) { + return new Object(); + } + + @Override + public Object put(Object key, Object value) { + return new Object(); + } + + @Override + public Object remove(Object key) { + return new Object(); + } + + @Override + public void putAll(Map t) { } + + @Override + public void clear() {} + + @Override + public Set keySet() { + return new HashSet(); + } + + @Override + public Collection values() { + return new HashSet(); + } + + @Override + public Set entrySet() { + return new HashSet(); + } + + @Override + public boolean equals(Object o) { + return true; + } + + @Override + public int hashCode() { + return 0; + } +} \ No newline at end of file diff --git a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlException.java b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlException.java new file mode 100644 index 00000000000..447515aa58f --- /dev/null +++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlException.java @@ -0,0 +1,3 @@ +package ognl; + +public class OgnlException extends Exception {} \ No newline at end of file diff --git a/java/ql/test/experimental/stubs/struts2-core-2.5.22/com/opensymphony/xwork2/ognl/OgnlUtil.java b/java/ql/test/experimental/stubs/struts2-core-2.5.22/com/opensymphony/xwork2/ognl/OgnlUtil.java new file mode 100644 index 00000000000..5b1d031cf21 --- /dev/null +++ b/java/ql/test/experimental/stubs/struts2-core-2.5.22/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -0,0 +1,16 @@ +package com.opensymphony.xwork2.ognl; + +import java.util.*; +import ognl.OgnlException; + +public class OgnlUtil { + public Object getValue(final String name, final Map context, final Object root) throws OgnlException { + return new Object(); + } + + public void setValue(final String name, final Map context, final Object root, final Object value) throws OgnlException {} + + public Object callMethod(final String name, final Map context, final Object root) throws OgnlException { + return new Object(); + } +} \ No newline at end of file