Skip to content

Commit 36b3376

Browse files
erik-krogherik-krogh
committed
use allowImplicitRead instead of a taint-step from elements to the array
1 parent b0797a2 commit 36b3376

File tree

6 files changed

+23
-44
lines changed

6 files changed

+23
-44
lines changed

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

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1825,30 +1825,6 @@ module Array {
18251825
preservesValue = true
18261826
}
18271827
}
1828-
1829-
private import codeql.ruby.frameworks.core.Kernel
1830-
1831-
/**
1832-
* Holds if there an array element `pred` might taint the array defined by `succ`.
1833-
* This is used for queries where we consider an entire array to be tainted if any of its elements are tainted.
1834-
*/
1835-
predicate taintedArrayObjectSteps(DataFlow::Node pred, DataFlow::Node succ) {
1836-
exists(DataFlow::CallNode call |
1837-
call.getMethodName() = ["<<", "push", "append"] and
1838-
call.getReceiver() = succ and
1839-
pred = call.getArgument(0) and
1840-
call.getNumberOfArguments() = 1
1841-
)
1842-
or
1843-
exists(DataFlow::CallNode call |
1844-
call.asExpr().getExpr() = getAStaticArrayCall("[]")
1845-
or
1846-
call.(Kernel::KernelMethodCall).getMethodName() = "Array"
1847-
|
1848-
succ = call and
1849-
pred = call.getArgument(_)
1850-
)
1851-
}
18521828
}
18531829

18541830
/**

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ class Configuration extends TaintTracking::Configuration {
3232
result instanceof DataFlow::FeatureHasSourceCallContext
3333
}
3434

35-
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
36-
// if an array element gets tainted, then we treat the entire array as tainted
37-
Array::taintedArrayObjectSteps(pred, succ)
35+
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
36+
this.isSink(node) and
37+
c.isKnownOrUnknownElement(_)
3838
}
3939
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ class Configuration extends TaintTracking::Configuration {
3434
result instanceof DataFlow::FeatureHasSourceCallContext
3535
}
3636

37-
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
38-
// if an array element gets tainted, then we treat the entire array as tainted
39-
Array::taintedArrayObjectSteps(pred, succ)
37+
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
38+
this.isSink(node) and
39+
c.isKnownOrUnknownElement(_)
4040
}
4141
}

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +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:64:14:64:16 | arr |
16-
| impl/unsafeShell.rb:61:20:61:20 | x : | impl/unsafeShell.rb:68:14:68:16 | arr |
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] : |
1719
nodes
1820
| impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : |
1921
| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
@@ -41,6 +43,8 @@ nodes
4143
| impl/unsafeShell.rb:57:21:57:21 | x : | semmle.label | x : |
4244
| impl/unsafeShell.rb:58:23:58:23 | x | semmle.label | x |
4345
| 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 : |
4448
| impl/unsafeShell.rb:64:14:64:16 | arr | semmle.label | arr |
4549
| impl/unsafeShell.rb:68:14:68:16 | arr | semmle.label | arr |
4650
subpaths

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

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,14 @@ 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 |
11-
| impl/unsafeCode.rb:59:21:59:21 | x : | impl/unsafeCode.rb:61:10:61:12 | arr |
12-
| impl/unsafeCode.rb:59:21:59:21 | x : | impl/unsafeCode.rb:64:10:64:12 | arr |
1314
nodes
1415
| impl/unsafeCode.rb:2:12:2:17 | target : | semmle.label | target : |
1516
| impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} |
@@ -20,17 +21,17 @@ nodes
2021
| impl/unsafeCode.rb:28:17:28:22 | my_arr : | semmle.label | my_arr : |
2122
| impl/unsafeCode.rb:29:10:29:15 | my_arr | semmle.label | my_arr |
2223
| impl/unsafeCode.rb:32:21:32:21 | x : | semmle.label | x : |
24+
| impl/unsafeCode.rb:33:12:33:12 | x : | semmle.label | x : |
2325
| impl/unsafeCode.rb:34:10:34:12 | arr | semmle.label | arr |
2426
| impl/unsafeCode.rb:37:15:37:15 | x : | semmle.label | x : |
27+
| impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : | semmle.label | [post] arr [element] : |
28+
| impl/unsafeCode.rb:39:14:39:14 | x : | semmle.label | x : |
2529
| impl/unsafeCode.rb:40:10:40:12 | arr | semmle.label | arr |
2630
| impl/unsafeCode.rb:44:10:44:12 | arr | semmle.label | arr |
2731
| impl/unsafeCode.rb:47:15:47:15 | x : | semmle.label | x : |
2832
| impl/unsafeCode.rb:49:9:49:12 | #{...} | semmle.label | #{...} |
2933
| impl/unsafeCode.rb:54:21:54:21 | x : | semmle.label | x : |
3034
| impl/unsafeCode.rb:55:22:55:22 | x | semmle.label | x |
31-
| impl/unsafeCode.rb:59:21:59:21 | x : | semmle.label | x : |
32-
| impl/unsafeCode.rb:61:10:61:12 | arr | semmle.label | arr |
33-
| impl/unsafeCode.rb:64:10:64:12 | arr | semmle.label | arr |
3435
subpaths
3536
#select
3637
| 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 |
@@ -42,5 +43,3 @@ subpaths
4243
| 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 |
4344
| 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 |
4445
| 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 |
45-
| 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 |
46-
| impl/unsafeCode.rb:64:10:64:12 | arr | impl/unsafeCode.rb:59:21:59:21 | x : | impl/unsafeCode.rb:64:10:64:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:59:21:59:21 | x | library input | impl/unsafeCode.rb:64:5:64:24 | call to eval | interpreted as code |

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,9 +58,9 @@ def string_concat(x)
5858

5959
def join_indirect(x, y)
6060
arr = Array("foo = ", x)
61-
eval(arr.join(" ")) # NOT OK
61+
eval(arr.join(" ")) # NOT OK - but not currently flagged by the query
6262

6363
arr2 = [Array("foo = ", y).join(" ")]
64-
eval(arr.join("\n")) # NOT OK
64+
eval(arr2.join("\n")) # NOT OK - but not currently flagged by the query
6565
end
6666
end

0 commit comments

Comments
 (0)