Skip to content

Commit af98ceb

Browse files
erik-krogherik-krogh
authored
Merge pull request github#11478 from erik-krogh/more-shell-taint
Rb: more taint-steps for shell-command-construction
2 parents 5f14af5 + cc3efcd commit af98ceb

File tree

9 files changed

+116
-17
lines changed

9 files changed

+116
-17
lines changed

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1186,6 +1186,16 @@ module Array {
11861186
}
11871187
}
11881188

1189+
private class JoinSummary extends SimpleSummarizedCallable {
1190+
JoinSummary() { this = ["join"] }
1191+
1192+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
1193+
input = "Argument[self].Element[any]" and
1194+
output = "ReturnValue" and
1195+
preservesValue = false
1196+
}
1197+
}
1198+
11891199
private class PushSummary extends SimpleSummarizedCallable {
11901200
// `append` is an alias for `push`
11911201
PushSummary() { this = ["push", "append"] }

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,4 +197,38 @@ module Kernel {
197197
super.getMethodName() = ["autoload", "autoload?"]
198198
}
199199
}
200+
201+
private import codeql.ruby.ast.internal.Module as Module
202+
203+
/**
204+
* A call to `Array()`, that converts it's singular argument to an array.
205+
* This summary is based on https://ruby-doc.org/3.2.1/Kernel.html#method-i-Array
206+
*/
207+
private class KernelArraySummary extends SummarizedCallable {
208+
KernelArraySummary() { this = "Array()" }
209+
210+
override MethodCall getACallSimple() {
211+
result.getMethodName() = "Array" and
212+
// I have to have a simplified "KernelMethodCall" implementation inlined here, because relying on `UnknownMethodCall` results in non-monotonic recursion (even if using `getACall`).
213+
(
214+
// similar to `getAStaticArrayCall` from Array.qll
215+
Module::resolveConstantReadAccess(result.getReceiver()) = Module::TResolved("Kernel")
216+
or
217+
result.getReceiver() instanceof SelfVariableAccess
218+
)
219+
}
220+
221+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
222+
(
223+
// already an array
224+
input = "Argument[0].WithElement[0..]" and
225+
output = "ReturnValue"
226+
or
227+
// not already an array
228+
input = "Argument[0].WithoutElement[0..]" and
229+
output = "ReturnValue.Element[0]"
230+
) and
231+
preservesValue = true
232+
}
233+
}
200234
}

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

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,19 +31,11 @@ class Configuration extends TaintTracking::Configuration {
3131
result instanceof DataFlow::FeatureHasSourceCallContext
3232
}
3333

34-
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
35-
// if an array element gets tainted, then we treat the entire array as tainted
36-
exists(DataFlow::CallNode call |
37-
call.getMethodName() = ["<<", "push", "append"] and
38-
call.getReceiver() = succ and
39-
pred = call.getArgument(0) and
40-
call.getNumberOfArguments() = 1
41-
)
42-
or
43-
exists(DataFlow::CallNode call |
44-
call.getMethodName() = "[]" and
45-
succ = call and
46-
pred = call.getArgument(_)
47-
)
34+
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
35+
// allow implicit reads of array elements
36+
this.isSink(node) and
37+
set.isKnownOrUnknownElement(any(DataFlow::Content::KnownElementContent content |
38+
content.getIndex().getValueType() = "int"
39+
))
4840
}
4941
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,4 +32,12 @@ class Configuration extends TaintTracking::Configuration {
3232
override DataFlow::FlowFeature getAFeature() {
3333
result instanceof DataFlow::FeatureHasSourceCallContext
3434
}
35+
36+
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
37+
// allow implicit reads of array elements
38+
this.isSink(node) and
39+
set.isKnownOrUnknownElement(any(DataFlow::Content::KnownElementContent content |
40+
content.getIndex().getValueType() = "int"
41+
))
42+
}
3543
}

