hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-28 18:25:24 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #9566 from alexrford/ruby/activerecord-findby-dynamic

Browse Source 
Ruby: recognize ActiveRecord `find_by_x` methods
This commit is contained in:
Alex Ford
2022-06-17 09:39:46 +01:00
committed by GitHub
parent cac53b5163 c44a68613a
commit 5923eb4962
4 changed files with 101 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -240,7 +240,7 @@ abstract class ActiveRecordModelInstantiation extends OrmInstantiation::Range,
// Names of class methods on ActiveRecord models that may return one or more
// instances of that model. This also includes the `initialize` method.
// See https://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html
private string finderMethodName() {
private string staticFinderMethodName() {
exists(string baseName |
baseName =
[
@@ -287,7 +287,12 @@ private class ActiveRecordModelFinderCall extends ActiveRecordModelInstantiation
callScope = cls.getAMethod()
)
) and
call.getMethodName() = finderMethodName()
(
call.getMethodName() = staticFinderMethodName()
or
// dynamically generated finder methods
call.getMethodName().indexOf("find_by_") = 0
)
)
}

66
ruby/ql/test/library-tests/frameworks/ActionController.expected
View File

@@ -1,17 +1,17 @@
actionControllerControllerClasses
| ActiveRecordInjection.rb:27:1:58:3 | FooController |
| ActiveRecordInjection.rb:60:1:90:3 | BarController |
| ActiveRecordInjection.rb:92:1:96:3 | BazController |
| ActiveRecord.rb:23:1:39:3 | FooController |
| ActiveRecord.rb:41:1:64:3 | BarController |
| ActiveRecord.rb:66:1:70:3 | BazController |
| app/controllers/comments_controller.rb:1:1:7:3 | CommentsController |
| app/controllers/foo/bars_controller.rb:3:1:31:3 | BarsController |
| app/controllers/photos_controller.rb:1:1:4:3 | PhotosController |
| app/controllers/posts_controller.rb:1:1:10:3 | PostsController |
| app/controllers/users/notifications_controller.rb:2:3:5:5 | NotificationsController |
actionControllerActionMethods
| ActiveRecordInjection.rb:32:3:57:5 | some_request_handler |
| ActiveRecordInjection.rb:61:3:69:5 | some_other_request_handler |
| ActiveRecordInjection.rb:71:3:89:5 | safe_paths |
| ActiveRecordInjection.rb:93:3:95:5 | yet_another_handler |
| ActiveRecord.rb:27:3:38:5 | some_request_handler |
| ActiveRecord.rb:42:3:47:5 | some_other_request_handler |
| ActiveRecord.rb:49:3:63:5 | safe_paths |
| ActiveRecord.rb:67:3:69:5 | yet_another_handler |
| app/controllers/comments_controller.rb:2:3:3:5 | index |
| app/controllers/comments_controller.rb:5:3:6:5 | show |
| app/controllers/foo/bars_controller.rb:5:3:7:5 | index |
@@ -23,38 +23,38 @@ actionControllerActionMethods
| app/controllers/posts_controller.rb:8:3:9:5 | upvote |
| app/controllers/users/notifications_controller.rb:3:5:4:7 | mark_as_read |
paramsCalls
| ActiveRecordInjection.rb:35:30:35:35 | call to params |
| ActiveRecordInjection.rb:39:29:39:34 | call to params |
| ActiveRecordInjection.rb:43:31:43:36 | call to params |
| ActiveRecordInjection.rb:48:21:48:26 | call to params |
| ActiveRecordInjection.rb:54:34:54:39 | call to params |
| ActiveRecordInjection.rb:56:23:56:28 | call to params |
| ActiveRecordInjection.rb:56:38:56:43 | call to params |
| ActiveRecordInjection.rb:62:10:62:15 | call to params |
| ActiveRecordInjection.rb:72:11:72:16 | call to params |
| ActiveRecordInjection.rb:77:12:77:17 | call to params |
| ActiveRecordInjection.rb:83:12:83:17 | call to params |
| ActiveRecordInjection.rb:88:15:88:20 | call to params |
| ActiveRecordInjection.rb:94:21:94:26 | call to params |
| ActiveRecord.rb:28:30:28:35 | call to params |
| ActiveRecord.rb:29:29:29:34 | call to params |
| ActiveRecord.rb:30:31:30:36 | call to params |
| ActiveRecord.rb:32:21:32:26 | call to params |
| ActiveRecord.rb:34:34:34:39 | call to params |
| ActiveRecord.rb:35:23:35:28 | call to params |
| ActiveRecord.rb:35:38:35:43 | call to params |
| ActiveRecord.rb:43:10:43:15 | call to params |
| ActiveRecord.rb:50:11:50:16 | call to params |
| ActiveRecord.rb:54:12:54:17 | call to params |
| ActiveRecord.rb:59:12:59:17 | call to params |
| ActiveRecord.rb:62:15:62:20 | call to params |
| ActiveRecord.rb:68:21:68:26 | call to params |
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
| app/controllers/foo/bars_controller.rb:22:10:22:15 | call to params |
| app/views/foo/bars/show.html.erb:5:9:5:14 | call to params |
paramsSources
| ActiveRecordInjection.rb:35:30:35:35 | call to params |
| ActiveRecordInjection.rb:39:29:39:34 | call to params |
| ActiveRecordInjection.rb:43:31:43:36 | call to params |
| ActiveRecordInjection.rb:48:21:48:26 | call to params |
| ActiveRecordInjection.rb:54:34:54:39 | call to params |
| ActiveRecordInjection.rb:56:23:56:28 | call to params |
| ActiveRecordInjection.rb:56:38:56:43 | call to params |
| ActiveRecordInjection.rb:62:10:62:15 | call to params |
| ActiveRecordInjection.rb:72:11:72:16 | call to params |
| ActiveRecordInjection.rb:77:12:77:17 | call to params |
| ActiveRecordInjection.rb:83:12:83:17 | call to params |
| ActiveRecordInjection.rb:88:15:88:20 | call to params |
| ActiveRecordInjection.rb:94:21:94:26 | call to params |
| ActiveRecord.rb:28:30:28:35 | call to params |
| ActiveRecord.rb:29:29:29:34 | call to params |
| ActiveRecord.rb:30:31:30:36 | call to params |
| ActiveRecord.rb:32:21:32:26 | call to params |
| ActiveRecord.rb:34:34:34:39 | call to params |
| ActiveRecord.rb:35:23:35:28 | call to params |
| ActiveRecord.rb:35:38:35:43 | call to params |
| ActiveRecord.rb:43:10:43:15 | call to params |
| ActiveRecord.rb:50:11:50:16 | call to params |
| ActiveRecord.rb:54:12:54:17 | call to params |
| ActiveRecord.rb:59:12:59:17 | call to params |
| ActiveRecord.rb:62:15:62:20 | call to params |
| ActiveRecord.rb:68:21:68:26 | call to params |
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |

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

