Skip to content

Commit aaeb8a0

Browse files
hmachmac
authored
Merge pull request github#12493 from hmac/ar-sinks
2 parents 959f93a + d814e15 commit aaeb8a0

File tree

5 files changed

+114
-33
lines changed

5 files changed

+114
-33
lines changed

‎ruby/ql/lib/change-notes/2023-03-13-rails-sinks.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The Active Record query methods `reorder` and `count_by_sql` are now recognised as SQL executions.
5+
* Calls to `ActiveRecord::Connection#execute`, including those via subclasses, are now recognised as SQL executions.
6+
* Data flow through `ActionController::Parameters#require` is now tracked properly.

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -632,9 +632,9 @@ private module ParamsSummaries {
632632
// dig doesn't always return a Parameters instance, but it will if the
633633
// given key refers to a nested hash parameter.
634634
"dig", "each", "each_key", "each_pair", "each_value", "except", "keep_if", "merge",
635-
"merge!", "permit", "reject", "reject!", "reverse_merge", "reverse_merge!", "select",
636-
"select!", "slice", "slice!", "transform_keys", "transform_keys!", "transform_values",
637-
"transform_values!", "with_defaults", "with_defaults!"
635+
"merge!", "permit", "reject", "reject!", "require", "reverse_merge", "reverse_merge!",
636+
"select", "select!", "slice", "slice!", "transform_keys", "transform_keys!",
637+
"transform_values", "transform_values!", "with_defaults", "with_defaults!"
638638
]
639639
}
640640

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

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,18 @@ private predicate isBuiltInMethodForActiveRecordModelInstance(string methodName)
3131
methodName = objectInstanceMethodName()
3232
}
3333

