Skip to content

Commit ae3d91b

Browse files
hmachmac
committed
Ruby: First draft of rails callback flow
1 parent 6eeb711 commit ae3d91b

5 files changed

Lines changed: 206 additions & 1 deletion

File tree

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

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,4 +387,119 @@ module Filters {
387387
* `pred` and `succ` may be methods bound to callbacks or controller actions.
388388
*/
389389
predicate next(Method pred, Method succ) { next(_, pred, succ) }
390+
391+
/**
392+
* Holds if `n` is the post-update node of a self-variable access in method
393+
* `m` which stores content in the self variable.
394+
* In the example below, `n` is the post-update node for `@x = 1`.
395+
* ```rb
396+
* def m
397+
* @x = 1
398+
* end
399+
* ```
400+
*/
401+
private predicate variableAccessPostUpdate(DataFlow::PostUpdateNode n, Method m) {
402+
n.getPreUpdateNode().asExpr() instanceof SelfVariableAccessCfgNode and
403+
m = n.getPreUpdateNode().asExpr().getExpr().getEnclosingCallable() and
404+
DataFlowPrivate::storeStep(_, _, n)
405+
}
406+
407+
/**
408+
* Holds if `n` is the syntactically last dataflow node in `m` that satisfies `variableAccessPostUpdate`.
409+
* In the example below, `n` is the post-update node for `@y = 2`.
410+
* ```rb
411+
* def m
412+
* @x = 1
413+
* @y = 2
414+
* end
415+
* ```
416+
*/
417+
private predicate lastVariableAccessPostUpdate(DataFlow::PostUpdateNode n, Method m) {
418+
variableAccessPostUpdate(n, m) and
419+
not exists(DataFlow::PostUpdateNode n2 |
420+
variableAccessPostUpdate(n2, m) and
421+
n.getPreUpdateNode().asExpr().getASuccessor+() = n2.getPreUpdateNode().asExpr()
422+
)
423+
}
424+
425+
/**
426+
* Holds if `a` is the syntactically last access of the self variable `v` in method `m`.
427+
*/
428+
private predicate lastMethodSelfVarAccess(SelfVariableAccessCfgNode a, SelfVariable v, Method m) {
429+
a.getExpr().getEnclosingMethod() = m and
430+
v = a.getExpr().getVariable() and
431+
not exists(SelfVariableAccessCfgNode a2 |
432+
a2.getExpr().getVariable() = v and
433+
a.getASuccessor+() = a2
434+
)
435+
}
436+
437+
/**
438+
* Holds if there is no access to `self` in method `m`.
439+
*/
440+
private predicate noSelfVarAccess(Method m) {
441+
not exists(SelfVariableAccess a | a.getEnclosingMethod() = m)
442+
}
443+
444+
/**
445+
* Holds if `n` is the self parameter of method `m`.
446+
*/
447+
private predicate selfParameter(DataFlowPrivate::SelfParameterNode n, Method m) {
448+
m = n.getMethod()
449+
}
450+
451+
/**
452+
* A class defining additional jump steps arising from filters.
453+
*/
454+
class FilterJumpStep extends DataFlowPrivate::AdditionalJumpStep {
455+
/**
456+
* Holds if data can flow from `pred` to `succ` via a callback chain.
457+
* `pred` is the post-update node of the self parameter in a method, and
458+
* `succ` is the self parameter of a subsequent method that is executed as
459+
* part of the callback chain.
460+
*/
461+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
462+
exists(Method predMethod, Method succMethod |
463+
next(predMethod, succMethod) and
464+
(
465+
// Flow from an update of self in `pred` to the self parameter of `succ`
466+
//
467+
// def a
468+
// @x = 1 ---+
469+
// end |
470+
// |
471+
// def b <----+
472+
// ...
473+
//
474+
lastVariableAccessPostUpdate(pred, predMethod) and
475+
selfParameter(succ, succMethod)
476+
or
477+
// When there is no update of self in `pred`, flow from the last access to self to the self parameter of `succ`
478+
//
479+
// def a
480+
// foo() ---+
481+
// x = 1 |
482+
// end |
483+
// |
484+
// def b <---+
485+
// ...
486+
//
487+
not variableAccessPostUpdate(_, predMethod) and
488+
lastMethodSelfVarAccess(pred.asExpr(), _, predMethod) and
489+
selfParameter(succ, succMethod)
490+
or
491+
// When there is no access to self in `pred`, flow from the self parameter of `pred` to the self parameter of `succ`
492+
//
493+
// def a ---+
494+
// end |
495+
// |
496+
// def b <--+
497+
// ...
498+
noSelfVarAccess(predMethod) and
499+
selfParameter(pred, predMethod) and
500+
selfParameter(succ, succMethod)
501+
)
502+
)
503+
}
504+
}
390505
}

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,36 @@
1+
additionalFlowSteps
2+
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:74:3:77:5 | self in ensure_user_can_edit_comments |
3+
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
4+
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
5+
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/posts_controller.rb:12:3:13:5 | self in index |
6+
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/posts_controller.rb:15:3:16:5 | self in show |
7+
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/posts_controller.rb:18:3:19:5 | self in upvote |
8+
| controllers/application_controller.rb:11:53:11:59 | self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
9+
| controllers/application_controller.rb:11:53:11:59 | self | controllers/photos_controller.rb:3:3:6:5 | self in show |
10+
| controllers/application_controller.rb:11:53:11:59 | self | controllers/posts_controller.rb:23:3:25:5 | self in set_post |
11+
| controllers/comments_controller.rb:50:5:50:12 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
12+
| controllers/comments_controller.rb:53:3:54:5 | self in create | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
13+
| controllers/comments_controller.rb:57:5:61:7 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
14+
| controllers/comments_controller.rb:58:33:58:48 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
15+
| controllers/comments_controller.rb:60:58:60:63 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
16+
| controllers/comments_controller.rb:65:15:65:20 | self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
17+
| controllers/comments_controller.rb:69:12:69:18 | self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
18+
| controllers/comments_controller.rb:76:5:76:68 | self | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
19+
| controllers/comments_controller.rb:76:5:76:68 | self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
20+
| controllers/comments_controller.rb:80:5:80:12 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
21+
| controllers/comments_controller.rb:84:61:84:68 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
22+
| controllers/comments_controller.rb:88:5:88:28 | self | controllers/comments_controller.rb:95:3:97:5 | self in this_must_run_last |
23+
| controllers/comments_controller.rb:91:3:93:5 | self in this_must_run_first | controllers/application_controller.rb:10:3:12:5 | self in log_request |
24+
| controllers/comments_controller.rb:99:3:100:5 | self in foo | controllers/comments_controller.rb:102:3:103:5 | self in bar |
25+
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:17:3:51:5 | self in index |
26+
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:53:3:54:5 | self in create |
27+
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:56:3:62:5 | self in show |
28+
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:64:3:66:5 | self in photo |
29+
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:68:3:70:5 | self in destroy |
30+
| controllers/photos_controller.rb:5:5:5:6 | [post] self | controllers/photos_controller.rb:8:3:9:5 | self in foo |
31+
| controllers/posts_controller.rb:18:3:19:5 | self in upvote | controllers/posts_controller.rb:27:3:29:5 | self in log_upvote |
32+
| controllers/posts_controller.rb:24:5:24:9 | [post] self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
33+
filterChain
134
| controllers/comments_controller.rb:17:3:51:5 | index | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:99:3:100:5 | foo |
235
| controllers/comments_controller.rb:17:3:51:5 | index | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
336
| controllers/comments_controller.rb:17:3:51:5 | index | controllers/comments_controller.rb:17:3:51:5 | index | controllers/comments_controller.rb:87:3:89:5 | check_feature_flags |

