Skip to content

Commit 615beee

Browse files
hmachmac
committed
Identify more vulnerable ActiveRecord methods
This change identifies the following patterns: - `Model.select(input)` - `Model.reselect(input)` - `Model.rewhere(input)` - `Model.update_all(input)` - `model.reload(lock: input)`
1 parent 270d13e commit 615beee

File tree

3 files changed

+48
-11
lines changed

3 files changed

+48
-11
lines changed

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@ private Expr sqlFragmentArgument(MethodCall call) {
7070
[
7171
"delete_by", "destroy_by", "exists?", "find_by", "find_by!", "find_or_create_by",
7272
"find_or_create_by!", "find_or_initialize_by", "find_by_sql", "from", "group", "having",
73-
"joins", "lock", "not", "order", "pluck", "where"
73+
"joins", "lock", "not", "order", "pluck", "where", "rewhere", "select", "reselect",
74+
"update_all"
7475
] and
7576
result = call.getArgument(0)
7677
or
@@ -82,6 +83,9 @@ private Expr sqlFragmentArgument(MethodCall call) {
8283
// This format was supported until Rails 2.3.8
8384
methodName = ["all", "find", "first", "last"] and
8485
result = call.getKeywordArgument("conditions")
86+
or
87+
methodName = "reload" and
88+
result = call.getKeywordArgument("lock")
8589
)
8690
)
8791
}
@@ -122,7 +126,6 @@ class PotentiallyUnsafeSqlExecutingMethodCall extends ActiveRecordModelClassMeth
122126
// The SQL fragment argument itself
123127
private Expr sqlFragmentExpr;
124128

