Skip to content

Commit 08bdd1a

Browse files
atorralbaatorralba
authored
Merge branch 'main' into atorralba/promote-ognl-injection
2 parents 8b50b3d + bbbbeda commit 08bdd1a

File tree

21 files changed

+384
-475
lines changed

21 files changed

+384
-475
lines changed

‎cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -735,7 +735,12 @@ private module FieldFlow {
735735
private class FieldConfiguration extends Configuration {
736736
FieldConfiguration() { this = "FieldConfiguration" }
737737

738-
override predicate isSource(Node source) { storeStep(source, _, _) }
738+
override predicate isSource(Node source) {
739+
storeStep(source, _, _)
740+
or
741+
// Also mark `foo(a.b);` as a source when `a.b` may be overwritten by `foo`.
742+
readStep(_, _, any(Node node | node.asExpr() = source.asDefiningArgument()))
743+
}
739744

740745
override predicate isSink(Node sink) { readStep(_, _, sink) }
741746

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,8 @@
7070
| test.cpp:391:11:391:13 | tmp | test.cpp:391:10:391:13 | & ... |
7171
| test.cpp:391:17:391:23 | source1 | test.cpp:391:10:391:13 | ref arg & ... |
7272
| test.cpp:391:17:391:23 | source1 | test.cpp:391:16:391:23 | & ... |
73+
| test.cpp:480:67:480:67 | s | test.cpp:481:21:481:21 | s |
74+
| test.cpp:480:67:480:67 | s | test.cpp:482:20:482:20 | s |
75+
| test.cpp:481:21:481:21 | s [post update] | test.cpp:482:20:482:20 | s |
76+
| test.cpp:481:24:481:30 | ref arg content | test.cpp:482:23:482:29 | content |
77+
| test.cpp:482:23:482:29 | content | test.cpp:483:9:483:17 | p_content |

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -470,3 +470,15 @@ void viaOutparam() {
470470
intOutparamSource(&x);
471471
sink(x); // $ ast,ir
472472
}
473+
474+
void writes_to_content(void*);
475+
476+
struct MyStruct {
477+
int* content;
478+
};
479+
480+
void local_field_flow_def_by_ref_steps_with_local_flow(MyStruct * s) {
481+
writes_to_content(s->content);
482+
int* p_content = s->content;
483+
sink(*p_content);
484+
}

‎cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -496,9 +496,13 @@
496496
| map.cpp:49:7:49:7 | f [post update] | map.cpp:51:7:51:7 | f | |
497497
| map.cpp:49:7:49:7 | f [post update] | map.cpp:53:30:53:30 | f | |
498498
| map.cpp:49:7:49:7 | f [post update] | map.cpp:59:6:59:6 | f | |
499+
| map.cpp:49:9:49:13 | ref arg first | map.cpp:54:9:54:13 | first | |
500+
| map.cpp:49:9:49:13 | ref arg first | map.cpp:60:9:60:13 | first | |
499501
| map.cpp:50:7:50:7 | f [post update] | map.cpp:51:7:51:7 | f | |
500502
| map.cpp:50:7:50:7 | f [post update] | map.cpp:53:30:53:30 | f | |
501503
| map.cpp:50:7:50:7 | f [post update] | map.cpp:59:6:59:6 | f | |
504+
| map.cpp:50:9:50:14 | ref arg second | map.cpp:55:9:55:14 | second | |
505+
| map.cpp:50:9:50:14 | ref arg second | map.cpp:61:9:61:14 | second | |
502506
| map.cpp:53:30:53:30 | f | map.cpp:54:7:54:7 | g | |
503507
| map.cpp:53:30:53:30 | f | map.cpp:55:7:55:7 | g | |
504508
| map.cpp:53:30:53:30 | f | map.cpp:56:7:56:7 | g | |
@@ -3395,6 +3399,7 @@
33953399
| smart_pointer.cpp:125:20:125:20 | call to operator-> [post update] | smart_pointer.cpp:125:18:125:19 | ref arg p1 | TAINT |
33963400
| smart_pointer.cpp:125:22:125:22 | q | smart_pointer.cpp:125:18:125:22 | call to shared_ptr | |
33973401
| smart_pointer.cpp:125:22:125:22 | ref arg q | smart_pointer.cpp:125:22:125:22 | q [inner post update] | |
3402+
| smart_pointer.cpp:125:22:125:22 | ref arg q | smart_pointer.cpp:126:12:126:12 | q | |
33983403
| smart_pointer.cpp:126:8:126:9 | p1 | smart_pointer.cpp:126:10:126:10 | call to operator-> | |
33993404
| smart_pointer.cpp:126:8:126:9 | ref arg p1 | smart_pointer.cpp:124:48:124:49 | p1 | |
34003405
| smart_pointer.cpp:126:10:126:10 | call to operator-> [post update] | smart_pointer.cpp:126:8:126:9 | ref arg p1 | TAINT |
@@ -3432,6 +3437,7 @@
34323437
| smart_pointer.cpp:133:23:133:24 | ref arg p1 | smart_pointer.cpp:132:53:132:54 | p1 | |
34333438
| smart_pointer.cpp:133:23:133:24 | ref arg p1 | smart_pointer.cpp:134:8:134:9 | p1 | |
34343439
| smart_pointer.cpp:133:25:133:25 | call to operator-> [post update] | smart_pointer.cpp:133:23:133:24 | ref arg p1 | TAINT |
3440+
| smart_pointer.cpp:133:27:133:27 | ref arg q | smart_pointer.cpp:134:12:134:12 | q | |
34353441
| smart_pointer.cpp:134:8:134:9 | p1 | smart_pointer.cpp:134:10:134:10 | call to operator-> | |
34363442
| smart_pointer.cpp:134:8:134:9 | ref arg p1 | smart_pointer.cpp:132:53:132:54 | p1 | |
34373443
| smart_pointer.cpp:134:10:134:10 | call to operator-> [post update] | smart_pointer.cpp:134:8:134:9 | ref arg p1 | TAINT |
@@ -6435,6 +6441,7 @@
64356441
| taint.cpp:669:18:669:18 | s [post update] | taint.cpp:671:7:671:7 | s | |
64366442
| taint.cpp:669:18:669:18 | s [post update] | taint.cpp:672:7:672:7 | s | |
64376443
| taint.cpp:669:18:669:18 | s [post update] | taint.cpp:673:7:673:7 | s | |
6444+
| taint.cpp:669:20:669:20 | ref arg x | taint.cpp:672:9:672:9 | x | |
64386445
| taint.cpp:672:7:672:7 | s [post update] | taint.cpp:673:7:673:7 | s | |
64396446
| vector.cpp:16:43:16:49 | source1 | vector.cpp:17:26:17:32 | source1 | |
64406447
| vector.cpp:16:43:16:49 | source1 | vector.cpp:31:38:31:44 | source1 | |
@@ -7076,14 +7083,20 @@
70767083
| vector.cpp:198:3:198:4 | ee [post update] | vector.cpp:200:3:200:4 | ee | |
70777084
| vector.cpp:198:3:198:4 | ee [post update] | vector.cpp:201:8:201:9 | ee | |
70787085
| vector.cpp:198:3:198:4 | ee [post update] | vector.cpp:202:2:202:2 | ee | |
7086+
| vector.cpp:198:6:198:7 | ref arg vs | vector.cpp:199:11:199:12 | vs | |
7087+
| vector.cpp:198:6:198:7 | ref arg vs | vector.cpp:200:6:200:7 | vs | |
7088+
| vector.cpp:198:6:198:7 | ref arg vs | vector.cpp:201:11:201:12 | vs | |
70797089
| vector.cpp:198:19:198:19 | 0 | vector.cpp:198:6:198:7 | ref arg vs | TAINT |
70807090
| vector.cpp:199:8:199:9 | ee [post update] | vector.cpp:200:3:200:4 | ee | |
70817091
| vector.cpp:199:8:199:9 | ee [post update] | vector.cpp:201:8:201:9 | ee | |
70827092
| vector.cpp:199:8:199:9 | ee [post update] | vector.cpp:202:2:202:2 | ee | |
7093+
| vector.cpp:199:11:199:12 | ref arg vs | vector.cpp:200:6:200:7 | vs | |
7094+
| vector.cpp:199:11:199:12 | ref arg vs | vector.cpp:201:11:201:12 | vs | |
70837095
| vector.cpp:199:11:199:12 | vs | vector.cpp:199:13:199:13 | call to operator[] | TAINT |
70847096
| vector.cpp:200:3:200:4 | ee [post update] | vector.cpp:201:8:201:9 | ee | |
70857097
| vector.cpp:200:3:200:4 | ee [post update] | vector.cpp:202:2:202:2 | ee | |
70867098
| vector.cpp:200:3:200:21 | ... = ... | vector.cpp:200:8:200:8 | call to operator[] [post update] | |
7099+
| vector.cpp:200:6:200:7 | ref arg vs | vector.cpp:201:11:201:12 | vs | |
70877100
| vector.cpp:200:6:200:7 | vs | vector.cpp:200:8:200:8 | call to operator[] | TAINT |
70887101
| vector.cpp:200:8:200:8 | call to operator[] [post update] | vector.cpp:200:6:200:7 | ref arg vs | TAINT |
70897102
| vector.cpp:200:14:200:19 | call to source | vector.cpp:200:3:200:21 | ... = ... | |

‎java/change-notes/2021-06-02-mvel-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 "Expression language injection (MVEL) (`java/mvel-expression-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 @artem-smotrakov](https://github.com/github/codeql/pull/3329)

‎java/ql/src/Security/CWE/CWE-094/MvelExpressionEvaluation.java

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
public void evaluate(Socket socket) throws IOException {
2+
try (BufferedReader reader = new BufferedReader(
3+
new InputStreamReader(socket.getInputStream()))) {
4+
5+
String expression = reader.readLine();
6+
// BAD: the user-provided expression is directly evaluated
7+
MVEL.eval(expression);
8+
}
9+
}
10+
11+
public void safeEvaluate(Socket socket) throws IOException {
12+
try (BufferedReader reader = new BufferedReader(
13+
new InputStreamReader(socket.getInputStream()))) {
14+
15+
String expression = reader.readLine();
16+
// GOOD: the user-provided expression is validated before evaluation
17+
validateExpression(expression);
18+
MVEL.eval(expression);
19+
}
20+
}
21+
22+
private void validateExpression(String expression) {
23+
// Validate that the expression does not contain unexpected code.
24+
// For instance, this can be done with allow-lists or deny-lists of code patterns.
25+
}

‎java/ql/src/experimental/Security/CWE/CWE-094/MvelInjection.qhelp renamed to ‎java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@
33

44
<overview>
55
<p>
6-
MVEL is an expression language based on Java-syntax.
7-
The language offers many features
6+
MVEL is an expression language based on Java-syntax,
7+
which offers many features
88
including invocation of methods available in the JVM.
99
If a MVEL expression is built using attacker-controlled data,
10-
and then evaluated, then it may allow the attacker to run arbitrary code.
10+
and then evaluated, then it may allow attackers to run arbitrary code.
1111
</p>
1212
</overview>
1313

@@ -19,10 +19,12 @@ Including user input in a MVEL expression should be avoided.
1919

2020
<example>
2121
<p>
22-
The following example uses untrusted data to build a MVEL expression
23-
and then runs it in the default powerfull context.
22+
In the following sample, the first example uses untrusted data to build a MVEL expression
23+
and then runs it in the default context. In the second example, the untrusted data is
24+
validated with a custom method that checks that the expression does not contain unexpected code
25+
before evaluating it.
2426
</p>
25-
<sample src="UnsafeMvelExpressionEvaluation.java" />
27+
<sample src="MvelExpressionEvaluation.java" />
2628
</example>
2729

2830
<references>
@@ -35,4 +37,4 @@ and then runs it in the default powerfull context.
3537
<a href="https://owasp.org/www-community/vulnerabilities/Expression_Language_Injection">Expression Language Injection</a>.
3638
</li>
3739
</references>
38-
</qhelp>
40+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-094/MvelInjection.ql renamed to ‎java/ql/src/Security/CWE/CWE-094/MvelInjection.ql

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

1313
import java
14-
import MvelInjectionLib
14+
import semmle.code.java.security.MvelInjectionQuery
1515
import DataFlow::PathGraph
1616

17-
from DataFlow::PathNode source, DataFlow::PathNode sink, MvelInjectionConfig conf
17+
from DataFlow::PathNode source, DataFlow::PathNode sink, MvelInjectionFlowConfig conf
1818
where conf.hasFlowPath(source, sink)
1919
select sink.getNode(), source, sink, "MVEL injection from $@.", source.getNode(), "this user input"

0 commit comments

Comments
 (0)