‎ruby/ql/test/library-tests/frameworks/action_controller/Filters.ql‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,7 @@ private import codeql.ruby.frameworks.ActionController
33
private import codeql.ruby.DataFlow
44

55
query predicate filterChain = ActionController::Filters::next/3;
6+
7+
query predicate additionalFlowSteps(DataFlow::Node pred, DataFlow::Node succ) {
8+
any(ActionController::Filters::FilterJumpStep s).step(pred, succ)
9+
}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,20 @@ edges
2121
| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" |
2222
| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:90:10:90:13 | code |
2323
| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:90:10:90:13 | code |
24+
| CodeInjection.rb:101:3:102:5 | self in index [@foo] : | CodeInjection.rb:111:3:113:5 | self in baz [@foo] : |
25+
| CodeInjection.rb:101:3:102:5 | self in index [@foo] : | CodeInjection.rb:111:3:113:5 | self in baz [@foo] : |
26+
| CodeInjection.rb:105:5:105:8 | [post] self [@foo] : | CodeInjection.rb:108:3:109:5 | self in bar [@foo] : |
27+
| CodeInjection.rb:105:5:105:8 | [post] self [@foo] : | CodeInjection.rb:108:3:109:5 | self in bar [@foo] : |
28+
| CodeInjection.rb:105:12:105:17 | call to params : | CodeInjection.rb:105:12:105:23 | ...[...] : |
29+
| CodeInjection.rb:105:12:105:17 | call to params : | CodeInjection.rb:105:12:105:23 | ...[...] : |
30+
| CodeInjection.rb:105:12:105:23 | ...[...] : | CodeInjection.rb:105:5:105:8 | [post] self [@foo] : |
31+
| CodeInjection.rb:105:12:105:23 | ...[...] : | CodeInjection.rb:105:5:105:8 | [post] self [@foo] : |
32+
| CodeInjection.rb:108:3:109:5 | self in bar [@foo] : | CodeInjection.rb:101:3:102:5 | self in index [@foo] : |
33+
| CodeInjection.rb:108:3:109:5 | self in bar [@foo] : | CodeInjection.rb:101:3:102:5 | self in index [@foo] : |
34+
| CodeInjection.rb:111:3:113:5 | self in baz [@foo] : | CodeInjection.rb:112:10:112:13 | self [@foo] : |
35+
| CodeInjection.rb:111:3:113:5 | self in baz [@foo] : | CodeInjection.rb:112:10:112:13 | self [@foo] : |
36+
| CodeInjection.rb:112:10:112:13 | self [@foo] : | CodeInjection.rb:112:10:112:13 | @foo |
37+
| CodeInjection.rb:112:10:112:13 | self [@foo] : | CodeInjection.rb:112:10:112:13 | @foo |
2438
nodes
2539
| CodeInjection.rb:5:12:5:17 | call to params : | semmle.label | call to params : |
2640
| CodeInjection.rb:5:12:5:17 | call to params : | semmle.label | call to params : |
@@ -50,6 +64,22 @@ nodes
5064
| CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | semmle.label | "prefix_#{...}_suffix" |
5165
| CodeInjection.rb:90:10:90:13 | code | semmle.label | code |
5266
| CodeInjection.rb:90:10:90:13 | code | semmle.label | code |
67+
| CodeInjection.rb:101:3:102:5 | self in index [@foo] : | semmle.label | self in index [@foo] : |
68+
| CodeInjection.rb:101:3:102:5 | self in index [@foo] : | semmle.label | self in index [@foo] : |
69+
| CodeInjection.rb:105:5:105:8 | [post] self [@foo] : | semmle.label | [post] self [@foo] : |
70+
| CodeInjection.rb:105:5:105:8 | [post] self [@foo] : | semmle.label | [post] self [@foo] : |
71+
| CodeInjection.rb:105:12:105:17 | call to params : | semmle.label | call to params : |
72+
| CodeInjection.rb:105:12:105:17 | call to params : | semmle.label | call to params : |
73+
| CodeInjection.rb:105:12:105:23 | ...[...] : | semmle.label | ...[...] : |
74+
| CodeInjection.rb:105:12:105:23 | ...[...] : | semmle.label | ...[...] : |
75+
| CodeInjection.rb:108:3:109:5 | self in bar [@foo] : | semmle.label | self in bar [@foo] : |
76+
| CodeInjection.rb:108:3:109:5 | self in bar [@foo] : | semmle.label | self in bar [@foo] : |
77+
| CodeInjection.rb:111:3:113:5 | self in baz [@foo] : | semmle.label | self in baz [@foo] : |
78+
| CodeInjection.rb:111:3:113:5 | self in baz [@foo] : | semmle.label | self in baz [@foo] : |
79+
| CodeInjection.rb:112:10:112:13 | @foo | semmle.label | @foo |
80+
| CodeInjection.rb:112:10:112:13 | @foo | semmle.label | @foo |
81+
| CodeInjection.rb:112:10:112:13 | self [@foo] : | semmle.label | self [@foo] : |
82+
| CodeInjection.rb:112:10:112:13 | self [@foo] : | semmle.label | self [@foo] : |
5383
subpaths
5484
#select
5585
| CodeInjection.rb:8:10:8:13 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:8:10:8:13 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
@@ -64,3 +94,4 @@ subpaths
6494
| CodeInjection.rb:86:10:86:37 | ... + ... | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:86:10:86:37 | ... + ... | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |
6595
| CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |
6696
| CodeInjection.rb:90:10:90:13 | code | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:90:10:90:13 | code | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |
97+
| CodeInjection.rb:112:10:112:13 | @foo | CodeInjection.rb:105:12:105:17 | call to params : | CodeInjection.rb:112:10:112:13 | @foo | This code execution depends on a $@. | CodeInjection.rb:105:12:105:17 | call to params | user-provided value |

‎ruby/ql/test/query-tests/security/cwe-094/CodeInjection/CodeInjection.rb‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,4 +89,26 @@ def create
8989

9090
eval(code); # BAD
9191
end
92-
end
92+
end
93+
94+
Rails.application.routes.draw { resources :posts }
95+
96+
class PostsController < ActionController::Base
97+
before_action :foo
98+
before_action :bar
99+
after_action :baz
100+
101+
def index
102+
end
103+
104+
def foo
105+
@foo = params[:foo]
106+
end
107+
108+
def bar
109+
end
110+
111+
def baz
112+
eval(@foo)
113+
end
114+
end

0 commit comments

Comments
 (0)