125-
// TODO: `find` with `lock:` option also takes an SQL fragment
126129
PotentiallyUnsafeSqlExecutingMethodCall() {
127130
exists(Expr arg |
128131
arg = sqlFragmentArgument(this) and

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,24 @@ def some_request_handler
6262
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')` LIMIT 1
6363
# where `params[:id]` is unsanitized
6464
User.find_or_initialize_by("id = '#{params[:id]}'")
65+
66+
user = User.first
67+
# BAD: executes `SELECT "users".* FROM "users" WHERE id = 1 LIMIT 1 #{params[:lock]}`
68+
# where `params[:lock]` is unsanitized
69+
user.reload(lock: params[:lock])
70+
71+
# BAD: executes `SELECT #{params[:column]} FROM "users"`
72+
# where `params[:column]` is unsanitized
73+
User.select(params[:column])
74+
User.reselect(params[:column])
75+
76+
# BAD: executes `SELECT "users".* FROM "users" WHERE (#{params[:condition]})`
77+
# where `params[:condition]` is unsanitized
78+
User.rewhere(params[:condition])
79+
80+
# BAD: executes `UPDATE "users" SET #{params[:fields]}`
81+
# where `params[:fields]` is unsanitized
82+
User.update_all(params[:fields])
6583
end
6684
end
6785

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

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,13 @@ edges
1313
| ActiveRecordInjection.rb:60:38:60:43 | call to params : | ActiveRecordInjection.rb:60:38:60:50 | ...[...] : |
1414
| ActiveRecordInjection.rb:60:38:60:50 | ...[...] : | ActiveRecordInjection.rb:8:31:8:34 | pass : |
1515
| ActiveRecordInjection.rb:64:41:64:46 | call to params : | ActiveRecordInjection.rb:64:32:64:54 | "id = '#{...}'" |
16-
| ActiveRecordInjection.rb:70:10:70:15 | call to params : | ActiveRecordInjection.rb:76:20:76:32 | ... + ... |
17-
| ActiveRecordInjection.rb:109:21:109:26 | call to params : | ActiveRecordInjection.rb:109:21:109:44 | ...[...] : |
18-
| ActiveRecordInjection.rb:109:21:109:44 | ...[...] : | ActiveRecordInjection.rb:20:22:20:30 | condition : |
16+
| ActiveRecordInjection.rb:73:17:73:22 | call to params : | ActiveRecordInjection.rb:73:17:73:31 | ...[...] |
17+
| ActiveRecordInjection.rb:74:19:74:24 | call to params : | ActiveRecordInjection.rb:74:19:74:33 | ...[...] |
18+
| ActiveRecordInjection.rb:78:18:78:23 | call to params : | ActiveRecordInjection.rb:78:18:78:35 | ...[...] |
19+
| ActiveRecordInjection.rb:82:21:82:26 | call to params : | ActiveRecordInjection.rb:82:21:82:35 | ...[...] |
20+
| ActiveRecordInjection.rb:88:10:88:15 | call to params : | ActiveRecordInjection.rb:94:20:94:32 | ... + ... |
21+
| ActiveRecordInjection.rb:127:21:127:26 | call to params : | ActiveRecordInjection.rb:127:21:127:44 | ...[...] : |
22+
| ActiveRecordInjection.rb:127:21:127:44 | ...[...] : | ActiveRecordInjection.rb:20:22:20:30 | condition : |
1923
nodes
2024
| ActiveRecordInjection.rb:8:25:8:28 | name : | semmle.label | name : |
2125
| ActiveRecordInjection.rb:8:31:8:34 | pass : | semmle.label | pass : |
@@ -40,20 +44,32 @@ nodes
4044
| ActiveRecordInjection.rb:60:38:60:50 | ...[...] : | semmle.label | ...[...] : |
4145
| ActiveRecordInjection.rb:64:32:64:54 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
4246
| ActiveRecordInjection.rb:64:41:64:46 | call to params : | semmle.label | call to params : |
43-
| ActiveRecordInjection.rb:70:10:70:15 | call to params : | semmle.label | call to params : |
44-
| ActiveRecordInjection.rb:76:20:76:32 | ... + ... | semmle.label | ... + ... |
45-
| ActiveRecordInjection.rb:109:21:109:26 | call to params : | semmle.label | call to params : |
46-
| ActiveRecordInjection.rb:109:21:109:44 | ...[...] : | semmle.label | ...[...] : |
47+
| ActiveRecordInjection.rb:73:17:73:22 | call to params : | semmle.label | call to params : |
48+
| ActiveRecordInjection.rb:73:17:73:31 | ...[...] | semmle.label | ...[...] |
49+
| ActiveRecordInjection.rb:74:19:74:24 | call to params : | semmle.label | call to params : |
50+
| ActiveRecordInjection.rb:74:19:74:33 | ...[...] | semmle.label | ...[...] |
51+
| ActiveRecordInjection.rb:78:18:78:23 | call to params : | semmle.label | call to params : |
52+
| ActiveRecordInjection.rb:78:18:78:35 | ...[...] | semmle.label | ...[...] |
53+
| ActiveRecordInjection.rb:82:21:82:26 | call to params : | semmle.label | call to params : |
54+
| ActiveRecordInjection.rb:82:21:82:35 | ...[...] | semmle.label | ...[...] |
55+
| ActiveRecordInjection.rb:88:10:88:15 | call to params : | semmle.label | call to params : |
56+
| ActiveRecordInjection.rb:94:20:94:32 | ... + ... | semmle.label | ... + ... |
57+
| ActiveRecordInjection.rb:127:21:127:26 | call to params : | semmle.label | call to params : |
58+
| ActiveRecordInjection.rb:127:21:127:44 | ...[...] : | semmle.label | ...[...] : |
4759
subpaths
4860
#select
4961
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:60:23:60:28 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:60:23:60:28 | call to params | a user-provided value |
5062
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:60:38:60:43 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:60:38:60:43 | call to params | a user-provided value |
51-
| ActiveRecordInjection.rb:23:16:23:24 | condition | ActiveRecordInjection.rb:109:21:109:26 | call to params : | ActiveRecordInjection.rb:23:16:23:24 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:109:21:109:26 | call to params | a user-provided value |
63+
| ActiveRecordInjection.rb:23:16:23:24 | condition | ActiveRecordInjection.rb:127:21:127:26 | call to params : | ActiveRecordInjection.rb:23:16:23:24 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:127:21:127:26 | call to params | a user-provided value |
5264
| 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 $@. | ActiveRecordInjection.rb:35:30:35:35 | call to params | a user-provided value |
5365
| 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 $@. | ActiveRecordInjection.rb:39:18:39:23 | call to params | a user-provided value |
5466
| 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 $@. | ActiveRecordInjection.rb:43:29:43:34 | call to params | a user-provided value |
5567
| ActiveRecordInjection.rb:47:22:47:44 | "id = '#{...}'" | ActiveRecordInjection.rb:47:31:47:36 | call to params : | ActiveRecordInjection.rb:47:22:47:44 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:47:31:47:36 | call to params | a user-provided value |
5668
| ActiveRecordInjection.rb:51:16:51:21 | <<-SQL | ActiveRecordInjection.rb:52:21:52:26 | call to params : | ActiveRecordInjection.rb:51:16:51:21 | <<-SQL | This SQL query depends on $@. | ActiveRecordInjection.rb:52:21:52:26 | call to params | a user-provided value |
5769
| ActiveRecordInjection.rb:58:20:58:47 | "user.id = '#{...}'" | ActiveRecordInjection.rb:58:34:58:39 | call to params : | ActiveRecordInjection.rb:58:20:58:47 | "user.id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:58:34:58:39 | call to params | a user-provided value |
5870
| ActiveRecordInjection.rb:64:32:64:54 | "id = '#{...}'" | ActiveRecordInjection.rb:64:41:64:46 | call to params : | ActiveRecordInjection.rb:64:32:64:54 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:64:41:64:46 | call to params | a user-provided value |
59-
| ActiveRecordInjection.rb:76:20:76:32 | ... + ... | ActiveRecordInjection.rb:70:10:70:15 | call to params : | ActiveRecordInjection.rb:76:20:76:32 | ... + ... | This SQL query depends on $@. | ActiveRecordInjection.rb:70:10:70:15 | call to params | a user-provided value |
71+
| ActiveRecordInjection.rb:73:17:73:31 | ...[...] | ActiveRecordInjection.rb:73:17:73:22 | call to params : | ActiveRecordInjection.rb:73:17:73:31 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:73:17:73:22 | call to params | a user-provided value |
72+
| ActiveRecordInjection.rb:74:19:74:33 | ...[...] | ActiveRecordInjection.rb:74:19:74:24 | call to params : | ActiveRecordInjection.rb:74:19:74:33 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:74:19:74:24 | call to params | a user-provided value |
73+
| ActiveRecordInjection.rb:78:18:78:35 | ...[...] | ActiveRecordInjection.rb:78:18:78:23 | call to params : | ActiveRecordInjection.rb:78:18:78:35 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:78:18:78:23 | call to params | a user-provided value |
74+
| ActiveRecordInjection.rb:82:21:82:35 | ...[...] | ActiveRecordInjection.rb:82:21:82:26 | call to params : | ActiveRecordInjection.rb:82:21:82:35 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:82:21:82:26 | call to params | a user-provided value |
75+
| ActiveRecordInjection.rb:94:20:94:32 | ... + ... | ActiveRecordInjection.rb:88:10:88:15 | call to params : | ActiveRecordInjection.rb:94:20:94:32 | ... + ... | This SQL query depends on $@. | ActiveRecordInjection.rb:88:10:88:15 | call to params | a user-provided value |

0 commit comments

Comments
 (0)