‎ruby/ql/test/library-tests/frameworks/pathname/Pathname.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ pathnameInstances
6565
| file://:0:0:0:0 | parameter self of Pathname;Method[relative_path_from] |
6666
| file://:0:0:0:0 | parameter self of Pathname;Method[sub_ext] |
6767
| file://:0:0:0:0 | parameter self of Pathname;Method[to_path] |
68+
| file://:0:0:0:0 | parameter self of join |
6869
| file://:0:0:0:0 | parameter self of sub |
6970
| file://:0:0:0:0 | parameter self of to_s |
7071
fileSystemAccesses
@@ -141,6 +142,7 @@ fileNameSources
141142
| file://:0:0:0:0 | parameter self of Pathname;Method[relative_path_from] |
142143
| file://:0:0:0:0 | parameter self of Pathname;Method[sub_ext] |
143144
| file://:0:0:0:0 | parameter self of Pathname;Method[to_path] |
145+
| file://:0:0:0:0 | parameter self of join |
144146
| file://:0:0:0:0 | parameter self of sub |
145147
| file://:0:0:0:0 | parameter self of to_s |
146148
fileSystemReadAccesses

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ edges
1212
| impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:52:14:52:14 | x |
1313
| impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:54:29:54:29 | x |
1414
| impl/unsafeShell.rb:57:21:57:21 | x : | impl/unsafeShell.rb:58:23:58:23 | x |
15+
| impl/unsafeShell.rb:61:20:61:20 | x : | impl/unsafeShell.rb:63:14:63:14 | x : |
16+
| impl/unsafeShell.rb:63:5:63:7 | [post] arr [element] : | impl/unsafeShell.rb:64:14:64:16 | arr |
17+
| impl/unsafeShell.rb:63:5:63:7 | [post] arr [element] : | impl/unsafeShell.rb:68:14:68:16 | arr |
18+
| impl/unsafeShell.rb:63:14:63:14 | x : | impl/unsafeShell.rb:63:5:63:7 | [post] arr [element] : |
1519
nodes
1620
| impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : |
1721
| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
@@ -38,6 +42,11 @@ nodes
3842
| impl/unsafeShell.rb:54:29:54:29 | x | semmle.label | x |
3943
| impl/unsafeShell.rb:57:21:57:21 | x : | semmle.label | x : |
4044
| impl/unsafeShell.rb:58:23:58:23 | x | semmle.label | x |
45+
| impl/unsafeShell.rb:61:20:61:20 | x : | semmle.label | x : |
46+
| impl/unsafeShell.rb:63:5:63:7 | [post] arr [element] : | semmle.label | [post] arr [element] : |
47+
| impl/unsafeShell.rb:63:14:63:14 | x : | semmle.label | x : |
48+
| impl/unsafeShell.rb:64:14:64:16 | arr | semmle.label | arr |
49+
| impl/unsafeShell.rb:68:14:68:16 | arr | semmle.label | arr |
4150
subpaths
4251
#select
4352
| impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command |
@@ -53,3 +62,5 @@ subpaths
5362
| impl/unsafeShell.rb:52:14:52:24 | call to join | impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:52:14:52:14 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:52:5:52:30 | call to popen | shell command |
5463
| impl/unsafeShell.rb:54:14:54:40 | call to join | impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:54:29:54:29 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:54:5:54:46 | call to popen | shell command |
5564
| impl/unsafeShell.rb:58:14:58:23 | ... + ... | impl/unsafeShell.rb:57:21:57:21 | x : | impl/unsafeShell.rb:58:23:58:23 | x | This string concatenation which depends on $@ is later used in a $@. | impl/unsafeShell.rb:57:21:57:21 | x | library input | impl/unsafeShell.rb:58:5:58:29 | call to popen | shell command |
65+
| impl/unsafeShell.rb:64:14:64:26 | call to join | impl/unsafeShell.rb:61:20:61:20 | x : | impl/unsafeShell.rb:64:14:64:16 | arr | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:61:20:61:20 | x | library input | impl/unsafeShell.rb:64:5:64:32 | call to popen | shell command |
66+
| impl/unsafeShell.rb:68:14:68:26 | call to join | impl/unsafeShell.rb:61:20:61:20 | x : | impl/unsafeShell.rb:68:14:68:16 | arr | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:61:20:61:20 | x | library input | impl/unsafeShell.rb:68:5:68:32 | call to popen | shell command |

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,14 @@ def arrayJoin(x)
5757
def string_concat(x)
5858
IO.popen("cat " + x, "w") # NOT OK
5959
end
60+
61+
def array_taint (x, y)
62+
arr = ["cat"]
63+
arr.push(x)
64+
IO.popen(arr.join(' '), "w") # NOT OK
65+
66+
arr2 = ["cat"]
67+
arr2 << y
68+
IO.popen(arr.join(' '), "w") # NOT OK
69+
end
6070
end

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

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,21 @@ edges
33
| impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x |
44
| impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x |
55
| impl/unsafeCode.rb:28:17:28:22 | my_arr : | impl/unsafeCode.rb:29:10:29:15 | my_arr |
6-
| impl/unsafeCode.rb:32:21:32:21 | x : | impl/unsafeCode.rb:34:10:34:12 | arr |
7-
| impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:40:10:40:12 | arr |
8-
| impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:44:10:44:12 | arr |
6+
| impl/unsafeCode.rb:32:21:32:21 | x : | impl/unsafeCode.rb:33:12:33:12 | x : |
7+
| impl/unsafeCode.rb:33:12:33:12 | x : | impl/unsafeCode.rb:34:10:34:12 | arr |
8+
| impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:39:14:39:14 | x : |
9+
| impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : | impl/unsafeCode.rb:40:10:40:12 | arr |
10+
| impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : | impl/unsafeCode.rb:44:10:44:12 | arr |
11+
| impl/unsafeCode.rb:39:14:39:14 | x : | impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : |
912
| impl/unsafeCode.rb:47:15:47:15 | x : | impl/unsafeCode.rb:49:9:49:12 | #{...} |
1013
| impl/unsafeCode.rb:54:21:54:21 | x : | impl/unsafeCode.rb:55:22:55:22 | x |
14+
| impl/unsafeCode.rb:59:21:59:21 | x : | impl/unsafeCode.rb:60:17:60:17 | x : |
15+
| impl/unsafeCode.rb:59:24:59:24 | y : | impl/unsafeCode.rb:63:30:63:30 | y : |
16+
| impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] : | impl/unsafeCode.rb:61:10:61:12 | arr |
17+
| impl/unsafeCode.rb:60:17:60:17 | x : | impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] : |
18+
| impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] : | impl/unsafeCode.rb:63:13:63:42 | call to join : |
19+
| impl/unsafeCode.rb:63:13:63:42 | call to join : | impl/unsafeCode.rb:64:10:64:13 | arr2 |
20+
| impl/unsafeCode.rb:63:30:63:30 | y : | impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] : |
1121
nodes
1222
| impl/unsafeCode.rb:2:12:2:17 | target : | semmle.label | target : |
1323
| impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} |
@@ -18,14 +28,26 @@ nodes
1828
| impl/unsafeCode.rb:28:17:28:22 | my_arr : | semmle.label | my_arr : |
1929
| impl/unsafeCode.rb:29:10:29:15 | my_arr | semmle.label | my_arr |
2030
| impl/unsafeCode.rb:32:21:32:21 | x : | semmle.label | x : |
31+
| impl/unsafeCode.rb:33:12:33:12 | x : | semmle.label | x : |
2132
| impl/unsafeCode.rb:34:10:34:12 | arr | semmle.label | arr |
2233
| impl/unsafeCode.rb:37:15:37:15 | x : | semmle.label | x : |
34+
| impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : | semmle.label | [post] arr [element] : |
35+
| impl/unsafeCode.rb:39:14:39:14 | x : | semmle.label | x : |
2336
| impl/unsafeCode.rb:40:10:40:12 | arr | semmle.label | arr |
2437
| impl/unsafeCode.rb:44:10:44:12 | arr | semmle.label | arr |
2538
| impl/unsafeCode.rb:47:15:47:15 | x : | semmle.label | x : |
2639
| impl/unsafeCode.rb:49:9:49:12 | #{...} | semmle.label | #{...} |
2740
| impl/unsafeCode.rb:54:21:54:21 | x : | semmle.label | x : |
2841
| impl/unsafeCode.rb:55:22:55:22 | x | semmle.label | x |
42+
| impl/unsafeCode.rb:59:21:59:21 | x : | semmle.label | x : |
43+
| impl/unsafeCode.rb:59:24:59:24 | y : | semmle.label | y : |
44+
| impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] : | semmle.label | call to Array [element 0] : |
45+
| impl/unsafeCode.rb:60:17:60:17 | x : | semmle.label | x : |
46+
| impl/unsafeCode.rb:61:10:61:12 | arr | semmle.label | arr |
47+
| impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] : | semmle.label | call to Array [element 1] : |
48+
| impl/unsafeCode.rb:63:13:63:42 | call to join : | semmle.label | call to join : |
49+
| impl/unsafeCode.rb:63:30:63:30 | y : | semmle.label | y : |
50+
| impl/unsafeCode.rb:64:10:64:13 | arr2 | semmle.label | arr2 |
2951
subpaths
3052
#select
3153
| impl/unsafeCode.rb:3:17:3:25 | #{...} | impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:2:12:2:17 | target | library input | impl/unsafeCode.rb:3:5:3:27 | call to eval | interpreted as code |
@@ -37,3 +59,5 @@ subpaths
3759
| impl/unsafeCode.rb:44:10:44:12 | arr | impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:44:10:44:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:37:15:37:15 | x | library input | impl/unsafeCode.rb:44:5:44:24 | call to eval | interpreted as code |
3860
| impl/unsafeCode.rb:49:9:49:12 | #{...} | impl/unsafeCode.rb:47:15:47:15 | x : | impl/unsafeCode.rb:49:9:49:12 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:47:15:47:15 | x | library input | impl/unsafeCode.rb:51:5:51:13 | call to eval | interpreted as code |
3961
| impl/unsafeCode.rb:55:22:55:22 | x | impl/unsafeCode.rb:54:21:54:21 | x : | impl/unsafeCode.rb:55:22:55:22 | x | This string concatenation which depends on $@ is later $@. | impl/unsafeCode.rb:54:21:54:21 | x | library input | impl/unsafeCode.rb:56:5:56:13 | call to eval | interpreted as code |
62+
| impl/unsafeCode.rb:61:10:61:12 | arr | impl/unsafeCode.rb:59:21:59:21 | x : | impl/unsafeCode.rb:61:10:61:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:59:21:59:21 | x | library input | impl/unsafeCode.rb:61:5:61:23 | call to eval | interpreted as code |
63+
| impl/unsafeCode.rb:64:10:64:13 | arr2 | impl/unsafeCode.rb:59:24:59:24 | y : | impl/unsafeCode.rb:64:10:64:13 | arr2 | This array which depends on $@ is later $@. | impl/unsafeCode.rb:59:24:59:24 | y | library input | impl/unsafeCode.rb:64:5:64:25 | call to eval | interpreted as code |

‎ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,4 +55,12 @@ def string_concat(x)
5555
foo = "foo = " + x
5656
eval(foo) # NOT OK
5757
end
58+
59+
def join_indirect(x, y)
60+
arr = Array(x)
61+
eval(arr.join(" ")) # NOT OK
62+
63+
arr2 = [Array(["foo = ", y]).join(" ")]
64+
eval(arr2.join("\n")) # NOT OK
65+
end
5866
end

0 commit comments

Comments
 (0)