hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-27 01:35:13 +02:00
Code Issues Packages Projects Releases Wiki Activity

delete/destroy_all -> delete/destroy_by

Browse Source 
The ActiveRecord `delete_all` and `destroy_all` methods do not take a
condition argument - they act on the scope of their receiver.

The `delete_by` and `destroy_by` methods do take an argument which can
be raw SQL, and are therefore vulnerable to SQL injection.

For more info:

https://api.rubyonrails.org/v6.1.4/classes/ActiveRecord/Relation.html#method-i-delete_all
https://api.rubyonrails.org/v6.1.4/classes/ActiveRecord/Relation.html#method-i-delete_by
This commit is contained in:
Harry Maclean
2021-09-29 10:34:02 +01:00
parent 3a1b294c21
commit 56919eee0b
6 changed files with 49 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
ql/lib/codeql/ruby/frameworks/ActionController.qll
View File

@@ -28,7 +28,7 @@ private class ApplicationControllerAccess extends ConstantReadAccess {
* class FooController < ActionController::Base
* def delete_handler
* uid = params[:id]
* User.delete_all("id = ?", uid)
* User.delete_by("id = ?", uid)
* end
* end
* ```

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

@@ -68,8 +68,8 @@ private Expr sqlFragmentArgument(MethodCall call) {
(
methodName =
[
"delete_all", "destroy_all", "exists?", "find_by", "find_by_sql", "from", "group",
"having", "joins", "lock", "not", "order", "pluck", "where"
"delete_by", "destroy_by", "exists?", "find_by", "find_by_sql", "from", "group", "having",
"joins", "lock", "not", "order", "pluck", "where"
] and
result = call.getArgument(0)
or

26
ql/test/library-tests/frameworks/ActiveRecord.expected
View File

@@ -4,13 +4,13 @@ activeRecordModelClasses
| ActiveRecordInjection.rb:19:1:25:3 | Admin |
activeRecordSqlExecutionRanges
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
| ActiveRecordInjection.rb:23:17:23:25 | condition |
| ActiveRecordInjection.rb:23:16:23:24 | condition |
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] |
| ActiveRecordInjection.rb:39:21:39:43 | "id = '#{...}'" |
| ActiveRecordInjection.rb:43:23:43:45 | "id = '#{...}'" |
| ActiveRecordInjection.rb:39:20:39:42 | "id = '#{...}'" |
| ActiveRecordInjection.rb:43:22:43:44 | "id = '#{...}'" |
| ActiveRecordInjection.rb:47:16:47:21 | <<-SQL |
| ActiveRecordInjection.rb:54:20:54:47 | "user.id = '#{...}'" |
| ActiveRecordInjection.rb:68:21:68:33 | ... + ... |
| ActiveRecordInjection.rb:68:20:68:32 | ... + ... |
| ActiveRecordInjection.rb:75:16:75:28 | "name #{...}" |
| ActiveRecordInjection.rb:80:20:80:39 | "username = #{...}" |
activeRecordModelClassMethodCalls
@@ -19,28 +19,28 @@ activeRecordModelClassMethodCalls
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by |
| ActiveRecordInjection.rb:15:5:15:46 | call to users |
| ActiveRecordInjection.rb:23:5:23:26 | call to destroy_all |
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
| ActiveRecordInjection.rb:39:5:39:44 | call to delete_all |
| ActiveRecordInjection.rb:43:5:43:47 | call to destroy_all |
| ActiveRecordInjection.rb:39:5:39:43 | call to delete_by |
| ActiveRecordInjection.rb:43:5:43:46 | call to destroy_by |
| ActiveRecordInjection.rb:47:5:47:35 | call to where |
| ActiveRecordInjection.rb:54:5:54:14 | call to where |
| ActiveRecordInjection.rb:54:5:54:48 | call to not |
| ActiveRecordInjection.rb:56:5:56:51 | call to authenticate |
| ActiveRecordInjection.rb:68:5:68:34 | call to delete_all |
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by |
| ActiveRecordInjection.rb:88:5:88:34 | call to find |
| ActiveRecordInjection.rb:94:5:94:46 | call to delete_all |
| ActiveRecordInjection.rb:94:5:94:45 | call to delete_by |
potentiallyUnsafeSqlExecutingMethodCall
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
| ActiveRecordInjection.rb:23:5:23:26 | call to destroy_all |
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
| ActiveRecordInjection.rb:39:5:39:44 | call to delete_all |
| ActiveRecordInjection.rb:43:5:43:47 | call to destroy_all |
| ActiveRecordInjection.rb:39:5:39:43 | call to delete_by |
| ActiveRecordInjection.rb:43:5:43:46 | call to destroy_by |
| ActiveRecordInjection.rb:47:5:47:35 | call to where |
| ActiveRecordInjection.rb:54:5:54:48 | call to not |
| ActiveRecordInjection.rb:68:5:68:34 | call to delete_all |
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |

14
ql/test/library-tests/frameworks/ActiveRecordInjection.rb
View File

@@ -17,10 +17,10 @@ class User < ApplicationRecord
end
class Admin < User
def self.delete_all(condition = nil)
# BAD: `delete_all` overrides an ActiveRecord method, but doesn't perform
def self.delete_by(condition = nil)
# BAD: `delete_by` overrides an ActiveRecord method, but doesn't perform
# any validation before passing its arguments on to another ActiveRecord method
destroy_all(condition)
destroy_by(condition)
end
end
@@ -36,11 +36,11 @@ class FooController < ActionController::Base
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
# where `params[:id]` is unsanitized
User.delete_all("id = '#{params[:id]}'")
User.delete_by("id = '#{params[:id]}'")
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
# where `params[:id]` is unsanitized
User.destroy_all(["id = '#{params[:id]}'"])
User.destroy_by(["id = '#{params[:id]}'"])
# BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
# where `params[:min_id]` is unsanitized
@@ -65,7 +65,7 @@ class BarController < ApplicationController
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
# where `uid` is unsantized
User.delete_all("id " + uidEq)
User.delete_by("id " + uidEq)
end
def safe_paths
@@ -91,6 +91,6 @@ end
class BazController < BarController
def yet_another_handler
Admin.delete_all(params[:admin_condition])
Admin.delete_by(params[:admin_condition])
end
end

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

@@ -17,10 +17,10 @@ class User < ApplicationRecord
end
class Admin < User
def self.delete_all(condition = nil)
# BAD: `delete_all` overrides an ActiveRecord method, but doesn't perform
def self.delete_by(condition = nil)
# BAD: `delete_by overrides an ActiveRecord method, but doesn't perform
# any validation before passing its arguments on to another ActiveRecord method
destroy_all(condition)
destroy_by(condition)
end
end
@@ -40,11 +40,11 @@ class FooController < ActionController::Base
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
# where `params[:id]` is unsanitized
User.delete_all("id = '#{params[:id]}'")
User.delete_by("id = '#{params[:id]}'")
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
# where `params[:id]` is unsanitized
User.destroy_all(["id = '#{params[:id]}'"])
User.destroy_by(["id = '#{params[:id]}'"])
# BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
# where `params[:min_id]` is unsanitized
@@ -69,7 +69,7 @@ class BarController < ApplicationController
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
# where `uid` is unsantized
User.delete_all("id " + uidEq)
User.delete_by("id " + uidEq)
end
def safe_paths
@@ -102,6 +102,6 @@ end
class BazController < BarController
def yet_another_handler
Admin.delete_all(params[:admin_condition])
Admin.delete_by(params[:admin_condition])
end
end

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

@@ -1,34 +1,34 @@
edges
| ActiveRecordInjection.rb:8:25:8:28 | name : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
| ActiveRecordInjection.rb:8:31:8:34 | pass : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
| ActiveRecordInjection.rb:20:23:20:31 | condition : | ActiveRecordInjection.rb:23:17:23:25 | condition |
| ActiveRecordInjection.rb:20:22:20:30 | condition : | ActiveRecordInjection.rb:23:16:23:24 | condition |
| ActiveRecordInjection.rb:35:30:35:35 | call to params : | ActiveRecordInjection.rb:35:30:35:44 | ...[...] |
| ActiveRecordInjection.rb:39:18:39:23 | call to params : | ActiveRecordInjection.rb:39:18:39:32 | ...[...] |
| ActiveRecordInjection.rb:43:30:43:35 | call to params : | ActiveRecordInjection.rb:43:21:43:43 | "id = '#{...}'" |
| ActiveRecordInjection.rb:47:32:47:37 | call to params : | ActiveRecordInjection.rb:47:23:47:45 | "id = '#{...}'" |
| ActiveRecordInjection.rb:43:29:43:34 | call to params : | ActiveRecordInjection.rb:43:20:43:42 | "id = '#{...}'" |
| ActiveRecordInjection.rb:47:31:47:36 | call to params : | ActiveRecordInjection.rb:47:22:47:44 | "id = '#{...}'" |
| ActiveRecordInjection.rb:52:21:52:26 | call to params : | ActiveRecordInjection.rb:51:16:51:21 | <<-SQL |
| ActiveRecordInjection.rb:58:34:58:39 | call to params : | ActiveRecordInjection.rb:58:20:58:47 | "user.id = '#{...}'" |
| ActiveRecordInjection.rb:60:23:60:28 | call to params : | ActiveRecordInjection.rb:60:23:60:35 | ...[...] : |
| ActiveRecordInjection.rb:60:23:60:35 | ...[...] : | ActiveRecordInjection.rb:8:25:8:28 | name : |
| 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:66:10:66:15 | call to params : | ActiveRecordInjection.rb:72:21:72:33 | ... + ... |
| ActiveRecordInjection.rb:105:22:105:27 | call to params : | ActiveRecordInjection.rb:105:22:105:45 | ...[...] : |
| ActiveRecordInjection.rb:105:22:105:45 | ...[...] : | ActiveRecordInjection.rb:20:23:20:31 | condition : |
| ActiveRecordInjection.rb:66:10:66:15 | call to params : | ActiveRecordInjection.rb:72:20:72:32 | ... + ... |
| ActiveRecordInjection.rb:105:21:105:26 | call to params : | ActiveRecordInjection.rb:105:21:105:44 | ...[...] : |
| ActiveRecordInjection.rb:105:21:105: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 : |
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | semmle.label | "name='#{...}' and pass='#{...}'" |
| ActiveRecordInjection.rb:20:23:20:31 | condition : | semmle.label | condition : |
| ActiveRecordInjection.rb:23:17:23:25 | condition | semmle.label | condition |
| ActiveRecordInjection.rb:20:22:20:30 | condition : | semmle.label | condition : |
| ActiveRecordInjection.rb:23:16:23:24 | condition | semmle.label | condition |
| ActiveRecordInjection.rb:35:30:35:35 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] | semmle.label | ...[...] |
| ActiveRecordInjection.rb:39:18:39:23 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:39:18:39:32 | ...[...] | semmle.label | ...[...] |
| ActiveRecordInjection.rb:43:21:43:43 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
| ActiveRecordInjection.rb:43:30:43:35 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:47:23:47:45 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
| ActiveRecordInjection.rb:47:32:47:37 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:43:20:43:42 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
| ActiveRecordInjection.rb:43:29:43:34 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:47:22:47:44 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
| ActiveRecordInjection.rb:47:31:47:36 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:51:16:51:21 | <<-SQL | semmle.label | <<-SQL |
| ActiveRecordInjection.rb:52:21:52:26 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:58:20:58:47 | "user.id = '#{...}'" | semmle.label | "user.id = '#{...}'" |
@@ -38,18 +38,18 @@ nodes
| ActiveRecordInjection.rb:60:38:60:43 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:60:38:60:50 | ...[...] : | semmle.label | ...[...] : |
| ActiveRecordInjection.rb:66:10:66:15 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:72:21:72:33 | ... + ... | semmle.label | ... + ... |
| ActiveRecordInjection.rb:105:22:105:27 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:105:22:105:45 | ...[...] : | semmle.label | ...[...] : |
| ActiveRecordInjection.rb:72:20:72:32 | ... + ... | semmle.label | ... + ... |
| ActiveRecordInjection.rb:105:21:105:26 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:105:21:105: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:17:23:25 | condition | ActiveRecordInjection.rb:105:22:105:27 | call to params : | ActiveRecordInjection.rb:23:17:23:25 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:105:22:105:27 | call to params | a user-provided value |
| ActiveRecordInjection.rb:23:16:23:24 | condition | ActiveRecordInjection.rb:105:21:105:26 | call to params : | ActiveRecordInjection.rb:23:16:23:24 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:105:21:105: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:21:43:43 | "id = '#{...}'" | ActiveRecordInjection.rb:43:30:43:35 | call to params : | ActiveRecordInjection.rb:43:21:43:43 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:43:30:43:35 | call to params | a user-provided value |
| ActiveRecordInjection.rb:47:23:47:45 | "id = '#{...}'" | ActiveRecordInjection.rb:47:32:47:37 | call to params : | ActiveRecordInjection.rb:47:23:47:45 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:47:32:47:37 | 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 |
| 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 |
| 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:72:21:72:33 | ... + ... | ActiveRecordInjection.rb:66:10:66:15 | call to params : | ActiveRecordInjection.rb:72:21:72:33 | ... + ... | This SQL query depends on $@. | ActiveRecordInjection.rb:66:10:66:15 | call to params | a user-provided value |
| ActiveRecordInjection.rb:72:20:72:32 | ... + ... | ActiveRecordInjection.rb:66:10:66:15 | call to params : | ActiveRecordInjection.rb:72:20:72:32 | ... + ... | This SQL query depends on $@. | ActiveRecordInjection.rb:66:10:66:15 | call to params | a user-provided value |