Skip to content

Commit 13cb853

Browse files
aschackmullaschackmull
authored
Merge pull request github#3294 from ggolawski/ognl-injection
CodeQL query to detect OGNL injections
2 parents b57cfc9 + aff0e0e commit 13cb853

File tree

14 files changed

+397
-0
lines changed

14 files changed

+397
-0
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import ognl.Ognl;
2+
import ognl.OgnlException;
3+
4+
public void evaluate(HttpServletRequest request, Object root) throws OgnlException {
5+
String expression = request.getParameter("expression");
6+
7+
// BAD: User provided expression is evaluated
8+
Ognl.getValue(expression, root);
9+
10+
// GOOD: The name is validated and expression is evaluated in sandbox
11+
System.setProperty("ognl.security.manager", ""); // Or add -Dognl.security.manager to JVM args
12+
if (isValid(expression)) {
13+
Ognl.getValue(expression, root);
14+
} else {
15+
// Reject the request
16+
}
17+
}

‎java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.qhelp

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Object-Graph Navigation Language (OGNL) is an open-source Expression Language (EL) for Java. Due
7+
to its ability to create or change executable code, OGNL is capable of introducing critical
8+
security flaws to any application that uses it. Evaluation of unvalidated expressions can let
9+
attacker to modify Java objects' properties or execute arbitrary code.</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>The general recommendation is to not evaluate untrusted ONGL expressions. If user provided OGNL
14+
expressions must be evaluated, do this in sandbox (add `-Dognl.security.manager` to JVM arguments)
15+
and validate the expressions before evaluation.</p>
16+
</recommendation>
17+
18+
<example>
19+
<p>In the following examples, the code accepts an OGNL expression from the user and evaluates it.
20+
</p>
21+
22+
<p>In the first example, the user provided OGNL expression is parsed and evaluated.</p>
23+
24+
<p>The second example validates the expression and evaluates it inside the sandbox.</p>
25+
26+
<sample src="OgnlInjection.java" />
27+
</example>
28+
29+
<references>
30+
<li><a href="https://github.com/jkuhnert/ognl/">OGNL library</a>.</li>
31+
<li>Struts security: <a href="https://struts.apache.org/security/#proactively-protect-from-ognl-expression-injections-attacks-if-easily-applicable">Proactively protect from OGNL Expression Injections attacks</a>.</li>
32+
</references>
33+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.ql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name OGNL Expression Language statement with user-controlled input
3+
* @description Evaluation of OGNL Expression Language statement with user-controlled input can
4+
* lead to execution of arbitrary code.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id java/ognl-injection
9+
* @tags security
10+
* external/cwe/cwe-917
11+
*/
12+
13+
import java
14+
import semmle.code.java.dataflow.FlowSources
15+
import DataFlow
16+
import DataFlow::PathGraph
17+
import OgnlInjectionLib
18+
19+
from DataFlow::PathNode source, DataFlow::PathNode sink, OgnlInjectionFlowConfig conf
20+
where conf.hasFlowPath(source, sink)
21+
select sink.getNode(), source, sink, "OGNL expression might include input from $@.",
22+
source.getNode(), "this user input"

