Skip to content

Commit ef5132b

Browse files
erik-krogherik-krogh
authored
Merge pull request github#10883 from erik-krogh/codeSink
RB: don't flag code-injection for dynamic loading where an attacker only controls a substring
2 parents 2148e8b + bb8bcd4 commit ef5132b

File tree

11 files changed

+124
-9
lines changed

11 files changed

+124
-9
lines changed

‎ruby/ql/lib/codeql/ruby/Concepts.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -701,6 +701,9 @@ module SystemCommandExecution {
701701
class CodeExecution extends DataFlow::Node instanceof CodeExecution::Range {
702702
/** Gets the argument that specifies the code to be executed. */
703703
DataFlow::Node getCode() { result = super.getCode() }
704+
705+
/** Holds if this execution runs arbitrary code, as opposed to some restricted subset. E.g. `Object.send` will only run any method on an object. */
706+
predicate runsArbitraryCode() { super.runsArbitraryCode() }
704707
}
705708

706709
/** Provides a class for modeling new dynamic code execution APIs. */
@@ -714,6 +717,9 @@ module CodeExecution {
714717
abstract class Range extends DataFlow::Node {
715718
/** Gets the argument that specifies the code to be executed. */
716719
abstract DataFlow::Node getCode();
720+
721+
/** Holds if this execution runs arbitrary code, as opposed to some restricted subset. E.g. `Object.send` will only run any method on an object. */
722+
predicate runsArbitraryCode() { any() }
717723
}
718724
}
719725

‎ruby/ql/lib/codeql/ruby/frameworks/ActiveJob.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ module ActiveJob {
2525
}
2626

2727
override DataFlow::Node getCode() { result = this.getArgument(0) }
28+
29+
override predicate runsArbitraryCode() { none() }
2830
}
2931
}
3032
}

‎ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,5 +221,7 @@ module ActiveStorage {
221221
}
222222

223223
override DataFlow::Node getCode() { result = this.getArgument(0) }
224+
225+
override predicate runsArbitraryCode() { none() }
224226
}
225227
}

‎ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ module ActiveSupport {
3535
}
3636

3737
override DataFlow::Node getCode() { result = this.getReceiver() }
38+
39+
override predicate runsArbitraryCode() { none() }
3840
}
3941

4042
/**

‎ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,8 @@ module Kernel {
166166
SendCallCodeExecution() { this.getMethodName() = "send" }
167167

168168
override DataFlow::Node getCode() { result = this.getArgument(0) }
169+
170+
override predicate runsArbitraryCode() { none() }
169171
}
170172

171173
private class TapSummary extends SimpleSummarizedCallable {

‎ruby/ql/lib/codeql/ruby/frameworks/core/Module.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,5 +42,7 @@ module Module {
4242
}
4343

4444
override DataFlow::Node getCode() { result = this.getArgument(0) }
45+
46+
override predicate runsArbitraryCode() { none() }
4547
}
4648
}

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

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,20 +11,41 @@ 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.
2641
*/
27-
abstract class Sanitizer extends DataFlow::Node { }
42+
abstract class Sanitizer extends DataFlow::Node {
43+
/**
44+
* Gets a flow state for which this is a sanitizer.
45+
* Sanitizes all states if the result is empty.
46+
*/
47+
DataFlow::FlowState getAFlowState() { none() }
48+
}
2849

