Skip to content

Commit 3e51f6f

Browse files
erik-krogherik-krogh
committed
use flow-states to remove FPs related to an attacker only controlling a substring in code-injection
1 parent 2a72e89 commit 3e51f6f

File tree

4 files changed

+68
-9
lines changed

4 files changed

+68
-9
lines changed

‎ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,30 @@ private import codeql.ruby.dataflow.BarrierGuards
1111
* adding your own.
1212
*/
1313
module CodeInjection {
14+
/** Flow states used to distinguish whether an attacker controls the entire string. */
15+
module FlowState {
16+
/** Flow state used for normal tainted data, where an attacker might only control a substring. */
17+
DataFlow::FlowState substring() { result = "substring" }
18+
19+
/** Flow state used for data that is entirely controlled by the attacker. */
20+
DataFlow::FlowState full() { result = "full" }
21+
}
22+
1423
/**
1524
* A data flow source for "Code injection" vulnerabilities.
1625
*/
17-
abstract class Source extends DataFlow::Node { }
26+
abstract class Source extends DataFlow::Node {
27+
/** Gets a flow state for which this is a source. */
28+
DataFlow::FlowState getAFlowState() { result = [FlowState::substring(), FlowState::full()] }
29+
}
1830

1931
/**
2032
* A data flow sink for "Code injection" vulnerabilities.
2133
*/
22-
abstract class Sink extends DataFlow::Node { }
34+
abstract class Sink extends DataFlow::Node {
35+
/** Holds if this sink is safe for an attacker that only controls a substring. */
36+
DataFlow::FlowState getAFlowState() { result = [FlowState::substring(), FlowState::full()] }
37+
}
2338

2439
/**
2540
* A sanitizer for "Code injection" vulnerabilities.
@@ -42,6 +57,15 @@ module CodeInjection {
4257
* A call that evaluates its arguments as Ruby code, considered as a flow sink.
4358
*/
4459
class CodeExecutionAsSink extends Sink {
45-
CodeExecutionAsSink() { this = any(CodeExecution c).getCode() }
60+
CodeExecution c;
61+
62+
CodeExecutionAsSink() { this = c.getCode() }
63+
64+
/** Gets a flow state for which this is a sink. */
65+
override DataFlow::FlowState getAFlowState() {
66+
if c.runsImmediately()
67+
then result = [FlowState::substring(), FlowState::full()] // If it runs immediately, then it's always vulnerable.
68+
else result = FlowState::full() // If it "just" loads something, then it's only vulnerable if the attacker controls the entire string.
69+
}
4670
}
4771
}

‎ruby/ql/lib/codeql/ruby/security/CodeInjectionQuery.qll

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,23 +9,40 @@ import codeql.ruby.DataFlow
99
import codeql.ruby.TaintTracking
1010
import CodeInjectionCustomizations::CodeInjection
1111
import codeql.ruby.dataflow.BarrierGuards
12+
private import codeql.ruby.AST as Ast
1213

1314
/**
1415
* A taint-tracking configuration for detecting "Code injection" vulnerabilities.
1516
*/
1617
class Configuration extends TaintTracking::Configuration {
1718
Configuration() { this = "CodeInjection" }
1819

19-
override predicate isSource(DataFlow::Node source) { source instanceof Source }
20+
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
21+
state = source.(Source).getAFlowState()
22+
}
2023

21-
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
24+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
25+
state = sink.(Sink).getAFlowState()
26+
}
2227

2328
override predicate isSanitizer(DataFlow::Node node) {
2429
node instanceof Sanitizer or
2530
node instanceof StringConstCompareBarrier or
2631
node instanceof StringConstArrayInclusionCallBarrier
2732
}
2833

34+
override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) {
35+
// string concatenations sanitize the `full` state, as an attacker no longer controls the entire string
36+
exists(Ast::AstNode str |
37+
str instanceof Ast::StringLiteral
38+
or
39+
str instanceof Ast::AddExpr
40+
|
41+
node.asExpr().getExpr() = str and
42+
state = FlowState::full()
43+
)
44+
}
45+
2946
deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
3047
guard instanceof SanitizerGuard
3148
}