34+
private API::Node activeRecordClassApiNode() {
35+
result =
36+
// class Foo < ActiveRecord::Base
37+
// class Bar < Foo
38+
[
39+
API::getTopLevelMember("ActiveRecord").getMember("Base"),
40+
// In Rails applications `ApplicationRecord` typically extends `ActiveRecord::Base`, but we
41+
// treat it separately in case the `ApplicationRecord` definition is not in the database.
42+
API::getTopLevelMember("ApplicationRecord")
43+
].getASubclass()
44+
}
45+
3446
/**
3547
* A `ClassDeclaration` for a class that inherits from `ActiveRecord::Base`. For example,
3648
*
@@ -45,15 +57,8 @@ private predicate isBuiltInMethodForActiveRecordModelInstance(string methodName)
4557
*/
4658
class ActiveRecordModelClass extends ClassDeclaration {
4759
ActiveRecordModelClass() {
48-
// class Foo < ActiveRecord::Base
49-
// class Bar < Foo
5060
this.getSuperclassExpr() =
51-
[
52-
API::getTopLevelMember("ActiveRecord").getMember("Base"),
53-
// In Rails applications `ApplicationRecord` typically extends `ActiveRecord::Base`, but we
54-
// treat it separately in case the `ApplicationRecord` definition is not in the database.
55-
API::getTopLevelMember("ApplicationRecord")
56-
].getASubclass().getAValueReachableFromSource().asExpr().getExpr()
61+
activeRecordClassApiNode().getAValueReachableFromSource().asExpr().getExpr()
5762
}
5863

5964
// Gets the class declaration for this class and all of its super classes
@@ -116,14 +121,14 @@ private Expr sqlFragmentArgument(MethodCall call) {
116121
[
117122
"delete_all", "delete_by", "destroy_all", "destroy_by", "exists?", "find_by", "find_by!",
118123
"find_or_create_by", "find_or_create_by!", "find_or_initialize_by", "find_by_sql", "from",
119-
"group", "having", "joins", "lock", "not", "order", "pluck", "where", "rewhere", "select",
120-
"reselect", "update_all"
124+
"group", "having", "joins", "lock", "not", "order", "reorder", "pluck", "where",
125+
"rewhere", "select", "reselect", "update_all"
121126
] and
122127
result = call.getArgument(0)
123128
or
124129
methodName = "calculate" and result = call.getArgument(1)
125130
or
126-
methodName in ["average", "count", "maximum", "minimum", "sum"] and
131+
methodName in ["average", "count", "maximum", "minimum", "sum", "count_by_sql"] and
127132
result = call.getArgument(0)
128133
or
129134
// This format was supported until Rails 2.3.8
@@ -208,11 +213,18 @@ class ActiveRecordSqlExecutionRange extends SqlExecution::Range {
208213
exists(PotentiallyUnsafeSqlExecutingMethodCall mc |
209214
this.asExpr().getNode() = mc.getSqlFragmentSinkArgument()
210215
)
216+
or
217+
this = activeRecordConnectionInstance().getAMethodCall("execute").getArgument(0) and
218+
unsafeSqlExpr(this.asExpr().getExpr())
211219
}
212220

213221
override DataFlow::Node getSql() { result = this }
214222
}
215223

224+
private API::Node activeRecordConnectionInstance() {
225+
result = activeRecordClassApiNode().getReturn("connection")
226+
}
227+
216228
// TODO: model `ActiveRecord` sanitizers
217229
// https://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html
218230
/**

‎ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,10 @@ def some_request_handler
9090
# BAD: executes `UPDATE "users" SET #{params[:fields]}`
9191
# where `params[:fields]` is unsanitized
9292
User.update_all(params[:fields])
93+
94+
User.reorder(params[:direction])
95+
96+
User.count_by_sql(params[:custom_sql_query])
9397
end
9498
end
9599

@@ -151,3 +155,26 @@ def unsafe_action
151155
users = User.annotate("this is an unsafe annotation:#{params[:comment]}").find_by(user_name: name)
152156
end
153157
end
158+
159+
# A regression test
160+
161+
class Regression < ActiveRecord::Base
162+
end
163+
164+
class RegressionController < ActionController::Base
165+
def index
166+
my_params = permitted_params
167+
query = "SELECT * FROM users WHERE id = #{my_params[:user_id]}"
168+
result = Regression.find_by_sql(query)
169+
end
170+
171+
172+
def permitted_params
173+
params.require(:my_key).permit(:id, :user_id, :my_type)
174+
end
175+
176+
def show
177+
ActiveRecord::Base.connection.execute("SELECT * FROM users WHERE id = #{permitted_params[:user_id]}")
178+
Regression.connection.execute("SELECT * FROM users WHERE id = #{permitted_params[:user_id]}")
179+
end
180+
end

‎ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected

Lines changed: 55 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,27 @@ edges
2626
| ActiveRecordInjection.rb:84:19:84:24 | call to params : | ActiveRecordInjection.rb:84:19:84:33 | ...[...] |
2727
| ActiveRecordInjection.rb:88:18:88:23 | call to params : | ActiveRecordInjection.rb:88:18:88:35 | ...[...] |
2828
| ActiveRecordInjection.rb:92:21:92:26 | call to params : | ActiveRecordInjection.rb:92:21:92:35 | ...[...] |
29-
| ActiveRecordInjection.rb:98:10:98:15 | call to params : | ActiveRecordInjection.rb:99:11:99:12 | ps : |
30-
| ActiveRecordInjection.rb:99:11:99:12 | ps : | ActiveRecordInjection.rb:99:11:99:17 | ...[...] : |
31-
| ActiveRecordInjection.rb:99:11:99:17 | ...[...] : | ActiveRecordInjection.rb:104:20:104:32 | ... + ... |
32-
| ActiveRecordInjection.rb:137:21:137:26 | call to params : | ActiveRecordInjection.rb:137:21:137:44 | ...[...] : |
33-
| ActiveRecordInjection.rb:137:21:137:44 | ...[...] : | ActiveRecordInjection.rb:20:22:20:30 | condition : |
34-
| ActiveRecordInjection.rb:151:59:151:64 | call to params : | ActiveRecordInjection.rb:151:59:151:74 | ...[...] : |
35-
| ActiveRecordInjection.rb:151:59:151:74 | ...[...] : | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." |
29+
| ActiveRecordInjection.rb:94:18:94:23 | call to params : | ActiveRecordInjection.rb:94:18:94:35 | ...[...] |
30+
| ActiveRecordInjection.rb:96:23:96:28 | call to params : | ActiveRecordInjection.rb:96:23:96:47 | ...[...] |
31+
| ActiveRecordInjection.rb:102:10:102:15 | call to params : | ActiveRecordInjection.rb:103:11:103:12 | ps : |
32+
| ActiveRecordInjection.rb:103:11:103:12 | ps : | ActiveRecordInjection.rb:103:11:103:17 | ...[...] : |
33+
| ActiveRecordInjection.rb:103:11:103:17 | ...[...] : | ActiveRecordInjection.rb:108:20:108:32 | ... + ... |
34+
| ActiveRecordInjection.rb:141:21:141:26 | call to params : | ActiveRecordInjection.rb:141:21:141:44 | ...[...] : |
35+
| ActiveRecordInjection.rb:141:21:141:44 | ...[...] : | ActiveRecordInjection.rb:20:22:20:30 | condition : |
36+
| ActiveRecordInjection.rb:155:59:155:64 | call to params : | ActiveRecordInjection.rb:155:59:155:74 | ...[...] : |
37+
| ActiveRecordInjection.rb:155:59:155:74 | ...[...] : | ActiveRecordInjection.rb:155:27:155:76 | "this is an unsafe annotation:..." |
38+
| ActiveRecordInjection.rb:166:17:166:32 | call to permitted_params : | ActiveRecordInjection.rb:167:47:167:55 | my_params : |
39+
| ActiveRecordInjection.rb:167:47:167:55 | my_params : | ActiveRecordInjection.rb:167:47:167:65 | ...[...] : |
40+
| ActiveRecordInjection.rb:167:47:167:65 | ...[...] : | ActiveRecordInjection.rb:168:37:168:41 | query |
41+
| ActiveRecordInjection.rb:173:5:173:10 | call to params : | ActiveRecordInjection.rb:173:5:173:27 | call to require : |
42+
| ActiveRecordInjection.rb:173:5:173:27 | call to require : | ActiveRecordInjection.rb:173:5:173:59 | call to permit : |
43+
| ActiveRecordInjection.rb:173:5:173:59 | call to permit : | ActiveRecordInjection.rb:166:17:166:32 | call to permitted_params : |
44+
| ActiveRecordInjection.rb:173:5:173:59 | call to permit : | ActiveRecordInjection.rb:177:77:177:92 | call to permitted_params : |
45+
| ActiveRecordInjection.rb:173:5:173:59 | call to permit : | ActiveRecordInjection.rb:178:69:178:84 | call to permitted_params : |
46+
| ActiveRecordInjection.rb:177:77:177:92 | call to permitted_params : | ActiveRecordInjection.rb:177:77:177:102 | ...[...] : |
47+
| ActiveRecordInjection.rb:177:77:177:102 | ...[...] : | ActiveRecordInjection.rb:177:43:177:104 | "SELECT * FROM users WHERE id ..." |
48+
| ActiveRecordInjection.rb:178:69:178:84 | call to permitted_params : | ActiveRecordInjection.rb:178:69:178:94 | ...[...] : |
49+
| ActiveRecordInjection.rb:178:69:178:94 | ...[...] : | ActiveRecordInjection.rb:178:35:178:96 | "SELECT * FROM users WHERE id ..." |
3650
| ArelInjection.rb:4:12:4:17 | call to params : | ArelInjection.rb:4:12:4:29 | ...[...] : |
3751
| ArelInjection.rb:4:12:4:29 | ...[...] : | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." |
3852
nodes
@@ -78,23 +92,40 @@ nodes
7892
| ActiveRecordInjection.rb:88:18:88:35 | ...[...] | semmle.label | ...[...] |
7993
| ActiveRecordInjection.rb:92:21:92:26 | call to params : | semmle.label | call to params : |
8094
| ActiveRecordInjection.rb:92:21:92:35 | ...[...] | semmle.label | ...[...] |
81-
| ActiveRecordInjection.rb:98:10:98:15 | call to params : | semmle.label | call to params : |
82-
| ActiveRecordInjection.rb:99:11:99:12 | ps : | semmle.label | ps : |
83-
| ActiveRecordInjection.rb:99:11:99:17 | ...[...] : | semmle.label | ...[...] : |
84-
| ActiveRecordInjection.rb:104:20:104:32 | ... + ... | semmle.label | ... + ... |
85-
| ActiveRecordInjection.rb:137:21:137:26 | call to params : | semmle.label | call to params : |
86-
| ActiveRecordInjection.rb:137:21:137:44 | ...[...] : | semmle.label | ...[...] : |
87-
| ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | semmle.label | "this is an unsafe annotation:..." |
88-
| ActiveRecordInjection.rb:151:59:151:64 | call to params : | semmle.label | call to params : |
89-
| ActiveRecordInjection.rb:151:59:151:74 | ...[...] : | semmle.label | ...[...] : |
95+
| ActiveRecordInjection.rb:94:18:94:23 | call to params : | semmle.label | call to params : |
96+
| ActiveRecordInjection.rb:94:18:94:35 | ...[...] | semmle.label | ...[...] |
97+
| ActiveRecordInjection.rb:96:23:96:28 | call to params : | semmle.label | call to params : |
98+
| ActiveRecordInjection.rb:96:23:96:47 | ...[...] | semmle.label | ...[...] |
99+
| ActiveRecordInjection.rb:102:10:102:15 | call to params : | semmle.label | call to params : |
100+
| ActiveRecordInjection.rb:103:11:103:12 | ps : | semmle.label | ps : |
101+
| ActiveRecordInjection.rb:103:11:103:17 | ...[...] : | semmle.label | ...[...] : |
102+
| ActiveRecordInjection.rb:108:20:108:32 | ... + ... | semmle.label | ... + ... |
103+
| ActiveRecordInjection.rb:141:21:141:26 | call to params : | semmle.label | call to params : |
104+
| ActiveRecordInjection.rb:141:21:141:44 | ...[...] : | semmle.label | ...[...] : |
105+
| ActiveRecordInjection.rb:155:27:155:76 | "this is an unsafe annotation:..." | semmle.label | "this is an unsafe annotation:..." |
106+
| ActiveRecordInjection.rb:155:59:155:64 | call to params : | semmle.label | call to params : |
107+
| ActiveRecordInjection.rb:155:59:155:74 | ...[...] : | semmle.label | ...[...] : |
108+
| ActiveRecordInjection.rb:166:17:166:32 | call to permitted_params : | semmle.label | call to permitted_params : |
109+
| ActiveRecordInjection.rb:167:47:167:55 | my_params : | semmle.label | my_params : |
110+
| ActiveRecordInjection.rb:167:47:167:65 | ...[...] : | semmle.label | ...[...] : |
111+
| ActiveRecordInjection.rb:168:37:168:41 | query | semmle.label | query |
112+
| ActiveRecordInjection.rb:173:5:173:10 | call to params : | semmle.label | call to params : |
113+
| ActiveRecordInjection.rb:173:5:173:27 | call to require : | semmle.label | call to require : |
114+
| ActiveRecordInjection.rb:173:5:173:59 | call to permit : | semmle.label | call to permit : |
115+
| ActiveRecordInjection.rb:177:43:177:104 | "SELECT * FROM users WHERE id ..." | semmle.label | "SELECT * FROM users WHERE id ..." |
116+
| ActiveRecordInjection.rb:177:77:177:92 | call to permitted_params : | semmle.label | call to permitted_params : |
117+
| ActiveRecordInjection.rb:177:77:177:102 | ...[...] : | semmle.label | ...[...] : |
118+
| ActiveRecordInjection.rb:178:35:178:96 | "SELECT * FROM users WHERE id ..." | semmle.label | "SELECT * FROM users WHERE id ..." |
119+
| ActiveRecordInjection.rb:178:69:178:84 | call to permitted_params : | semmle.label | call to permitted_params : |
120+
| ActiveRecordInjection.rb:178:69:178:94 | ...[...] : | semmle.label | ...[...] : |
90121
| ArelInjection.rb:4:12:4:17 | call to params : | semmle.label | call to params : |
91122
| ArelInjection.rb:4:12:4:29 | ...[...] : | semmle.label | ...[...] : |
92123
| ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | semmle.label | "SELECT * FROM users WHERE nam..." |
93124
subpaths
94125
#select
95126
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:70:23:70:28 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on a $@. | ActiveRecordInjection.rb:70:23:70:28 | call to params | user-provided value |
96127
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:70:38:70:43 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on a $@. | ActiveRecordInjection.rb:70:38:70:43 | call to params | user-provided value |
97-
| ActiveRecordInjection.rb:23:16:23:24 | condition | ActiveRecordInjection.rb:137:21:137:26 | call to params : | ActiveRecordInjection.rb:23:16:23:24 | condition | This SQL query depends on a $@. | ActiveRecordInjection.rb:137:21:137:26 | call to params | user-provided value |
128+
| ActiveRecordInjection.rb:23:16:23:24 | condition | ActiveRecordInjection.rb:141:21:141:26 | call to params : | ActiveRecordInjection.rb:23:16:23:24 | condition | This SQL query depends on a $@. | ActiveRecordInjection.rb:141:21:141:26 | call to params | user-provided value |
98129
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] | ActiveRecordInjection.rb:35:30:35:35 | call to params : | ActiveRecordInjection.rb:35:30:35:44 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:35:30:35:35 | call to params | user-provided value |
99130
| ActiveRecordInjection.rb:39:18:39:32 | ...[...] | ActiveRecordInjection.rb:39:18:39:23 | call to params : | ActiveRecordInjection.rb:39:18:39:32 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:39:18:39:23 | call to params | user-provided value |
100131
| ActiveRecordInjection.rb:43:20:43:42 | "id = '#{...}'" | ActiveRecordInjection.rb:43:29:43:34 | call to params : | ActiveRecordInjection.rb:43:20:43:42 | "id = '#{...}'" | This SQL query depends on a $@. | ActiveRecordInjection.rb:43:29:43:34 | call to params | user-provided value |
@@ -108,6 +139,11 @@ subpaths
108139
| ActiveRecordInjection.rb:84:19:84:33 | ...[...] | ActiveRecordInjection.rb:84:19:84:24 | call to params : | ActiveRecordInjection.rb:84:19:84:33 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:84:19:84:24 | call to params | user-provided value |
109140
| ActiveRecordInjection.rb:88:18:88:35 | ...[...] | ActiveRecordInjection.rb:88:18:88:23 | call to params : | ActiveRecordInjection.rb:88:18:88:35 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:88:18:88:23 | call to params | user-provided value |
110141
| ActiveRecordInjection.rb:92:21:92:35 | ...[...] | ActiveRecordInjection.rb:92:21:92:26 | call to params : | ActiveRecordInjection.rb:92:21:92:35 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:92:21:92:26 | call to params | user-provided value |
111-
| ActiveRecordInjection.rb:104:20:104:32 | ... + ... | ActiveRecordInjection.rb:98:10:98:15 | call to params : | ActiveRecordInjection.rb:104:20:104:32 | ... + ... | This SQL query depends on a $@. | ActiveRecordInjection.rb:98:10:98:15 | call to params | user-provided value |
112-
| ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | ActiveRecordInjection.rb:151:59:151:64 | call to params : | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | This SQL query depends on a $@. | ActiveRecordInjection.rb:151:59:151:64 | call to params | user-provided value |
142+
| ActiveRecordInjection.rb:94:18:94:35 | ...[...] | ActiveRecordInjection.rb:94:18:94:23 | call to params : | ActiveRecordInjection.rb:94:18:94:35 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:94:18:94:23 | call to params | user-provided value |
143+
| ActiveRecordInjection.rb:96:23:96:47 | ...[...] | ActiveRecordInjection.rb:96:23:96:28 | call to params : | ActiveRecordInjection.rb:96:23:96:47 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:96:23:96:28 | call to params | user-provided value |
144+
| ActiveRecordInjection.rb:108:20:108:32 | ... + ... | ActiveRecordInjection.rb:102:10:102:15 | call to params : | ActiveRecordInjection.rb:108:20:108:32 | ... + ... | This SQL query depends on a $@. | ActiveRecordInjection.rb:102:10:102:15 | call to params | user-provided value |
145+
| ActiveRecordInjection.rb:155:27:155:76 | "this is an unsafe annotation:..." | ActiveRecordInjection.rb:155:59:155:64 | call to params : | ActiveRecordInjection.rb:155:27:155:76 | "this is an unsafe annotation:..." | This SQL query depends on a $@. | ActiveRecordInjection.rb:155:59:155:64 | call to params | user-provided value |
146+
| ActiveRecordInjection.rb:168:37:168:41 | query | ActiveRecordInjection.rb:173:5:173:10 | call to params : | ActiveRecordInjection.rb:168:37:168:41 | query | This SQL query depends on a $@. | ActiveRecordInjection.rb:173:5:173:10 | call to params | user-provided value |
147+
| ActiveRecordInjection.rb:177:43:177:104 | "SELECT * FROM users WHERE id ..." | ActiveRecordInjection.rb:173:5:173:10 | call to params : | ActiveRecordInjection.rb:177:43:177:104 | "SELECT * FROM users WHERE id ..." | This SQL query depends on a $@. | ActiveRecordInjection.rb:173:5:173:10 | call to params | user-provided value |
148+
| ActiveRecordInjection.rb:178:35:178:96 | "SELECT * FROM users WHERE id ..." | ActiveRecordInjection.rb:173:5:173:10 | call to params : | ActiveRecordInjection.rb:178:35:178:96 | "SELECT * FROM users WHERE id ..." | This SQL query depends on a $@. | ActiveRecordInjection.rb:173:5:173:10 | call to params | user-provided value |
113149
| ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | ArelInjection.rb:4:12:4:17 | call to params : | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | This SQL query depends on a $@. | ArelInjection.rb:4:12:4:17 | call to params | user-provided value |

0 commit comments

Comments
 (0)