hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-23 07:45:17 +02:00
Code Issues Packages Projects Releases Wiki Activity

Identify more vulnerable ActiveRecord methods

Browse Source 
This change identifies the following patterns:

- `Model.select(input)`
- `Model.reselect(input)`
- `Model.rewhere(input)`
- `Model.update_all(input)`
- `model.reload(lock: input)`
This commit is contained in:
Harry Maclean
2021-09-29 11:47:07 +01:00
parent 270d13e4ac
commit 615beeec80
3 changed files with 48 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
View File

@@ -70,7 +70,8 @@ private Expr sqlFragmentArgument(MethodCall call) {
[
"delete_by", "destroy_by", "exists?", "find_by", "find_by!", "find_or_create_by",
"find_or_create_by!", "find_or_initialize_by", "find_by_sql", "from", "group", "having",
"joins", "lock", "not", "order", "pluck", "where"
"joins", "lock", "not", "order", "pluck", "where", "rewhere", "select", "reselect",
"update_all"
] and
result = call.getArgument(0)
or
@@ -82,6 +83,9 @@ private Expr sqlFragmentArgument(MethodCall call) {
// This format was supported until Rails 2.3.8
methodName = ["all", "find", "first", "last"] and
result = call.getKeywordArgument("conditions")
or
methodName = "reload" and
result = call.getKeywordArgument("lock")
)
)
}
@@ -122,7 +126,6 @@ class PotentiallyUnsafeSqlExecutingMethodCall extends ActiveRecordModelClassMeth
// The SQL fragment argument itself
private Expr sqlFragmentExpr;
// TODO: `find` with `lock:` option also takes an SQL fragment
PotentiallyUnsafeSqlExecutingMethodCall() {
exists(Expr arg |
arg = sqlFragmentArgument(this) and

18
ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb
View File

@@ -62,6 +62,24 @@ class FooController < ActionController::Base
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')` LIMIT 1
# where `params[:id]` is unsanitized
User.find_or_initialize_by("id = '#{params[:id]}'")
user = User.first
# BAD: executes `SELECT "users".* FROM "users" WHERE id = 1 LIMIT 1 #{params[:lock]}`
# where `params[:lock]` is unsanitized
user.reload(lock: params[:lock])
# BAD: executes `SELECT #{params[:column]} FROM "users"`
# where `params[:column]` is unsanitized
User.select(params[:column])
User.reselect(params[:column])
# BAD: executes `SELECT "users".* FROM "users" WHERE (#{params[:condition]})`
# where `params[:condition]` is unsanitized
User.rewhere(params[:condition])
# BAD: executes `UPDATE "users" SET #{params[:fields]}`
# where `params[:fields]` is unsanitized
User.update_all(params[:fields])
end
end

34
ql/test/query-tests/security/cwe-089/SqlInjection.expected
View File

@@ -13,9 +13,13 @@ edges
| ActiveRecordInjection.rb:60:38:60:43 | call to params : | ActiveRecordInjection.rb:60:38:60:50 | ...[...] : |
| ActiveRecordInjection.rb:60:38:60:50 | ...[...] : | ActiveRecordInjection.rb:8:31:8:34 | pass : |
| ActiveRecordInjection.rb:64:41:64:46 | call to params : | ActiveRecordInjection.rb:64:32:64:54 | "id = '#{...}'" |
| ActiveRecordInjection.rb:70:10:70:15 | call to params : | ActiveRecordInjection.rb:76:20:76:32 | ... + ... |
| ActiveRecordInjection.rb:109:21:109:26 | call to params : | ActiveRecordInjection.rb:109:21:109:44 | ...[...] : |
| ActiveRecordInjection.rb:109:21:109:44 | ...[...] : | ActiveRecordInjection.rb:20:22:20:30 | condition : |
| ActiveRecordInjection.rb:73:17:73:22 | call to params : | ActiveRecordInjection.rb:73:17:73:31 | ...[...] |
| ActiveRecordInjection.rb:74:19:74:24 | call to params : | ActiveRecordInjection.rb:74:19:74:33 | ...[...] |
| ActiveRecordInjection.rb:78:18:78:23 | call to params : | ActiveRecordInjection.rb:78:18:78:35 | ...[...] |
| ActiveRecordInjection.rb:82:21:82:26 | call to params : | ActiveRecordInjection.rb:82:21:82:35 | ...[...] |
| ActiveRecordInjection.rb:88:10:88:15 | call to params : | ActiveRecordInjection.rb:94:20:94:32 | ... + ... |
| ActiveRecordInjection.rb:127:21:127:26 | call to params : | ActiveRecordInjection.rb:127:21:127:44 | ...[...] : |
| ActiveRecordInjection.rb:127:21:127:44 | ...[...] : | ActiveRecordInjection.rb:20:22:20:30 | condition : |
nodes
| ActiveRecordInjection.rb:8:25:8:28 | name : | semmle.label | name : |
| ActiveRecordInjection.rb:8:31:8:34 | pass : | semmle.label | pass : |
@@ -40,15 +44,23 @@ nodes
| ActiveRecordInjection.rb:60:38:60:50 | ...[...] : | semmle.label | ...[...] : |
| ActiveRecordInjection.rb:64:32:64:54 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
| ActiveRecordInjection.rb:64:41:64:46 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:70:10:70:15 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:76:20:76:32 | ... + ... | semmle.label | ... + ... |
| ActiveRecordInjection.rb:109:21:109:26 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:109:21:109:44 | ...[...] : | semmle.label | ...[...] : |
| ActiveRecordInjection.rb:73:17:73:22 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:73:17:73:31 | ...[...] | semmle.label | ...[...] |
| ActiveRecordInjection.rb:74:19:74:24 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:74:19:74:33 | ...[...] | semmle.label | ...[...] |
| ActiveRecordInjection.rb:78:18:78:23 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:78:18:78:35 | ...[...] | semmle.label | ...[...] |
| ActiveRecordInjection.rb:82:21:82:26 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:82:21:82:35 | ...[...] | semmle.label | ...[...] |
| ActiveRecordInjection.rb:88:10:88:15 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:94:20:94:32 | ... + ... | semmle.label | ... + ... |
| ActiveRecordInjection.rb:127:21:127:26 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:127:21:127:44 | ...[...] : | semmle.label | ...[...] : |
subpaths
#select
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
@@ -56,4 +68,8 @@ subpaths
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |