Skip to content

Commit 16fa8b2

Browse files
hvitvedhvitved
authored
Merge pull request github#12051 from hmac/actioncontroller-filter-flow-steps
Ruby: flow steps for ActionController filters
2 parents 0dc6ada + 92359e5 commit 16fa8b2

File tree

11 files changed

+495
-21
lines changed

11 files changed

+495
-21
lines changed

‎ruby/ql/lib/change-notes/2023-02-13-actioncontroller-filters.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+
* Flow is now tracked between ActionController `before_filter` and `after_filter` callbacks and their associated action methods.

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,6 +1157,8 @@ predicate jumpStep(Node pred, Node succ) {
11571157
succ.asExpr().getExpr().(ConstantReadAccess).getValue() = pred.asExpr().getExpr()
11581158
or
11591159
FlowSummaryImpl::Private::Steps::summaryJumpStep(pred, succ)
1160+
or
1161+
any(AdditionalJumpStep s).step(pred, succ)
11601162
}
11611163

11621164
private ContentSet getKeywordContent(string name) {
@@ -1484,3 +1486,15 @@ ContentApprox getContentApprox(Content c) {
14841486
or
14851487
result = TNonElementContentApprox(c)
14861488
}
1489+
1490+
/**
1491+
* A unit class for adding additional jump steps.
1492+
*
1493+
* Extend this class to add additional jump steps.
1494+
*/
1495+
class AdditionalJumpStep extends Unit {
1496+
/**
1497+
* Holds if data can flow from `pred` to `succ` in a way that discards call contexts.
1498+
*/
1499+
abstract predicate step(Node pred, Node succ);
1500+
}

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,13 @@ private module RenderCallUtils {
2929
result = getTemplatePathValue(renderCall).regexpCapture("^/?(.*/)?(?:[^/]*?)$", 1)
3030
}
3131

32-
// everything after the final slash, or the whole string if there is no slash
33-
private string getBaseName(MethodCall renderCall) {
34-
result = getTemplatePathValue(renderCall).regexpCapture("^/?(?:.*/)?([^/]*?)$", 1)
35-
}
36-
3732
/**
3833
* Gets the template file to be rendered by this render call, if any.
3934
*/
4035
ErbFile getTemplateFile(MethodCall renderCall) {
41-
result.getTemplateName() = getBaseName(renderCall) and
36+
// everything after the final slash, or the whole string if there is no slash
37+
result.getTemplateName() =
38+
getTemplatePathValue(renderCall).regexpCapture("^/?(?:.*/)?([^/]*?)$", 1) and
4239
result.getRelativePath().matches("%app/views/" + getSubPath(renderCall) + "%")
4340
}
4441

‎ruby/ql/lib/codeql/ruby/frameworks/actioncontroller/Filters.qll‎

Lines changed: 82 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ private import codeql.ruby.controlflow.CfgNodes::ExprNodes
99
private import codeql.ruby.DataFlow
1010
private import codeql.ruby.dataflow.internal.DataFlowPrivate as DataFlowPrivate
1111
private import codeql.ruby.ast.internal.Constant
12+
private import codeql.ruby.ast.internal.Module
1213

1314
/**
1415
* Provides modeling for ActionController filters.
@@ -34,6 +35,17 @@ module Filters {
3435
}
3536
}
3637

38+
bindingset[call]
39+
pragma[inline_late]
40+
private ActionControllerActionMethod getADescendentAction(MethodCallCfgNode call) {
41+
result = call.getExpr().getEnclosingModule().getAMethod()
42+
or
43+
exists(ModuleBase m |
44+
m.getModule() = call.getExpr().getEnclosingModule().getModule().getAnImmediateDescendent+() and
45+
result = m.getAMethod()
46+
)
47+
}
48+
3749
/**
3850
* A call to a class method that adds or removes a filter from the callback chain.
3951
* This class exists to encapsulate common behavior between calls that
@@ -64,14 +76,7 @@ module Filters {
6476
not exists(this.getOnlyArgument()) and
6577
forall(string except | except = this.getExceptArgument() | result.getName() != except)
6678
) and
67-
(
68-
result = this.getExpr().getEnclosingModule().getAMethod()
69-
or
70-
exists(ModuleBase m |
71-
m.getModule() = this.getExpr().getEnclosingModule().getModule().getADescendent() and
72-
result = m.getAMethod()
73-
)
74-
)
79+
result = getADescendentAction(this)
7580
}
7681

7782
private string getOnlyArgument() {
@@ -104,8 +109,12 @@ module Filters {
104109

105110
StringlikeLiteralCfgNode getFilterArgument() { result = this.getPositionalArgument(_) }
106111

112+
string getFilterArgumentName() {
113+
result = this.getFilterArgument().getConstantValue().getStringlikeValue()
114+
}
115+
107116
/**
108-
* Gets the callable that implements the filter with name `name`.
117+
* Gets the callable that implements a filter registered by this call.
109118
* This currently only finds methods in the local class or superclass.
110119
* It doesn't handle:
111120
* - lambdas
@@ -122,10 +131,9 @@ module Filters {
122131
* end
123132
* ```
124133
*/
125-
Callable getFilterCallable(string name) {
126-
result.(MethodBase).getName() = name and
127-
result.getEnclosingModule().getModule() =
128-
this.getExpr().getEnclosingModule().getModule().getAnAncestor()
134+
Callable getAFilterCallable() {
135+
result =
136+
lookupMethod(this.getExpr().getEnclosingModule().getModule(), this.getFilterArgumentName())
129137
}
130138
}
131139

@@ -321,7 +329,9 @@ module Filters {
321329

322330
string getFilterName() { result = this.getConstantValue().getStringlikeValue() }
323331

324-
Callable getFilterCallable() { result = call.getFilterCallable(this.getFilterName()) }
332+
Callable getFilterCallable() {
333+
result = call.getAFilterCallable() and result.(MethodBase).getName() = this.getFilterName()
334+
}
325335

326336
ActionControllerActionMethod getAnAction() { result = call.getAnAction() }
327337
}
@@ -387,4 +397,62 @@ module Filters {
387397
* `pred` and `succ` may be methods bound to callbacks or controller actions.
388398
*/
389399
predicate next(Method pred, Method succ) { next(_, pred, succ) }
400+
401+
/**
402+
* Holds if `n` is a post-update node for `self` in method `m`.
403+
*/
404+
private predicate selfPostUpdate(DataFlow::PostUpdateNode n, Method m) {
405+
n.getPreUpdateNode().asExpr().getExpr() =
406+
any(SelfVariableAccess self |
407+
pragma[only_bind_into](m) = self.getEnclosingCallable() and
408+
self.getVariable().getDeclaringScope() = m
409+
)
410+
}
411+
412+
/**
413+
* Holds if `n` is the self parameter of method `m`.
414+
*/
415+
private predicate selfParameter(DataFlowPrivate::SelfParameterNode n, Method m) {
416+
m = n.getMethod()
417+
}
418+
419+
/**
420+
* A class defining additional jump steps arising from filters.
421+
*/
422+
class FilterJumpStep extends DataFlowPrivate::AdditionalJumpStep {
423+
/**
424+
* Holds if data can flow from `pred` to `succ` via a callback chain.
425+
* `pred` is the post-update node of the self parameter in a method, and
426+
* `succ` is the self parameter of a subsequent method that is executed as
427+
* part of the callback chain.
428+
*/
429+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
430+
exists(Method predMethod, Method succMethod | next(predMethod, succMethod) |
431+
// Flow from a post-update node of self in `pred` to the self parameter of `succ`
432+
//
433+
// def a
434+
// foo() ---------+
435+
// @x = 1 ---+ |
436+
// end | |
437+
// | |
438+
// def b <----+----+
439+
// ...
440+
//
441+
selfPostUpdate(pred, predMethod) and
442+
selfParameter(succ, succMethod)
443+
or
444+
// Flow from the self parameter of `pred` to the self parameter of `succ`
445+
//
446+
// def a ---+
447+
// ... |
448+
// end |
449+
// |
450+
// def b <-+
451+
// ...
452+
//
453+
selfParameter(pred, predMethod) and
454+
selfParameter(succ, succMethod)
455+
)
456+
}
457+
}
390458
}

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ actionControllerControllerClasses
66
| controllers/posts_controller.rb:1:1:32:3 | PostsController |
77
| controllers/tags_controller.rb:1:1:2:3 | TagsController |
88
| controllers/users/notifications_controller.rb:2:3:5:5 | Users::NotificationsController |
9+
| filter_flow.rb:9:1:23:3 | OneController |
10+
| filter_flow.rb:25:1:40:3 | TwoController |
11+
| filter_flow.rb:42:1:57:3 | ThreeController |
12+
| filter_flow.rb:59:1:73:3 | FourController |
13+
| filter_flow.rb:75:1:93:3 | FiveController |
914
| input_access.rb:1:1:50:3 | UsersController |
1015
| params_flow.rb:1:1:162:3 | MyController |
1116
| params_flow.rb:170:1:178:3 | Subclass |
@@ -27,6 +32,22 @@ actionControllerActionMethods
2732
| controllers/posts_controller.rb:17:3:18:5 | show |
2833
| controllers/posts_controller.rb:20:3:21:5 | upvote |
2934
| controllers/users/notifications_controller.rb:3:5:4:7 | mark_as_read |
35+
| filter_flow.rb:13:3:15:5 | a |
36+
| filter_flow.rb:17:3:18:5 | b |
37+
| filter_flow.rb:20:3:22:5 | c |
38+
| filter_flow.rb:29:3:31:5 | a |
39+
| filter_flow.rb:33:3:35:5 | b |
40+
| filter_flow.rb:37:3:39:5 | c |
41+
| filter_flow.rb:46:3:49:5 | a |
42+
| filter_flow.rb:51:3:52:5 | b |
43+
| filter_flow.rb:54:3:56:5 | c |
44+
| filter_flow.rb:63:3:65:5 | a |
45+
| filter_flow.rb:67:3:68:5 | b |
46+
| filter_flow.rb:70:3:72:5 | c |
47+
| filter_flow.rb:79:3:81:5 | a |
48+
| filter_flow.rb:83:3:84:5 | b |
49+
| filter_flow.rb:86:3:88:5 | c |
50+
| filter_flow.rb:90:3:92:5 | taint_foo |
3051
| input_access.rb:2:3:49:5 | index |
3152
| logging.rb:2:5:8:7 | index |
3253
| params_flow.rb:2:3:4:5 | m1 |
@@ -72,6 +93,11 @@ paramsCalls
7293
| controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
7394
| controllers/foo/bars_controller.rb:22:10:22:15 | call to params |
7495
| controllers/posts_controller.rb:26:23:26:28 | call to params |
96+
| filter_flow.rb:14:12:14:17 | call to params |
97+
| filter_flow.rb:30:12:30:17 | call to params |
98+
| filter_flow.rb:47:12:47:17 | call to params |
99+
| filter_flow.rb:64:16:64:21 | call to params |
100+
| filter_flow.rb:91:12:91:17 | call to params |
75101
| params_flow.rb:3:10:3:15 | call to params |
76102
| params_flow.rb:7:10:7:15 | call to params |
77103
| params_flow.rb:11:10:11:15 | call to params |
@@ -127,6 +153,11 @@ paramsSources
127153
| controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
128154
| controllers/foo/bars_controller.rb:22:10:22:15 | call to params |
129155
| controllers/posts_controller.rb:26:23:26:28 | call to params |
156+
| filter_flow.rb:14:12:14:17 | call to params |
157+
| filter_flow.rb:30:12:30:17 | call to params |
158+
| filter_flow.rb:47:12:47:17 | call to params |
159+
| filter_flow.rb:64:16:64:21 | call to params |
160+
| filter_flow.rb:91:12:91:17 | call to params |
130161
| params_flow.rb:3:10:3:15 | call to params |
131162
| params_flow.rb:7:10:7:15 | call to params |
132163
| params_flow.rb:11:10:11:15 | call to params |
@@ -192,6 +223,11 @@ httpInputAccesses
192223
| controllers/foo/bars_controller.rb:21:21:21:26 | call to params | ActionController::Metal#params |
193224
| controllers/foo/bars_controller.rb:22:10:22:15 | call to params | ActionController::Metal#params |
194225
| controllers/posts_controller.rb:26:23:26:28 | call to params | ActionController::Metal#params |
226+
| filter_flow.rb:14:12:14:17 | call to params | ActionController::Metal#params |
227+
| filter_flow.rb:30:12:30:17 | call to params | ActionController::Metal#params |
228+
| filter_flow.rb:47:12:47:17 | call to params | ActionController::Metal#params |
229+
| filter_flow.rb:64:16:64:21 | call to params | ActionController::Metal#params |
230+
| filter_flow.rb:91:12:91:17 | call to params | ActionController::Metal#params |
195231
| input_access.rb:3:5:3:18 | call to params | ActionDispatch::Request#params |
196232
| input_access.rb:4:5:4:22 | call to parameters | ActionDispatch::Request#parameters |
197233
| input_access.rb:5:5:5:15 | call to GET | ActionDispatch::Request#GET |

0 commit comments

Comments
 (0)