‎ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,41 +1,57 @@
11
edges
22
| CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:5:12:5:24 | ...[...] : |
3+
| CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:5:12:5:24 | ...[...] : |
4+
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:8:10:8:13 | code |
35
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:8:10:8:13 | code |
46
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:20:20:20:23 | code |
7+
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:20:20:20:23 | code |
8+
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:23:21:23:24 | code |
59
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:23:21:23:24 | code |
610
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:29:15:29:18 | code |
711
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:32:19:32:22 | code |
812
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:38:24:38:27 | code : |
13+
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:38:24:38:27 | code : |
914
| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:41:40:41:43 | code |
1015
| CodeInjection.rb:38:24:38:27 | code : | CodeInjection.rb:38:10:38:28 | call to escape |
16+
| CodeInjection.rb:38:24:38:27 | code : | CodeInjection.rb:38:10:38:28 | call to escape |
1117
| CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:78:12:78:24 | ...[...] : |
1218
| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:80:16:80:19 | code |
13-
| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:82:16:82:43 | ... + ... |
1419
nodes
1520
| CodeInjection.rb:5:12:5:17 | call to params : | semmle.label | call to params : |
21+
| CodeInjection.rb:5:12:5:17 | call to params : | semmle.label | call to params : |
22+
| CodeInjection.rb:5:12:5:24 | ...[...] : | semmle.label | ...[...] : |
1623
| CodeInjection.rb:5:12:5:24 | ...[...] : | semmle.label | ...[...] : |
1724
| CodeInjection.rb:8:10:8:13 | code | semmle.label | code |
25+
| CodeInjection.rb:8:10:8:13 | code | semmle.label | code |
26+
| CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params |
1827
| CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params |
1928
| CodeInjection.rb:20:20:20:23 | code | semmle.label | code |
29+
| CodeInjection.rb:20:20:20:23 | code | semmle.label | code |
30+
| CodeInjection.rb:23:21:23:24 | code | semmle.label | code |
2031
| CodeInjection.rb:23:21:23:24 | code | semmle.label | code |
2132
| CodeInjection.rb:29:15:29:18 | code | semmle.label | code |
2233
| CodeInjection.rb:32:19:32:22 | code | semmle.label | code |
2334
| CodeInjection.rb:38:10:38:28 | call to escape | semmle.label | call to escape |
35+
| CodeInjection.rb:38:10:38:28 | call to escape | semmle.label | call to escape |
36+
| CodeInjection.rb:38:24:38:27 | code : | semmle.label | code : |
2437
| CodeInjection.rb:38:24:38:27 | code : | semmle.label | code : |
2538
| CodeInjection.rb:41:40:41:43 | code | semmle.label | code |
2639
| CodeInjection.rb:78:12:78:17 | call to params : | semmle.label | call to params : |
2740
| CodeInjection.rb:78:12:78:24 | ...[...] : | semmle.label | ...[...] : |
2841
| CodeInjection.rb:80:16:80:19 | code | semmle.label | code |
29-
| CodeInjection.rb:82:16:82:43 | ... + ... | semmle.label | ... + ... |
3042
subpaths
3143
#select
3244
| CodeInjection.rb:8:10:8:13 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:8:10:8:13 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
45+
| CodeInjection.rb:8:10:8:13 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:8:10:8:13 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
3346
| CodeInjection.rb:11:10:11:15 | call to params | CodeInjection.rb:11:10:11:15 | call to params | CodeInjection.rb:11:10:11:15 | call to params | This code execution depends on a $@. | CodeInjection.rb:11:10:11:15 | call to params | user-provided value |
47+
| CodeInjection.rb:11:10:11:15 | call to params | CodeInjection.rb:11:10:11:15 | call to params | CodeInjection.rb:11:10:11:15 | call to params | This code execution depends on a $@. | CodeInjection.rb:11:10:11:15 | call to params | user-provided value |
48+
| CodeInjection.rb:20:20:20:23 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:20:20:20:23 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
3449
| CodeInjection.rb:20:20:20:23 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:20:20:20:23 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
3550
| CodeInjection.rb:23:21:23:24 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:23:21:23:24 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
51+
| CodeInjection.rb:23:21:23:24 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:23:21:23:24 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
3652
| CodeInjection.rb:29:15:29:18 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:29:15:29:18 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
3753
| CodeInjection.rb:32:19:32:22 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:32:19:32:22 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
3854
| CodeInjection.rb:38:10:38:28 | call to escape | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:38:10:38:28 | call to escape | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
55+
| CodeInjection.rb:38:10:38:28 | call to escape | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:38:10:38:28 | call to escape | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
3956
| CodeInjection.rb:41:40:41:43 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:41:40:41:43 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
4057
| CodeInjection.rb:80:16:80:19 | code | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:80:16:80:19 | code | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |
41-
| CodeInjection.rb:82:16:82:43 | ... + ... | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:82:16:82:43 | ... + ... | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |

‎ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,8 @@ def create
7979

8080
obj().send(code, "foo"); # BAD
8181

82-
obj().send("prefix_" + code + "_suffix", "foo"); # GOOD - but still flagged by this query
82+
obj().send("prefix_" + code + "_suffix", "foo"); # GOOD
83+
84+
obj().send("prefix_#{code}_suffix", "foo"); # GOOD
8385
end
8486
end

0 commit comments

Comments
 (0)