‎java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
import java
2+
import semmle.code.java.dataflow.FlowSources
3+
import DataFlow
4+
import DataFlow::PathGraph
5+
6+
/**
7+
* A taint-tracking configuration for unvalidated user input that is used in OGNL EL evaluation.
8+
*/
9+
class OgnlInjectionFlowConfig extends TaintTracking::Configuration {
10+
OgnlInjectionFlowConfig() { this = "OgnlInjectionFlowConfig" }
11+
12+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
13+
14+
override predicate isSink(DataFlow::Node sink) { sink instanceof OgnlInjectionSink }
15+
16+
override predicate isSanitizer(DataFlow::Node node) {
17+
node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
18+
}
19+
20+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
21+
parseCompileExpressionStep(node1, node2)
22+
}
23+
}
24+
25+
/** The class `org.apache.commons.ognl.Ognl` or `ognl.Ognl`. */
26+
class TypeOgnl extends Class {
27+
TypeOgnl() {
28+
this.hasQualifiedName("org.apache.commons.ognl", "Ognl") or
29+
this.hasQualifiedName("ognl", "Ognl")
30+
}
31+
}
32+
33+
/** The interface `org.apache.commons.ognl.Node` or `ognl.Node`. */
34+
class TypeNode extends Interface {
35+
TypeNode() {
36+
this.hasQualifiedName("org.apache.commons.ognl", "Node") or
37+
this.hasQualifiedName("ognl", "Node")
38+
}
39+
}
40+
41+
/** The class `com.opensymphony.xwork2.ognl.OgnlUtil`. */
42+
class TypeOgnlUtil extends Class {
43+
TypeOgnlUtil() { this.hasQualifiedName("com.opensymphony.xwork2.ognl", "OgnlUtil") }
44+
}
45+
46+
/**
47+
* OGNL sink for OGNL injection vulnerabilities, i.e. 1st argument to `getValue` or `setValue`
48+
* method from `Ognl` or `getValue` or `setValue` method from `Node`.
49+
*/
50+
predicate ognlSinkMethod(Method m, int index) {
51+
(
52+
m.getDeclaringType() instanceof TypeOgnl
53+
or
54+
m.getDeclaringType().getAnAncestor*() instanceof TypeNode
55+
) and
56+
(
57+
m.hasName("getValue") or
58+
m.hasName("setValue")
59+
) and
60+
index = 0
61+
}
62+
63+
/**
64+
* Struts sink for OGNL injection vulnerabilities, i.e. 1st argument to `getValue`, `setValue` or
65+
* `callMethod` method from `OgnlUtil`.
66+
*/
67+
predicate strutsSinkMethod(Method m, int index) {
68+
m.getDeclaringType() instanceof TypeOgnlUtil and
69+
(
70+
m.hasName("getValue") or
71+
m.hasName("setValue") or
72+
m.hasName("callMethod")
73+
) and
74+
index = 0
75+
}
76+
77+
/** Holds if parameter at index `index` in method `m` is OGNL injection sink. */
78+
predicate ognlInjectionSinkMethod(Method m, int index) {
79+
ognlSinkMethod(m, index) or
80+
strutsSinkMethod(m, index)
81+
}
82+
83+
/** A data flow sink for unvalidated user input that is used in OGNL EL evaluation. */
84+
class OgnlInjectionSink extends DataFlow::ExprNode {
85+
OgnlInjectionSink() {
86+
exists(MethodAccess ma, Method m, int index |
87+
ma.getMethod() = m and
88+
(ma.getArgument(index) = this.getExpr() or ma.getQualifier() = this.getExpr()) and
89+
ognlInjectionSinkMethod(m, index)
90+
)
91+
}
92+
}
93+
94+
/**
95+
* Holds if `n1` to `n2` is a dataflow step that converts between `String` and `Object` or `Node`,
96+
* i.e. `Ognl.parseExpression(tainted)` or `Ognl.compileExpression(tainted)`.
97+
*/
98+
predicate parseCompileExpressionStep(ExprNode n1, ExprNode n2) {
99+
exists(MethodAccess ma, Method m, int index |
100+
n1.asExpr() = ma.getArgument(index) and
101+
n2.asExpr() = ma and
102+
ma.getMethod() = m and
103+
m.getDeclaringType() instanceof TypeOgnl
104+
|
105+
m.hasName("parseExpression") and index = 0
106+
or
107+
m.hasName("compileExpression") and index = 2
108+
)
109+
}

