Skip to content

Commit 75cb0ef

Browse files
hmachmac
authored
Merge pull request #10538 from hmac/hmac/actioncontroller-parameters
Ruby: Model flow through ActionController::Parameters
2 parents 5f740a5 + eaf6eb0 commit 75cb0ef

File tree

6 files changed

+584
-0
lines changed

6 files changed

+584
-0
lines changed

‎ruby/ql/lib/change-notes/2022-09-27-actioncontroller-parameters.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Taint flow through `ActionController::Parameters` is tracked more accurately.

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

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,3 +382,123 @@ private class SendFile extends FileSystemAccess::Range, DataFlow::CallNode {
382382

383383
override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
384384
}
385+
386+
private module ParamsSummaries {
387+
private import codeql.ruby.dataflow.FlowSummary
388+
389+
/**
390+
* An instance of `ActionController::Parameters`, including those returned
391+
* from method calls on other instances.
392+
*/
393+
private class ParamsInstance extends DataFlow::Node {
394+
ParamsInstance() {
395+
this.asExpr().getExpr() instanceof ParamsCall
396+
or
397+
this =
398+
any(DataFlow::CallNode call |
399+
call.getReceiver() instanceof ParamsInstance and
400+
call.getMethodName() = paramsMethodReturningParamsInstance()
401+
)
402+
or
403+
exists(ParamsInstance prev | prev.(DataFlow::LocalSourceNode).flowsTo(this))
404+
}
405+
}
406+
407+
/**
408+
* Methods on `ActionController::Parameters` that return an instance of
409+
* `ActionController::Parameters`.
410+
*/
411+
string paramsMethodReturningParamsInstance() {
412+
result =
413+
[
414+
"concat", "concat!", "compact_blank", "deep_dup", "deep_transform_keys", "delete_if",
415+
// dig doesn't always return a Parameters instance, but it will if the
416+
// given key refers to a nested hash parameter.
417+
"dig", "each", "each_key", "each_pair", "each_value", "except", "keep_if", "merge",
418+
"merge!", "permit", "reject", "reject!", "reverse_merge", "reverse_merge!", "select",
419+
"select!", "slice", "slice!", "transform_keys", "transform_keys!", "transform_values",
420+
"transform_values!", "with_defaults", "with_defaults!"
421+
]
422+
}
423+
424+
/**
425+
* Methods on `ActionController::Parameters` that propagate taint from
426+
* receiver to return value.
427+
*/
428+
string methodReturnsTaintFromSelf() {
429+
result =
430+
[
431+
"as_json", "permit", "require", "required", "deep_dup", "deep_transform_keys",
432+
"deep_transform_keys!", "delete_if", "extract!", "keep_if", "select", "select!", "reject",
433+
"reject!", "to_h", "to_hash", "to_query", "to_param", "to_unsafe_h", "to_unsafe_hash",
434+
"transform_keys", "transform_keys!", "transform_values", "transform_values!", "values_at"
435+
]
436+
}
437+
438+
/**
439+
* A flow summary for methods on `ActionController::Parameters` which
440+
* propagate taint from receiver to return value.
441+
*/
442+
private class MethodsReturningParamsInstanceSummary extends SummarizedCallable {
443+
MethodsReturningParamsInstanceSummary() { this = "ActionController::Parameters#<various>" }
444+
445+
override MethodCall getACall() {
446+
any(ParamsInstance i).asExpr().getExpr() = result.getReceiver() and
447+
result.getMethodName() = methodReturnsTaintFromSelf()
448+
}
449+
450+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
451+
input = "Argument[self]" and
452+
output = "ReturnValue" and
453+
preservesValue = false
454+
}
455+
}
456+
457+
/**
458+
* `#merge`
459+
* Returns a new ActionController::Parameters with all keys from other_hash merged into current hash.
460+
* `#reverse_merge`
461+
* `#with_defaults`
462+
* Returns a new ActionController::Parameters with all keys from current hash merged into other_hash.
463+
*/
464+
private class MergeSummary extends SummarizedCallable {
465+
MergeSummary() { this = "ActionController::Parameters#merge" }
466+
467+
override MethodCall getACall() {
468+
result.getMethodName() = ["merge", "reverse_merge", "with_defaults"] and
469+
exists(ParamsInstance i |
470+
i.asExpr().getExpr() = [result.getReceiver(), result.getArgument(0)]
471+
)
472+
}
473+
474+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
475+
input = ["Argument[self]", "Argument[0]"] and
476+
output = "ReturnValue" and
477+
preservesValue = false
478+
}
479+
}
480+
481+
/**
482+
* `#merge!`
483+
* Returns current ActionController::Parameters instance with current hash merged into other_hash.
484+
* `#reverse_merge!`
485+
* `#with_defaults!`
486+
* Returns a new ActionController::Parameters with all keys from current hash merged into other_hash.
487+
*/
488+
private class MergeBangSummary extends SummarizedCallable {
489+
MergeBangSummary() { this = "ActionController::Parameters#merge!" }
490+
491+
override MethodCall getACall() {
492+
result.getMethodName() = ["merge!", "reverse_merge!", "with_defaults!"] and
493+
exists(ParamsInstance i |
494+
i.asExpr().getExpr() = [result.getReceiver(), result.getArgument(0)]
495+
)
496+
}
497+
498+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
499+
input = ["Argument[self]", "Argument[0]"] and
500+
output = ["ReturnValue", "Argument[self]"] and
501+
preservesValue = false
502+
}
503+
}
504+
}

