Skip to content

Commit 7f103b9

Browse files
hmachmac
authored
Merge pull request #319 from github/hmac-activerecord-updates
Add some more vulnerable ActiveRecord methods
2 parents 0ea228e + 7191e1c commit 7f103b9

File tree

7 files changed

+148
-73
lines changed

7 files changed

+148
-73
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ private class ApplicationControllerAccess extends ConstantReadAccess {
2828
* class FooController < ActionController::Base
2929
* def delete_handler
3030
* uid = params[:id]
31-
* User.delete_all("id = ?", uid)
31+
* User.delete_by("id = ?", uid)
3232
* end
3333
* end
3434
* ```

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,16 +68,24 @@ private Expr sqlFragmentArgument(MethodCall call) {
6868
(
6969
methodName =
7070
[
71-
"delete_all", "destroy_all", "exists?", "find_by", "find_by_sql", "from", "group",
72-
"having", "joins", "lock", "not", "order", "pluck", "where"
71+
"delete_all", "delete_by", "destroy_all", "destroy_by", "exists?", "find_by", "find_by!",
72+
"find_or_create_by", "find_or_create_by!", "find_or_initialize_by", "find_by_sql", "from",
73+
"group", "having", "joins", "lock", "not", "order", "pluck", "where", "rewhere", "select",
74+
"reselect", "update_all"
7375
] and
7476
result = call.getArgument(0)
7577
or
7678
methodName = "calculate" and result = call.getArgument(1)
7779
or
80+
methodName in ["average", "count", "maximum", "minimum", "sum"] and
81+
result = call.getArgument(0)
82+
or
7883
// This format was supported until Rails 2.3.8
7984
methodName = ["all", "find", "first", "last"] and
8085
result = call.getKeywordArgument("conditions")
86+
or
87+
methodName = "reload" and
88+
result = call.getKeywordArgument("lock")
8189
)
8290
)
8391
}
@@ -118,7 +126,6 @@ class PotentiallyUnsafeSqlExecutingMethodCall extends ActiveRecordModelClassMeth
118126
// The SQL fragment argument itself
119127
private Expr sqlFragmentExpr;
120128

121-
// TODO: `find` with `lock:` option also takes an SQL fragment
122129
PotentiallyUnsafeSqlExecutingMethodCall() {
123130
exists(Expr arg |
124131
arg = sqlFragmentArgument(this) and

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ actionControllerActionMethods
1313
| app/controllers/foo/bars_controller.rb:15:3:19:5 | show |
1414
paramsCalls
1515
| ActiveRecordInjection.rb:35:30:35:35 | call to params |
16-
| ActiveRecordInjection.rb:39:30:39:35 | call to params |
17-
| ActiveRecordInjection.rb:43:32:43:37 | call to params |
16+
| ActiveRecordInjection.rb:39:29:39:34 | call to params |
17+
| ActiveRecordInjection.rb:43:31:43:36 | call to params |
1818
| ActiveRecordInjection.rb:48:21:48:26 | call to params |
1919
| ActiveRecordInjection.rb:54:34:54:39 | call to params |
2020
| ActiveRecordInjection.rb:56:23:56:28 | call to params |
@@ -24,16 +24,16 @@ paramsCalls
2424
| ActiveRecordInjection.rb:77:12:77:17 | call to params |
2525
| ActiveRecordInjection.rb:83:12:83:17 | call to params |
2626
| ActiveRecordInjection.rb:88:15:88:20 | call to params |
27-
| ActiveRecordInjection.rb:94:22:94:27 | call to params |
27+
| ActiveRecordInjection.rb:94:21:94:26 | call to params |
2828
| app/controllers/foo/bars_controller.rb:8:21:8:26 | call to params |
2929
| app/controllers/foo/bars_controller.rb:9:10:9:15 | call to params |
3030
| app/controllers/foo/bars_controller.rb:16:21:16:26 | call to params |
3131
| app/controllers/foo/bars_controller.rb:17:10:17:15 | call to params |
3232
| app/views/foo/bars/show.html.erb:5:9:5:14 | call to params |
3333
paramsSources
3434
| ActiveRecordInjection.rb:35:30:35:35 | call to params |
35-
| ActiveRecordInjection.rb:39:30:39:35 | call to params |
36-
| ActiveRecordInjection.rb:43:32:43:37 | call to params |
35+
| ActiveRecordInjection.rb:39:29:39:34 | call to params |
36+
| ActiveRecordInjection.rb:43:31:43:36 | call to params |
3737
| ActiveRecordInjection.rb:48:21:48:26 | call to params |
3838
| ActiveRecordInjection.rb:54:34:54:39 | call to params |
3939
| ActiveRecordInjection.rb:56:23:56:28 | call to params |
@@ -43,7 +43,7 @@ paramsSources
4343
| ActiveRecordInjection.rb:77:12:77:17 | call to params |
4444
| ActiveRecordInjection.rb:83:12:83:17 | call to params |
4545
| ActiveRecordInjection.rb:88:15:88:20 | call to params |
46-
| ActiveRecordInjection.rb:94:22:94:27 | call to params |
46+
| ActiveRecordInjection.rb:94:21:94:26 | call to params |
4747
| app/controllers/foo/bars_controller.rb:8:21:8:26 | call to params |
4848
| app/controllers/foo/bars_controller.rb:9:10:9:15 | call to params |
4949
| app/controllers/foo/bars_controller.rb:16:21:16:26 | call to params |

‎ql/test/library-tests/frameworks/ActiveRecord.expected

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@ activeRecordModelClasses
44
| ActiveRecordInjection.rb:19:1:25:3 | Admin |
55
activeRecordSqlExecutionRanges
66
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
7-
| ActiveRecordInjection.rb:23:17:23:25 | condition |
7+
| ActiveRecordInjection.rb:23:16:23:24 | condition |
88
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] |
9-
| ActiveRecordInjection.rb:39:21:39:43 | "id = '#{...}'" |
10-
| ActiveRecordInjection.rb:43:23:43:45 | "id = '#{...}'" |
9+
| ActiveRecordInjection.rb:39:20:39:42 | "id = '#{...}'" |
10+
| ActiveRecordInjection.rb:43:22:43:44 | "id = '#{...}'" |
1111
| ActiveRecordInjection.rb:47:16:47:21 | <<-SQL |
1212
| ActiveRecordInjection.rb:54:20:54:47 | "user.id = '#{...}'" |
13-
| ActiveRecordInjection.rb:68:21:68:33 | ... + ... |
13+
| ActiveRecordInjection.rb:68:20:68:32 | ... + ... |
1414
| ActiveRecordInjection.rb:75:16:75:28 | "name #{...}" |
1515
| ActiveRecordInjection.rb:80:20:80:39 | "username = #{...}" |
1616
activeRecordModelClassMethodCalls
@@ -19,28 +19,28 @@ activeRecordModelClassMethodCalls
1919
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
2020
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by |
2121
| ActiveRecordInjection.rb:15:5:15:46 | call to users |
22-
| ActiveRecordInjection.rb:23:5:23:26 | call to destroy_all |
22+
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
2323
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
24-
| ActiveRecordInjection.rb:39:5:39:44 | call to delete_all |
25-
| ActiveRecordInjection.rb:43:5:43:47 | call to destroy_all |
24+
| ActiveRecordInjection.rb:39:5:39:43 | call to delete_by |
25+
| ActiveRecordInjection.rb:43:5:43:46 | call to destroy_by |
2626
| ActiveRecordInjection.rb:47:5:47:35 | call to where |
2727
| ActiveRecordInjection.rb:54:5:54:14 | call to where |
2828
| ActiveRecordInjection.rb:54:5:54:48 | call to not |
2929
| ActiveRecordInjection.rb:56:5:56:51 | call to authenticate |
30-
| ActiveRecordInjection.rb:68:5:68:34 | call to delete_all |
30+
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
3131
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
3232
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
3333
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by |
3434
| ActiveRecordInjection.rb:88:5:88:34 | call to find |
35-
| ActiveRecordInjection.rb:94:5:94:46 | call to delete_all |
35+
| ActiveRecordInjection.rb:94:5:94:45 | call to delete_by |
3636
potentiallyUnsafeSqlExecutingMethodCall
3737
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
38-
| ActiveRecordInjection.rb:23:5:23:26 | call to destroy_all |
38+
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
3939
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
40-
| ActiveRecordInjection.rb:39:5:39:44 | call to delete_all |
41-
| ActiveRecordInjection.rb:43:5:43:47 | call to destroy_all |
40+
| ActiveRecordInjection.rb:39:5:39:43 | call to delete_by |
41+
| ActiveRecordInjection.rb:43:5:43:46 | call to destroy_by |
4242
| ActiveRecordInjection.rb:47:5:47:35 | call to where |
4343
| ActiveRecordInjection.rb:54:5:54:48 | call to not |
44-
| ActiveRecordInjection.rb:68:5:68:34 | call to delete_all |
44+
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
4545
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
4646
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |

‎ql/test/library-tests/frameworks/ActiveRecordInjection.rb

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ def self.from(user_group_id)
1717
end
1818

1919
class Admin < User
20-
def self.delete_all(condition = nil)
21-
# BAD: `delete_all` overrides an ActiveRecord method, but doesn't perform
20+
def self.delete_by(condition = nil)
21+
# BAD: `delete_by` overrides an ActiveRecord method, but doesn't perform
2222
# any validation before passing its arguments on to another ActiveRecord method
23-
destroy_all(condition)
23+
destroy_by(condition)
2424
end
2525
end
2626

@@ -36,11 +36,11 @@ def some_request_handler
3636

3737
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
3838
# where `params[:id]` is unsanitized
39-
User.delete_all("id = '#{params[:id]}'")
39+
User.delete_by("id = '#{params[:id]}'")
4040

4141
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
4242
# where `params[:id]` is unsanitized
43-
User.destroy_all(["id = '#{params[:id]}'"])
43+
User.destroy_by(["id = '#{params[:id]}'"])
4444

4545
# BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
4646
# where `params[:min_id]` is unsanitized
@@ -65,7 +65,7 @@ def some_other_request_handler
6565

6666
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
6767
# where `uid` is unsantized
68-
User.delete_all("id " + uidEq)
68+
User.delete_by("id " + uidEq)
6969
end
7070

7171
def safe_paths
@@ -91,6 +91,6 @@ def safe_paths
9191

9292
class BazController < BarController
9393
def yet_another_handler
94-
Admin.delete_all(params[:admin_condition])
94+
Admin.delete_by(params[:admin_condition])
9595
end
9696
end

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

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ def self.from(user_group_id)
1717
end
1818

1919
class Admin < User
20-
def self.delete_all(condition = nil)
21-
# BAD: `delete_all` overrides an ActiveRecord method, but doesn't perform
20+
def self.delete_by(condition = nil)
21+
# BAD: `delete_by overrides an ActiveRecord method, but doesn't perform
2222
# any validation before passing its arguments on to another ActiveRecord method
23-
destroy_all(condition)
23+
destroy_by(condition)
2424
end
2525
end
2626

@@ -34,12 +34,26 @@ def some_request_handler
3434
# where `params[:column]` is unsanitized
3535
User.calculate(:average, params[:column])
3636

37+
# BAD: executes `SELECT MAX(#{params[:column]}) FROM "users"`
38+
# where `params[:column]` is unsanitized
39+
User.maximum(params[:column])
40+
41+
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
42+
# where `params[:id]` is unsanitized
43+
User.delete_by("id = '#{params[:id]}'")
44+
3745
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
3846
# where `params[:id]` is unsanitized
47+
# (in Rails < 4.0)
3948
User.delete_all("id = '#{params[:id]}'")
4049

4150
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
4251
# where `params[:id]` is unsanitized
52+
User.destroy_by(["id = '#{params[:id]}'"])
53+
54+
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
55+
# where `params[:id]` is unsanitized
56+
# (in Rails < 4.0)
4357
User.destroy_all(["id = '#{params[:id]}'"])
4458

4559
# BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
@@ -54,6 +68,28 @@ def some_request_handler
5468
User.where.not("user.id = '#{params[:id]}'")
5569

5670
User.authenticate(params[:name], params[:pass])
71+
72+
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')` LIMIT 1
73+
# where `params[:id]` is unsanitized
74+
User.find_or_initialize_by("id = '#{params[:id]}'")
75+
76+
user = User.first
77+
# BAD: executes `SELECT "users".* FROM "users" WHERE id = 1 LIMIT 1 #{params[:lock]}`
78+
# where `params[:lock]` is unsanitized
79+
user.reload(lock: params[:lock])
80+
81+
# BAD: executes `SELECT #{params[:column]} FROM "users"`
82+
# where `params[:column]` is unsanitized
83+
User.select(params[:column])
84+
User.reselect(params[:column])
85+
86+
# BAD: executes `SELECT "users".* FROM "users" WHERE (#{params[:condition]})`
87+
# where `params[:condition]` is unsanitized
88+
User.rewhere(params[:condition])
89+
90+
# BAD: executes `UPDATE "users" SET #{params[:fields]}`
91+
# where `params[:fields]` is unsanitized
92+
User.update_all(params[:fields])
5793
end
5894
end
5995

@@ -65,7 +101,7 @@ def some_other_request_handler
65101

66102
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
67103
# where `uid` is unsantized
68-
User.delete_all("id " + uidEq)
104+
User.delete_by("id " + uidEq)
69105
end
70106

71107
def safe_paths
@@ -98,6 +134,6 @@ def safe_paths
98134

99135
class BazController < BarController
100136
def yet_another_handler
101-
Admin.delete_all(params[:admin_condition])
137+
Admin.delete_by(params[:admin_condition])
102138
end
103139
end

0 commit comments

Comments
 (0)