‎java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.expected

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
edges
2+
| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:13:19:13:22 | tree |
3+
| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:14:19:14:22 | tree |
4+
| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:16:17:16:27 | (...)... : Object |
5+
| OgnlInjection.java:16:17:16:27 | (...)... : Object | OgnlInjection.java:17:5:17:8 | node |
6+
| OgnlInjection.java:16:17:16:27 | (...)... : Object | OgnlInjection.java:18:5:18:8 | node |
7+
| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:23:19:23:22 | tree |
8+
| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:24:19:24:22 | tree |
9+
| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:26:5:26:8 | tree |
10+
| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:27:5:27:8 | tree |
11+
| OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:31:19:31:22 | expr |
12+
| OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:32:19:32:22 | expr |
13+
| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:37:19:37:22 | expr |
14+
| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:38:19:38:22 | expr |
15+
| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:39:31:39:34 | expr |
16+
nodes
17+
| OgnlInjection.java:11:39:11:63 | expr : String | semmle.label | expr : String |
18+
| OgnlInjection.java:13:19:13:22 | tree | semmle.label | tree |
19+
| OgnlInjection.java:14:19:14:22 | tree | semmle.label | tree |
20+
| OgnlInjection.java:16:17:16:27 | (...)... : Object | semmle.label | (...)... : Object |
21+
| OgnlInjection.java:17:5:17:8 | node | semmle.label | node |
22+
| OgnlInjection.java:18:5:18:8 | node | semmle.label | node |
23+
| OgnlInjection.java:21:41:21:65 | expr : String | semmle.label | expr : String |
24+
| OgnlInjection.java:23:19:23:22 | tree | semmle.label | tree |
25+
| OgnlInjection.java:24:19:24:22 | tree | semmle.label | tree |
26+
| OgnlInjection.java:26:5:26:8 | tree | semmle.label | tree |
27+
| OgnlInjection.java:27:5:27:8 | tree | semmle.label | tree |
28+
| OgnlInjection.java:30:40:30:64 | expr : String | semmle.label | expr : String |
29+
| OgnlInjection.java:31:19:31:22 | expr | semmle.label | expr |
30+
| OgnlInjection.java:32:19:32:22 | expr | semmle.label | expr |
31+
| OgnlInjection.java:35:26:35:50 | expr : String | semmle.label | expr : String |
32+
| OgnlInjection.java:37:19:37:22 | expr | semmle.label | expr |
33+
| OgnlInjection.java:38:19:38:22 | expr | semmle.label | expr |
34+
| OgnlInjection.java:39:31:39:34 | expr | semmle.label | expr |
35+
#select
36+
| 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 |
37+
| 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 |
38+
| 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 |
39+
| 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 |
40+
| 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 |
41+
| 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 |
42+
| 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 |
43+
| 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 |
44+
| 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 |
45+
| 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 |
46+
| 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 |
47+
| 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 |
48+
| 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 |

‎java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.java

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import ognl.Node;
2+
import ognl.Ognl;
3+
4+
import java.util.HashMap;
5+
6+
import com.opensymphony.xwork2.ognl.OgnlUtil;
7+
8+
import org.springframework.web.bind.annotation.RequestParam;
9+
10+
public class OgnlInjection {
11+
public void testOgnlParseExpression(@RequestParam String expr) throws Exception {
12+
Object tree = Ognl.parseExpression(expr);
13+
Ognl.getValue(tree, new HashMap<>(), new Object());
14+
Ognl.setValue(tree, new HashMap<>(), new Object());
15+
16+
Node node = (Node) tree;
17+
node.getValue(null, new Object());
18+
node.setValue(null, new Object(), new Object());
19+
}
20+
21+
public void testOgnlCompileExpression(@RequestParam String expr) throws Exception {
22+
Node tree = Ognl.compileExpression(null, new Object(), expr);
23+
Ognl.getValue(tree, new HashMap<>(), new Object());
24+
Ognl.setValue(tree, new HashMap<>(), new Object());
25+
26+
tree.getValue(null, new Object());
27+
tree.setValue(null, new Object(), new Object());
28+
}
29+
30+
public void testOgnlDirectlyToGetSet(@RequestParam String expr) throws Exception {
31+
Ognl.getValue(expr, new Object());
32+
Ognl.setValue(expr, new Object(), new Object());
33+
}
34+
35+
public void testStruts(@RequestParam String expr) throws Exception {
36+
OgnlUtil ognl = new OgnlUtil();
37+
ognl.getValue(expr, new HashMap<>(), new Object());
38+
ognl.setValue(expr, new HashMap<>(), new Object(), new Object());
39+
new OgnlUtil().callMethod(expr, new HashMap<>(), new Object());
40+
}
41+
}

‎java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-917/OgnlInjection.ql

‎java/ql/test/experimental/query-tests/security/CWE-917/options

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
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

‎java/ql/test/experimental/stubs/ognl-3.2.14/ognl/JavaSource.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
package ognl;
2+
3+
public interface JavaSource {}

‎java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Node.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
package ognl;
2+
3+
public interface Node extends JavaSource {
4+
public Object getValue(OgnlContext context, Object source) throws OgnlException;
5+
public void setValue(OgnlContext context, Object target, Object value) throws OgnlException;
6+
}

0 commit comments

Comments
 (0)