Skip to content

Commit 7784b9f

Browse files
RasmusWLRasmusWL
committed
Ruby: WIP: Make Argument[any] and any-named work
It's not fully working I think the problem is that the code below ties up `Argument[x]` with parameter positions, and `Parameter[x]` with argument positions. This flip might be correct for flow-summaries, but it does NOT seem to be correct for the `path` component in MaD. Specifically, quick-eval for ParameterPosition does NOT include `keyword key` while quick-eval for ArgumentPosition DOES include `keyword key`! For the test `Foo.sinkAnyNamedArg(key: tainted) # $ MISSING: hasValueFlow=tainted` https://github.com/github/codeql/blob/c8be8d30b3be15a54921a371e9b41f8c61dec87b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsSpecific.qll#L130-L133
1 parent df83a51 commit 7784b9f

File tree

5 files changed

+44
-8
lines changed

5 files changed

+44
-8
lines changed

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -796,6 +796,20 @@ module API {
796796
or
797797
pos.isBlock() and
798798
result = Label::blockParameter()
799+
or
800+
pos.isAny() and
801+
(
802+
result = Label::parameter(_)
803+
or
804+
result = Label::keywordParameter(_)
805+
or
806+
result = Label::blockParameter()
807+
// NOTE: `self` should NOT be included, as described in the QLDoc for `isAny()`
808+
)
809+
// TODO: needs handling of `self` ParameterPosition
810+
// or
811+
// pos.isSelf() and
812+
// ...
799813
}
800814
}
801815
}

‎ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,4 +281,10 @@ ParameterPosition parseArgBody(string s) {
281281
or
282282
s = "block" and
283283
result.isBlock()
284+
or
285+
s = "any" and
286+
result.isAny()
287+
or
288+
s = "any-named" and
289+
result.isKeyword(_)
284290
}