2950
/**
3051
* DEPRECATED: Use `Sanitizer` instead.
@@ -42,6 +63,35 @@ module CodeInjection {
4263
* A call that evaluates its arguments as Ruby code, considered as a flow sink.
4364
*/
4465
class CodeExecutionAsSink extends Sink {
45-
CodeExecutionAsSink() { this = any(CodeExecution c).getCode() }
66+
CodeExecution c;
67+
68+
CodeExecutionAsSink() { this = c.getCode() }
69+
70+
/** Gets a flow state for which this is a sink. */
71+
override DataFlow::FlowState getAFlowState() {
72+
if c.runsArbitraryCode()
73+
then result = [FlowState::substring(), FlowState::full()] // If it runs immediately, then it's always vulnerable.
74+
else result = FlowState::full() // If it "just" loads something, then it's only vulnerable if the attacker controls the entire string.
75+
}
76+
}
77+
78+
private import codeql.ruby.AST as Ast
79+
80+
/**
81+
* A string-concatenation that sanitizes the `full()` state.
82+
*/
83+
class StringConcatenationSanitizer extends Sanitizer {
84+
StringConcatenationSanitizer() {
85+
// string concatenations sanitize the `full` state, as an attacker no longer controls the entire string
86+
exists(Ast::AstNode str |
87+
str instanceof Ast::StringLiteral
88+
or
89+
str instanceof Ast::AddExpr
90+
|
91+
this.asExpr().getExpr() = str
92+
)
93+
}
94+
95+
override DataFlow::FlowState getAFlowState() { result = FlowState::full() }
4696
}
4797
}

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

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,26 @@ import codeql.ruby.dataflow.BarrierGuards
1616
class Configuration extends TaintTracking::Configuration {
1717
Configuration() { this = "CodeInjection" }
1818

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

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

2327
override predicate isSanitizer(DataFlow::Node node) {
24-
node instanceof Sanitizer or
25-
node instanceof StringConstCompareBarrier or
28+
node instanceof Sanitizer and not exists(node.(Sanitizer).getAFlowState())
29+
or
30+
node instanceof StringConstCompareBarrier
31+
or
2632
node instanceof StringConstArrayInclusionCallBarrier
2733
}
2834

35+
override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) {
36+
node.(Sanitizer).getAFlowState() = state
37+
}
38+
2939
deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
3040
guard instanceof SanitizerGuard
3141
}

‎ruby/ql/src/queries/security/cwe-094/CodeInjection.ql

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,13 @@ import DataFlow::PathGraph
2121
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Source sourceNode
2222
where
2323
config.hasFlowPath(source, sink) and
24-
sourceNode = source.getNode()
24+
sourceNode = source.getNode() and
25+
// removing duplications of the same path, but different flow-labels.
26+
sink =
27+
min(DataFlow::PathNode otherSink |
28+
config.hasFlowPath(any(DataFlow::PathNode s | s.getNode() = source.getNode()), otherSink)
29+
|
30+
otherSink order by otherSink.getState()
31+
)
2532
select sink.getNode(), source, sink, "This code execution depends on a $@.", source.getNode(),
2633
"user-provided value"

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,44 @@
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 |
17+
| CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:78:12:78:24 | ...[...] : |
18+
| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:80:16:80:19 | code |
1119
nodes
1220
| 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 : |
1322
| CodeInjection.rb:5:12:5:24 | ...[...] : | semmle.label | ...[...] : |
23+
| CodeInjection.rb:5:12:5:24 | ...[...] : | semmle.label | ...[...] : |
24+
| CodeInjection.rb:8:10:8:13 | code | semmle.label | code |
1425
| CodeInjection.rb:8:10:8:13 | code | semmle.label | code |
1526
| CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params |
27+
| CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params |
28+
| CodeInjection.rb:20:20:20:23 | code | semmle.label | code |
1629
| CodeInjection.rb:20:20:20:23 | code | semmle.label | code |
1730
| CodeInjection.rb:23:21:23:24 | code | semmle.label | code |
31+
| CodeInjection.rb:23:21:23:24 | code | semmle.label | code |
1832
| CodeInjection.rb:29:15:29:18 | code | semmle.label | code |
1933
| CodeInjection.rb:32:19:32:22 | code | semmle.label | code |
2034
| 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 : |
2137
| CodeInjection.rb:38:24:38:27 | code : | semmle.label | code : |
2238
| CodeInjection.rb:41:40:41:43 | code | semmle.label | code |
39+
| CodeInjection.rb:78:12:78:17 | call to params : | semmle.label | call to params : |
40+
| CodeInjection.rb:78:12:78:24 | ...[...] : | semmle.label | ...[...] : |
41+
| CodeInjection.rb:80:16:80:19 | code | semmle.label | code |
2342
subpaths
2443
#select
2544
| 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 |
@@ -30,3 +49,4 @@ subpaths
3049
| 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 |
3150
| 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 |
3251
| 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 |
52+
| 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 |

0 commit comments

Comments
 (0)