@@ -1,60 +1,64 @@
activeRecordModelClasses
| ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
| ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:19:1:25:3 | Admin |
| ActiveRecord.rb:1:1:3:3 | UserGroup |
| ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:17:1:21:3 | Admin |
activeRecordInstances
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by |
| ActiveRecordInjection.rb:79:5:81:7 | if ... |
| ActiveRecordInjection.rb:79:43:80:40 | then ... |
| 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 |
| ActiveRecord.rb:9:5:9:68 | call to find |
| ActiveRecord.rb:13:5:13:40 | call to find_by |
| ActiveRecord.rb:36:5:36:30 | call to find_by_name |
| ActiveRecord.rb:55:5:57:7 | if ... |
| ActiveRecord.rb:55:43:56:40 | then ... |
| ActiveRecord.rb:56:7:56:40 | call to find_by |
| ActiveRecord.rb:60:5:60:33 | call to find_by |
| ActiveRecord.rb:62:5:62:34 | call to find |
activeRecordSqlExecutionRanges
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
| ActiveRecordInjection.rb:23:16:23:24 | condition |
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] |
| 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:20:68:32 | ... + ... |
| ActiveRecordInjection.rb:75:16:75:28 | "name #{...}" |
| ActiveRecordInjection.rb:80:20:80:39 | "username = #{...}" |
| ActiveRecord.rb:9:33:9:67 | "name='#{...}' and pass='#{...}'" |
| ActiveRecord.rb:19:16:19:24 | condition |
| ActiveRecord.rb:28:30:28:44 | ...[...] |
| ActiveRecord.rb:29:20:29:42 | "id = '#{...}'" |
| ActiveRecord.rb:30:22:30:44 | "id = '#{...}'" |
| ActiveRecord.rb:31:16:31:21 | <<-SQL |
| ActiveRecord.rb:34:20:34:47 | "user.id = '#{...}'" |
| ActiveRecord.rb:46:20:46:32 | ... + ... |
| ActiveRecord.rb:52:16:52:28 | "name #{...}" |
| ActiveRecord.rb:56:20:56:39 | "username = #{...}" |
activeRecordModelClassMethodCalls
| ActiveRecordInjection.rb:2:3:2:17 | call to has_many |
| ActiveRecordInjection.rb:6:3:6:24 | call to belongs_to |
| 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:25 | call to destroy_by |
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
| 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: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:45 | call to delete_by |
| ActiveRecord.rb:2:3:2:17 | call to has_many |
| ActiveRecord.rb:6:3:6:24 | call to belongs_to |
| ActiveRecord.rb:9:5:9:68 | call to find |
| ActiveRecord.rb:13:5:13:40 | call to find_by |
| ActiveRecord.rb:13:5:13:46 | call to users |
| ActiveRecord.rb:19:5:19:25 | call to destroy_by |
| ActiveRecord.rb:28:5:28:45 | call to calculate |
| ActiveRecord.rb:29:5:29:43 | call to delete_by |
| ActiveRecord.rb:30:5:30:46 | call to destroy_by |
| ActiveRecord.rb:31:5:31:35 | call to where |
| ActiveRecord.rb:34:5:34:14 | call to where |
| ActiveRecord.rb:34:5:34:48 | call to not |
| ActiveRecord.rb:35:5:35:51 | call to authenticate |
| ActiveRecord.rb:36:5:36:30 | call to find_by_name |
| ActiveRecord.rb:37:5:37:36 | call to not_a_find_by_method |
| ActiveRecord.rb:46:5:46:33 | call to delete_by |
| ActiveRecord.rb:52:5:52:29 | call to order |
| ActiveRecord.rb:56:7:56:40 | call to find_by |
| ActiveRecord.rb:60:5:60:33 | call to find_by |
| ActiveRecord.rb:62:5:62:34 | call to find |
| ActiveRecord.rb:68:5:68:45 | call to delete_by |
potentiallyUnsafeSqlExecutingMethodCall
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
| 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:33 | call to delete_by |
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
| ActiveRecord.rb:9:5:9:68 | call to find |
| ActiveRecord.rb:19:5:19:25 | call to destroy_by |
| ActiveRecord.rb:28:5:28:45 | call to calculate |
| ActiveRecord.rb:29:5:29:43 | call to delete_by |
| ActiveRecord.rb:30:5:30:46 | call to destroy_by |
| ActiveRecord.rb:31:5:31:35 | call to where |
| ActiveRecord.rb:34:5:34:48 | call to not |
| ActiveRecord.rb:46:5:46:33 | call to delete_by |
| ActiveRecord.rb:52:5:52:29 | call to order |
| ActiveRecord.rb:56:7:56:40 | call to find_by |
activeRecordModelInstantiations
| ActiveRecordInjection.rb:10:5:10:68 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by | ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecordInjection.rb:88:5:88:34 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |
| ActiveRecord.rb:9:5:9:68 | call to find | ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:13:5:13:40 | call to find_by | ActiveRecord.rb:1:1:3:3 | UserGroup |
| ActiveRecord.rb:36:5:36:30 | call to find_by_name | ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:56:7:56:40 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:60:5:60:33 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
| ActiveRecord.rb:62:5:62:34 | call to find | ActiveRecord.rb:5:1:15:3 | User |