‎ruby/ql/test/library-tests/frameworks/ActionController.expected

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
actionControllerControllerClasses
2+
| action_controller/params_flow.rb:1:1:151:3 | MyController |
23
| active_record/ActiveRecord.rb:23:1:39:3 | FooController |
34
| active_record/ActiveRecord.rb:41:1:64:3 | BarController |
45
| active_record/ActiveRecord.rb:66:1:98:3 | BazController |
@@ -11,6 +12,39 @@ actionControllerControllerClasses
1112
| app/controllers/tags_controller.rb:1:1:2:3 | TagsController |
1213
| app/controllers/users/notifications_controller.rb:2:3:5:5 | NotificationsController |
1314
actionControllerActionMethods
15+
| action_controller/params_flow.rb:2:3:4:5 | m1 |
16+
| action_controller/params_flow.rb:6:3:8:5 | m2 |
17+
| action_controller/params_flow.rb:10:3:12:5 | m2 |
18+
| action_controller/params_flow.rb:14:3:16:5 | m3 |
19+
| action_controller/params_flow.rb:18:3:20:5 | m4 |
20+
| action_controller/params_flow.rb:22:3:24:5 | m5 |
21+
| action_controller/params_flow.rb:26:3:28:5 | m6 |
22+
| action_controller/params_flow.rb:30:3:32:5 | m7 |
23+
| action_controller/params_flow.rb:34:3:36:5 | m8 |
24+
| action_controller/params_flow.rb:38:3:40:5 | m9 |
25+
| action_controller/params_flow.rb:42:3:44:5 | m10 |
26+
| action_controller/params_flow.rb:46:3:48:5 | m11 |
27+
| action_controller/params_flow.rb:50:3:52:5 | m12 |
28+
| action_controller/params_flow.rb:54:3:56:5 | m13 |
29+
| action_controller/params_flow.rb:58:3:60:5 | m14 |
30+
| action_controller/params_flow.rb:62:3:64:5 | m15 |
31+
| action_controller/params_flow.rb:66:3:68:5 | m16 |
32+
| action_controller/params_flow.rb:70:3:72:5 | m17 |
33+
| action_controller/params_flow.rb:74:3:76:5 | m18 |
34+
| action_controller/params_flow.rb:78:3:80:5 | m19 |
35+
| action_controller/params_flow.rb:82:3:84:5 | m20 |
36+
| action_controller/params_flow.rb:86:3:88:5 | m21 |
37+
| action_controller/params_flow.rb:90:3:92:5 | m22 |
38+
| action_controller/params_flow.rb:94:3:96:5 | m23 |
39+
| action_controller/params_flow.rb:98:3:100:5 | m24 |
40+
| action_controller/params_flow.rb:102:3:104:5 | m25 |
41+
| action_controller/params_flow.rb:106:3:108:5 | m26 |
42+
| action_controller/params_flow.rb:110:3:113:5 | m27 |
43+
| action_controller/params_flow.rb:115:3:118:5 | m28 |
44+
| action_controller/params_flow.rb:120:3:123:5 | m29 |
45+
| action_controller/params_flow.rb:125:3:132:5 | m30 |
46+
| action_controller/params_flow.rb:134:3:141:5 | m31 |
47+
| action_controller/params_flow.rb:143:3:150:5 | m32 |
1448
| active_record/ActiveRecord.rb:27:3:38:5 | some_request_handler |
1549
| active_record/ActiveRecord.rb:42:3:47:5 | some_other_request_handler |
1650
| active_record/ActiveRecord.rb:49:3:63:5 | safe_paths |
@@ -39,6 +73,48 @@ actionControllerActionMethods
3973
| app/controllers/posts_controller.rb:8:3:9:5 | upvote |
4074
| app/controllers/users/notifications_controller.rb:3:5:4:7 | mark_as_read |
4175
paramsCalls
76+
| action_controller/params_flow.rb:3:10:3:15 | call to params |
77+
| action_controller/params_flow.rb:7:10:7:15 | call to params |
78+
| action_controller/params_flow.rb:11:10:11:15 | call to params |
79+
| action_controller/params_flow.rb:15:10:15:15 | call to params |
80+
| action_controller/params_flow.rb:19:10:19:15 | call to params |
81+
| action_controller/params_flow.rb:23:10:23:15 | call to params |
82+
| action_controller/params_flow.rb:27:10:27:15 | call to params |
83+
| action_controller/params_flow.rb:31:10:31:15 | call to params |
84+
| action_controller/params_flow.rb:35:10:35:15 | call to params |
85+
| action_controller/params_flow.rb:39:10:39:15 | call to params |
86+
| action_controller/params_flow.rb:43:10:43:15 | call to params |
87+
| action_controller/params_flow.rb:47:10:47:15 | call to params |
88+
| action_controller/params_flow.rb:51:10:51:15 | call to params |
89+
| action_controller/params_flow.rb:55:10:55:15 | call to params |
90+
| action_controller/params_flow.rb:59:10:59:15 | call to params |
91+
| action_controller/params_flow.rb:63:10:63:15 | call to params |
92+
| action_controller/params_flow.rb:67:10:67:15 | call to params |
93+
| action_controller/params_flow.rb:71:10:71:15 | call to params |
94+
| action_controller/params_flow.rb:75:10:75:15 | call to params |
95+
| action_controller/params_flow.rb:79:10:79:15 | call to params |
96+
| action_controller/params_flow.rb:83:10:83:15 | call to params |
97+
| action_controller/params_flow.rb:87:10:87:15 | call to params |
98+
| action_controller/params_flow.rb:91:10:91:15 | call to params |
99+
| action_controller/params_flow.rb:95:10:95:15 | call to params |
100+
| action_controller/params_flow.rb:99:10:99:15 | call to params |
101+
| action_controller/params_flow.rb:103:10:103:15 | call to params |
102+
| action_controller/params_flow.rb:107:10:107:15 | call to params |
103+
| action_controller/params_flow.rb:111:10:111:15 | call to params |
104+
| action_controller/params_flow.rb:112:23:112:28 | call to params |
105+
| action_controller/params_flow.rb:116:10:116:15 | call to params |
106+
| action_controller/params_flow.rb:117:31:117:36 | call to params |
107+
| action_controller/params_flow.rb:121:10:121:15 | call to params |
108+
| action_controller/params_flow.rb:122:31:122:36 | call to params |
109+
| action_controller/params_flow.rb:126:10:126:15 | call to params |
110+
| action_controller/params_flow.rb:127:24:127:29 | call to params |
111+
| action_controller/params_flow.rb:130:14:130:19 | call to params |
112+
| action_controller/params_flow.rb:135:10:135:15 | call to params |
113+
| action_controller/params_flow.rb:136:32:136:37 | call to params |
114+
| action_controller/params_flow.rb:139:22:139:27 | call to params |
115+
| action_controller/params_flow.rb:144:10:144:15 | call to params |
116+
| action_controller/params_flow.rb:145:32:145:37 | call to params |
117+
| action_controller/params_flow.rb:148:22:148:27 | call to params |
42118
| active_record/ActiveRecord.rb:28:30:28:35 | call to params |
43119
| active_record/ActiveRecord.rb:29:29:29:34 | call to params |
44120
| active_record/ActiveRecord.rb:30:31:30:36 | call to params |
@@ -71,6 +147,48 @@ paramsCalls
71147
| app/controllers/foo/bars_controller.rb:22:10:22:15 | call to params |
72148
| app/views/foo/bars/show.html.erb:5:9:5:14 | call to params |
73149
paramsSources
150+
| action_controller/params_flow.rb:3:10:3:15 | call to params |
151+
| action_controller/params_flow.rb:7:10:7:15 | call to params |
152+
| action_controller/params_flow.rb:11:10:11:15 | call to params |
153+
| action_controller/params_flow.rb:15:10:15:15 | call to params |
154+
| action_controller/params_flow.rb:19:10:19:15 | call to params |
155+
| action_controller/params_flow.rb:23:10:23:15 | call to params |
156+
| action_controller/params_flow.rb:27:10:27:15 | call to params |
157+
| action_controller/params_flow.rb:31:10:31:15 | call to params |
158+
| action_controller/params_flow.rb:35:10:35:15 | call to params |
159+
| action_controller/params_flow.rb:39:10:39:15 | call to params |
160+
| action_controller/params_flow.rb:43:10:43:15 | call to params |
161+
| action_controller/params_flow.rb:47:10:47:15 | call to params |
162+
| action_controller/params_flow.rb:51:10:51:15 | call to params |
163+
| action_controller/params_flow.rb:55:10:55:15 | call to params |
164+
| action_controller/params_flow.rb:59:10:59:15 | call to params |
165+
| action_controller/params_flow.rb:63:10:63:15 | call to params |
166+
| action_controller/params_flow.rb:67:10:67:15 | call to params |
167+
| action_controller/params_flow.rb:71:10:71:15 | call to params |
168+
| action_controller/params_flow.rb:75:10:75:15 | call to params |
169+
| action_controller/params_flow.rb:79:10:79:15 | call to params |
170+
| action_controller/params_flow.rb:83:10:83:15 | call to params |
171+
| action_controller/params_flow.rb:87:10:87:15 | call to params |
172+
| action_controller/params_flow.rb:91:10:91:15 | call to params |
173+
| action_controller/params_flow.rb:95:10:95:15 | call to params |
174+
| action_controller/params_flow.rb:99:10:99:15 | call to params |
175+
| action_controller/params_flow.rb:103:10:103:15 | call to params |
176+
| action_controller/params_flow.rb:107:10:107:15 | call to params |
177+
| action_controller/params_flow.rb:111:10:111:15 | call to params |
178+
| action_controller/params_flow.rb:112:23:112:28 | call to params |
179+
| action_controller/params_flow.rb:116:10:116:15 | call to params |
180+
| action_controller/params_flow.rb:117:31:117:36 | call to params |
181+
| action_controller/params_flow.rb:121:10:121:15 | call to params |
182+
| action_controller/params_flow.rb:122:31:122:36 | call to params |
183+
| action_controller/params_flow.rb:126:10:126:15 | call to params |
184+
| action_controller/params_flow.rb:127:24:127:29 | call to params |
185+
| action_controller/params_flow.rb:130:14:130:19 | call to params |
186+
| action_controller/params_flow.rb:135:10:135:15 | call to params |
187+
| action_controller/params_flow.rb:136:32:136:37 | call to params |
188+
| action_controller/params_flow.rb:139:22:139:27 | call to params |
189+
| action_controller/params_flow.rb:144:10:144:15 | call to params |
190+
| action_controller/params_flow.rb:145:32:145:37 | call to params |
191+
| action_controller/params_flow.rb:148:22:148:27 | call to params |
74192
| active_record/ActiveRecord.rb:28:30:28:35 | call to params |
75193
| active_record/ActiveRecord.rb:29:29:29:34 | call to params |
76194
| active_record/ActiveRecord.rb:30:31:30:36 | call to params |

0 commit comments

Comments
 (0)