Skip to content

Commit 7fb1e15

Browse files
aschackmullaschackmull
authored
Merge pull request github#5894 from atorralba/atorralba/promote-ognl-injection
Java: Promote OGNL Injection query from experimental
2 parents be6fd7c + f5cbec4 commit 7fb1e15

File tree

68 files changed

+3033
-475
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

68 files changed

+3033
-475
lines changed

‎java/change-notes/2021-05-13-ognl-injection-query.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query "OGNL Expression Language statement with user-controlled input" (`java/ognl-injection`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @ggolawski](https://github.com/github/codeql/pull/3294).

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,9 @@ public void evaluate(HttpServletRequest request, Object root) throws OgnlExcepti
1414
} else {
1515
// Reject the request
1616
}
17-
}
17+
}
18+
19+
public void isValid(Strig expression) {
20+
// Custom method to validate the expression.
21+
// For instance, make sure it doesn't include unexpected code.
22+
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
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.
7+
OGNL can create or change executable code, consequently it can introduce critical
8+
security flaws to any application that uses it. Evaluation of unvalidated expressions is a common
9+
flaw in OGNL. This exposes the properties of Java objects to modification by an attacker and
10+
may allow them to execute arbitrary code.</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>The general recommendation is to avoid evaluating untrusted ONGL expressions. If user-provided OGNL
15+
expressions must be evaluated, do this in a sandbox 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 a sandbox.
25+
You can add a sandbox by setting a system property, as shown in the example, or by adding
26+
<code>-Dognl.security.manager</code> to JVM arguments.</p>
27+
28+
<sample src="OgnlInjection.java" />
29+
</example>
30+
31+
<references>
32+
<li>Apache Commons: <a href="https://commons.apache.org/proper/commons-ognl/">Apache Commons OGNL</a>.</li>
33+
<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>
34+
</references>
35+
</qhelp>

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,10 @@
1111
*/
1212

1313
import java
14-
import semmle.code.java.dataflow.FlowSources
15-
import DataFlow
14+
import semmle.code.java.security.OgnlInjectionQuery
1615
import DataFlow::PathGraph
17-
import OgnlInjectionLib
1816

1917
from DataFlow::PathNode source, DataFlow::PathNode sink, OgnlInjectionFlowConfig conf
2018
where conf.hasFlowPath(source, sink)
21-
select sink.getNode(), source, sink, "OGNL expression might include input from $@.",
19+
select sink.getNode(), source, sink, "OGNL expression might include data from $@.",
2220
source.getNode(), "this user input"

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

Lines changed: 0 additions & 33 deletions
This file was deleted.

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

Lines changed: 0 additions & 109 deletions
This file was deleted.

‎java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ private module Frameworks {
101101
private import semmle.code.java.security.JexlInjectionSinkModels
102102
private import semmle.code.java.security.LdapInjection
103103
private import semmle.code.java.security.MvelInjection
104+
private import semmle.code.java.security.OgnlInjection
104105
private import semmle.code.java.security.XPath
105106
private import semmle.code.java.frameworks.android.SQLite
106107
private import semmle.code.java.frameworks.Jdbc

‎java/ql/src/semmle/code/java/frameworks/jackson/JacksonSerializability.qll

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ private class JacksonWriteValueMethod extends Method, TaintPreservingCallable {
5151
}
5252
}
5353

54+
/**
55+
* A method used for deserializing objects using Jackson. The first parameter is the object to be
56+
* deserialized.
57+
*/
5458
private class JacksonReadValueMethod extends Method, TaintPreservingCallable {
5559
JacksonReadValueMethod() {
5660
(
@@ -281,7 +285,10 @@ private class JacksonModel extends SummaryModelCsv {
281285
[
282286
"com.fasterxml.jackson.databind;ObjectMapper;true;valueToTree;;;Argument[0];ReturnValue;taint",
283287
"com.fasterxml.jackson.databind;ObjectMapper;true;valueToTree;;;MapValue of Argument[0];ReturnValue;taint",
284-
"com.fasterxml.jackson.databind;ObjectMapper;true;convertValue;;;Argument[0];ReturnValue;taint"
288+
"com.fasterxml.jackson.databind;ObjectMapper;true;convertValue;;;Argument[0];ReturnValue;taint",
289+
"com.fasterxml.jackson.databind;ObjectMapper;false;createParser;;;Argument[0];ReturnValue;taint",
290+
"com.fasterxml.jackson.databind;ObjectReader;false;createParser;;;Argument[0];ReturnValue;taint",
291+
"com.fasterxml.jackson.core;JsonFactory;false;createParser;;;Argument[0];ReturnValue;taint"
285292
]
286293
}
287294
}

‎java/ql/src/semmle/code/java/security/OgnlInjection.qll

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
/** Provides classes to reason about OGNL injection vulnerabilities. */
2+
3+
import java
4+
private import semmle.code.java.dataflow.DataFlow
5+
private import semmle.code.java.dataflow.ExternalFlow
6+
7+
/**
8+
* A data flow sink for unvalidated user input that is used in OGNL EL evaluation.
9+
*
10+
* Extend this class to add your own OGNL injection sinks.
11+
*/
12+
abstract class OgnlInjectionSink extends DataFlow::Node { }
13+
14+
/**
15+
* A unit class for adding additional taint steps.
16+
*
17+
* Extend this class to add additional taint steps that should apply to the `OgnlInjectionFlowConfig`.
18+
*/
19+
class OgnlInjectionAdditionalTaintStep extends Unit {
20+
/**
21+
* Holds if the step from `node1` to `node2` should be considered a taint
22+
* step for OGNL injection taint configurations.
23+
*/
24+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
25+
}
26+
27+
private class DefaultOgnlInjectionSinkModel extends SinkModelCsv {
28+
override predicate row(string row) {
29+
row =
30+
[
31+
"org.apache.commons.ognl;Ognl;false;getValue;;;Argument[0];ognl-injection",
32+
"org.apache.commons.ognl;Ognl;false;setValue;;;Argument[0];ognl-injection",
33+
"org.apache.commons.ognl;Node;true;getValue;;;Argument[-1];ognl-injection",
34+
"org.apache.commons.ognl;Node;true;setValue;;;Argument[-1];ognl-injection",
35+
"org.apache.commons.ognl.enhance;ExpressionAccessor;true;get;;;Argument[-1];ognl-injection",
36+
"org.apache.commons.ognl.enhance;ExpressionAccessor;true;set;;;Argument[-1];ognl-injection",
37+
"ognl;Ognl;false;getValue;;;Argument[0];ognl-injection",
38+
"ognl;Ognl;false;setValue;;;Argument[0];ognl-injection",
39+
"ognl;Node;false;getValue;;;Argument[-1];ognl-injection",
40+
"ognl;Node;false;setValue;;;Argument[-1];ognl-injection",
41+
"ognl.enhance;ExpressionAccessor;true;get;;;Argument[-1];ognl-injection",
42+
"ognl.enhance;ExpressionAccessor;true;set;;;Argument[-1];ognl-injection",
43+
"com.opensymphony.xwork2.ognl;OgnlUtil;false;getValue;;;Argument[0];ognl-injection",
44+
"com.opensymphony.xwork2.ognl;OgnlUtil;false;setValue;;;Argument[0];ognl-injection",
45+
"com.opensymphony.xwork2.ognl;OgnlUtil;false;callMethod;;;Argument[0];ognl-injection"
46+
]
47+
}
48+
}
49+
50+
private class DefaultOgnlInjectionSink extends OgnlInjectionSink {
51+
DefaultOgnlInjectionSink() { sinkNode(this, "ognl-injection") }
52+
}
53+
54+
/** The class `org.apache.commons.ognl.Ognl` or `ognl.Ognl`. */
55+
private class TypeOgnl extends Class {
56+
TypeOgnl() { this.hasQualifiedName(["org.apache.commons.ognl", "ognl"], "Ognl") }
57+
}
58+
59+
/** The interface `org.apache.commons.ognl.Node` or `ognl.Node`. */
60+
private class TypeNode extends Interface {
61+
TypeNode() { this.hasQualifiedName(["org.apache.commons.ognl", "ognl"], "Node") }
62+
}
63+
64+
/** The interface `org.apache.commons.ognl.enhance.ExpressionAccessor` or `ognl.enhance.ExpressionAccessor`. */
65+
private class TypeExpressionAccessor extends Interface {
66+
TypeExpressionAccessor() {
67+
this.hasQualifiedName(["org.apache.commons.ognl.enhance", "ognl.enhance"], "ExpressionAccessor")
68+
}
69+
}
70+
71+
/**
72+
* Holds if `n1` to `n2` is a dataflow step that converts between `String` and `Object` or `Node`,
73+
* i.e. `Ognl.parseExpression(tainted)` or `Ognl.compileExpression(tainted)`.
74+
*/
75+
private predicate parseCompileExpressionStep(DataFlow::Node n1, DataFlow::Node n2) {
76+
exists(MethodAccess ma, Method m, int index |
77+
n1.asExpr() = ma.getArgument(index) and
78+
n2.asExpr() = ma and
79+
ma.getMethod() = m and
80+
m.getDeclaringType() instanceof TypeOgnl
81+
|
82+
m.hasName("parseExpression") and index = 0
83+
or
84+
m.hasName("compileExpression") and index = 2
85+
)
86+
}
87+
88+
/**
89+
* Holds if `n1` to `n2` is a dataflow step that converts between `Node` and `Accessor`,
90+
* i.e. `Node.getAccessor()`.
91+
*/
92+
private predicate getAccessorStep(DataFlow::Node n1, DataFlow::Node n2) {
93+
exists(MethodAccess ma, Method m |
94+
ma.getMethod() = m and
95+
m.getDeclaringType().getASupertype*() instanceof TypeNode and
96+
m.hasName("getAccessor")
97+
|
98+
n1.asExpr() = ma.getQualifier() and
99+
n2.asExpr() = ma
100+
)
101+
}
102+
103+
/**
104+
* Holds if `n1` to `n2` is a dataflow step that converts between `Node` and `Accessor`
105+
* in a `setExpression` call, i.e. `accessor.setExpression(tainted)`
106+
*/
107+
private predicate setExpressionStep(DataFlow::Node n1, DataFlow::Node n2) {
108+
exists(MethodAccess ma, Method m |
109+
ma.getMethod() = m and
110+
m.hasName("setExpression") and
111+
m.getDeclaringType().getASupertype*() instanceof TypeExpressionAccessor
112+
|
113+
n1.asExpr() = ma.getArgument(0) and
114+
n2.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = ma.getQualifier()
115+
)
116+
}
117+
118+
private class DefaultOgnlInjectionAdditionalTaintStep extends OgnlInjectionAdditionalTaintStep {
119+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
120+
parseCompileExpressionStep(node1, node2) or
121+
getAccessorStep(node1, node2) or
122+
setExpressionStep(node1, node2)
123+
}
124+
}

‎java/ql/src/semmle/code/java/security/OgnlInjectionQuery.qll

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/** Provides taint tracking configurations to be used in OGNL injection queries. */
2+
3+
import java
4+
import semmle.code.java.dataflow.FlowSources
5+
import semmle.code.java.security.OgnlInjection
6+
7+
/**
8+
* A taint-tracking configuration for unvalidated user input that is used in OGNL EL evaluation.
9+
*/
10+
class OgnlInjectionFlowConfig extends TaintTracking::Configuration {
11+
OgnlInjectionFlowConfig() { this = "OgnlInjectionFlowConfig" }
12+
13+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
14+
15+
override predicate isSink(DataFlow::Node sink) { sink instanceof OgnlInjectionSink }
16+
17+
override predicate isSanitizer(DataFlow::Node node) {
18+
node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
19+
}
20+
21+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
22+
any(OgnlInjectionAdditionalTaintStep c).step(node1, node2)
23+
}
24+
}

0 commit comments

Comments
 (0)