‎ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsSpecific.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ predicate isExtraValidTokenArgumentInIdentifyingAccessPath(string name, string a
181181
or
182182
name = ["Argument", "Parameter"] and
183183
(
184-
argument = ["self", "block"]
184+
argument = ["self", "block", "any", "any-named"]
185185
or
186186
argument.regexpMatch("\\w+:") // keyword argument
187187
)

‎ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,15 @@ edges
2121
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:51:24:51:30 | tainted : |
2222
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:54:22:54:28 | tainted : |
2323
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:55:17:55:23 | tainted : |
24+
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:57:27:57:33 | tainted : |
2425
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:61:32:61:38 | tainted : |
2526
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:63:23:63:29 | tainted : |
2627
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:93:16:93:22 | tainted : |
2728
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:99:14:99:20 | tainted : |
29+
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:102:16:102:22 | tainted |
30+
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:102:16:102:22 | tainted |
31+
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:103:21:103:27 | tainted |
32+
| summaries.rb:1:11:1:36 | call to identity : | summaries.rb:103:21:103:27 | tainted |
2833
| summaries.rb:1:20:1:36 | call to source : | summaries.rb:1:11:1:36 | call to identity : |
2934
| summaries.rb:1:20:1:36 | call to source : | summaries.rb:1:11:1:36 | call to identity : |
3035
| summaries.rb:4:12:7:3 | call to apply_block : | summaries.rb:9:6:9:13 | tainted2 |
@@ -59,6 +64,7 @@ edges
5964
| summaries.rb:51:24:51:30 | tainted : | summaries.rb:51:6:51:31 | call to namedArg |
6065
| summaries.rb:54:22:54:28 | tainted : | summaries.rb:54:6:54:29 | call to anyArg |
6166
| summaries.rb:55:17:55:23 | tainted : | summaries.rb:55:6:55:24 | call to anyArg |
67+
| summaries.rb:57:27:57:33 | tainted : | summaries.rb:57:6:57:34 | call to anyNamedArg |
6268
| summaries.rb:61:32:61:38 | tainted : | summaries.rb:61:6:61:39 | call to anyPositionFromOne |
6369
| summaries.rb:63:23:63:29 | tainted : | summaries.rb:63:40:63:40 | x : |
6470
| summaries.rb:63:40:63:40 | x : | summaries.rb:64:8:64:8 | x |
@@ -89,6 +95,8 @@ edges
8995
| summaries.rb:88:6:88:6 | a [element 2] : | summaries.rb:88:6:88:9 | ...[...] |
9096
| summaries.rb:88:6:88:6 | a [element 2] : | summaries.rb:88:6:88:9 | ...[...] |
9197
| summaries.rb:93:16:93:22 | [post] tainted : | summaries.rb:99:14:99:20 | tainted : |
98+
| summaries.rb:93:16:93:22 | [post] tainted : | summaries.rb:102:16:102:22 | tainted |
99+
| summaries.rb:93:16:93:22 | [post] tainted : | summaries.rb:103:21:103:27 | tainted |
92100
| summaries.rb:93:16:93:22 | tainted : | summaries.rb:93:16:93:22 | [post] tainted : |
93101
| summaries.rb:93:16:93:22 | tainted : | summaries.rb:93:25:93:25 | [post] y : |
94102
| summaries.rb:93:16:93:22 | tainted : | summaries.rb:93:33:93:33 | [post] z : |
@@ -156,6 +164,8 @@ nodes
156164
| summaries.rb:54:22:54:28 | tainted : | semmle.label | tainted : |
157165
| summaries.rb:55:6:55:24 | call to anyArg | semmle.label | call to anyArg |
158166
| summaries.rb:55:17:55:23 | tainted : | semmle.label | tainted : |
167+
| summaries.rb:57:6:57:34 | call to anyNamedArg | semmle.label | call to anyNamedArg |
168+
| summaries.rb:57:27:57:33 | tainted : | semmle.label | tainted : |
159169
| summaries.rb:61:6:61:39 | call to anyPositionFromOne | semmle.label | call to anyPositionFromOne |
160170
| summaries.rb:61:32:61:38 | tainted : | semmle.label | tainted : |
161171
| summaries.rb:63:23:63:29 | tainted : | semmle.label | tainted : |
@@ -202,9 +212,12 @@ nodes
202212
| summaries.rb:99:1:99:1 | [post] x : | semmle.label | [post] x : |
203213
| summaries.rb:99:14:99:20 | tainted : | semmle.label | tainted : |
204214
| summaries.rb:100:6:100:6 | x | semmle.label | x |
215+
| summaries.rb:102:16:102:22 | tainted | semmle.label | tainted |
216+
| summaries.rb:102:16:102:22 | tainted | semmle.label | tainted |
217+
| summaries.rb:103:21:103:27 | tainted | semmle.label | tainted |
218+
| summaries.rb:103:21:103:27 | tainted | semmle.label | tainted |
205219
subpaths
206220
invalidSpecComponent
207-
| ;;Member[Foo].Method[anyNamedArg] | Argument[any-named] | Argument[any-named] |
208221
#select
209222
| summaries.rb:2:6:2:12 | tainted | summaries.rb:1:20:1:36 | call to source : | summaries.rb:2:6:2:12 | tainted | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
210223
| summaries.rb:2:6:2:12 | tainted | summaries.rb:1:20:1:36 | call to source : | summaries.rb:2:6:2:12 | tainted | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
@@ -235,6 +248,7 @@ invalidSpecComponent
235248
| summaries.rb:51:6:51:31 | call to namedArg | summaries.rb:1:20:1:36 | call to source : | summaries.rb:51:6:51:31 | call to namedArg | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
236249
| summaries.rb:54:6:54:29 | call to anyArg | summaries.rb:1:20:1:36 | call to source : | summaries.rb:54:6:54:29 | call to anyArg | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
237250
| summaries.rb:55:6:55:24 | call to anyArg | summaries.rb:1:20:1:36 | call to source : | summaries.rb:55:6:55:24 | call to anyArg | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
251+
| summaries.rb:57:6:57:34 | call to anyNamedArg | summaries.rb:1:20:1:36 | call to source : | summaries.rb:57:6:57:34 | call to anyNamedArg | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
238252
| summaries.rb:61:6:61:39 | call to anyPositionFromOne | summaries.rb:1:20:1:36 | call to source : | summaries.rb:61:6:61:39 | call to anyPositionFromOne | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
239253
| summaries.rb:64:8:64:8 | x | summaries.rb:1:20:1:36 | call to source : | summaries.rb:64:8:64:8 | x | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
240254
| summaries.rb:71:8:71:54 | call to preserveTaint | summaries.rb:71:24:71:53 | call to source : | summaries.rb:71:8:71:54 | call to preserveTaint | $@ | summaries.rb:71:24:71:53 | call to source : | call to source : |
@@ -250,13 +264,15 @@ invalidSpecComponent
250264
| summaries.rb:95:6:95:6 | y | summaries.rb:1:20:1:36 | call to source : | summaries.rb:95:6:95:6 | y | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
251265
| summaries.rb:96:6:96:6 | z | summaries.rb:1:20:1:36 | call to source : | summaries.rb:96:6:96:6 | z | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
252266
| summaries.rb:100:6:100:6 | x | summaries.rb:1:20:1:36 | call to source : | summaries.rb:100:6:100:6 | x | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
267+
| summaries.rb:102:16:102:22 | tainted | summaries.rb:1:20:1:36 | call to source : | summaries.rb:102:16:102:22 | tainted | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
268+
| summaries.rb:102:16:102:22 | tainted | summaries.rb:1:20:1:36 | call to source : | summaries.rb:102:16:102:22 | tainted | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
269+
| summaries.rb:103:21:103:27 | tainted | summaries.rb:1:20:1:36 | call to source : | summaries.rb:103:21:103:27 | tainted | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
270+
| summaries.rb:103:21:103:27 | tainted | summaries.rb:1:20:1:36 | call to source : | summaries.rb:103:21:103:27 | tainted | $@ | summaries.rb:1:20:1:36 | call to source : | call to source : |
253271
warning
254272
| CSV type row should have 5 columns but has 2: test;TooFewColumns |
255273
| CSV type row should have 5 columns but has 8: test;TooManyColumns;;;Member[Foo].Instance;too;many;columns |
256274
| Invalid argument '0-1' in token 'Argument[0-1]' in access path: Method[foo].Argument[0-1] |
257275
| Invalid argument '*' in token 'Argument[*]' in access path: Method[foo].Argument[*] |
258-
| Invalid argument 'any' in token 'Argument[any]' in access path: Member[Foo].Method[sinkAnyArg].Argument[any] |
259-
| Invalid argument 'any-named' in token 'Argument[any-named]' in access path: Member[Foo].Method[sinkAnyNamedArg].Argument[any-named] |
260276
| Invalid token 'Argument' is missing its arguments, in access path: Method[foo].Argument |
261277
| Invalid token 'Member' is missing its arguments, in access path: Method[foo].Member |
262278
| Invalid token name 'Arg' in access path: Method[foo].Arg[0] |

‎ruby/ql/test/library-tests/dataflow/summaries/summaries.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ def userDefinedFunction(x, y)
5454
sink(Foo.anyArg(foo: tainted)) # $ hasTaintFlow=tainted
5555
sink(Foo.anyArg(tainted)) # $ hasTaintFlow=tainted
5656

57-
sink(Foo.anyNamedArg(foo: tainted)) # $ MISSING: hasTaintFlow=tainted
57+
sink(Foo.anyNamedArg(foo: tainted)) # $ hasTaintFlow=tainted
5858
sink(Foo.anyNamedArg(tainted))
5959

6060
sink(Foo.anyPositionFromOne(tainted))
@@ -99,8 +99,8 @@ def userDefinedFunction(x, y)
9999
x.flowToSelf(tainted)
100100
sink(x) # $ hasTaintFlow=tainted
101101

102-
Foo.sinkAnyArg(tainted) # $ MISSING: hasTaintFlow=tainted
103-
Foo.sinkAnyArg(key: tainted) # $ MISSING: hasTaintFlow=tainted
102+
Foo.sinkAnyArg(tainted) # $ hasValueFlow=tainted
103+
Foo.sinkAnyArg(key: tainted) # $ hasValueFlow=tainted
104104

105105
Foo.sinkAnyNamedArg(tainted)
106-
Foo.sinkAnyNamedArg(key: tainted) # $ MISSING: hasTaintFlow=tainted
106+
Foo.sinkAnyNamedArg(key: tainted) # $ MISSING: hasValueFlow=tainted

0 commit comments

Comments
 (0)