32
ruby/ql/test/library-tests/frameworks/ActiveRecordInjection.rb → ruby/ql/test/library-tests/frameworks/ActiveRecord.rb
View File

@@ -6,20 +6,16 @@ class User < ApplicationRecord
belongs_to :user_group
def self.authenticate(name, pass)
# BAD: possible untrusted input interpolated into SQL fragment
find(:first, :conditions => "name='#{name}' and pass='#{pass}'")
end
def self.from(user_group_id)
# GOOD: `find_by` with hash argument
UserGroup.find_by(id: user_group_id).users
end
end
class Admin < User
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_by(condition)
end
end
@@ -28,32 +24,17 @@ class FooController < ActionController::Base
MAX_USER_ID = 100_000
# A string tainted by user input is inserted into an SQL query
def some_request_handler
# BAD: executes `SELECT AVG(#{params[:column]}) FROM "users"`
# where `params[:column]` is unsanitized
User.calculate(:average, params[:column])
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
# where `params[:id]` is unsanitized
User.delete_by("id = '#{params[:id]}'")
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
# where `params[:id]` is unsanitized
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
User.where(<<-SQL, MAX_USER_ID)
id BETWEEN '#{params[:min_id]}' AND ?
SQL
# BAD: chained method case
# executes `SELECT "users".* FROM "users" WHERE (NOT (user_id = 'params[:id]'))`
# where `params[:id]` is unsanitized
User.where.not("user.id = '#{params[:id]}'")
User.authenticate(params[:name], params[:pass])
User.find_by_name("alice")
User.not_a_find_by_method("bob")
end
end
@@ -62,29 +43,22 @@ class BarController < ApplicationController
ps = params
uid = ps[:id]
uidEq = "= '#{uid}'"
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
# where `uid` is unsantized
User.delete_by("id " + uidEq)
end
def safe_paths
dir = params[:order]
# GOOD: barrier guard prevents taint flow
dir = "DESC" unless dir == "ASC"
User.order("name #{dir}")
name = params[:user_name]
# GOOD: barrier guard prevents taint flow
if %w(alice bob charlie).include? name
User.find_by("username = #{name}")
end
name = params[:user_name]
# GOOD: hash arguments are sanitized by ActiveRecord
User.find_by(user_name: name)
# OK: `find` method is overridden in `User`
User.find(params[:user_group])
end
end
@@ -93,4 +67,4 @@ class BazController < BarController
def yet_another_handler
Admin.delete_by(params[:admin_condition])
end
end
end