Merge pull request #319 from github/hmac-activerecord-updates

Add some more vulnerable ActiveRecord methods

Author: hmac

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

@@ -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
  * ```

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

@@ -68,16 +68,24 @@ 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_all", "delete_by", "destroy_all", "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", "rewhere", "select",
+          "reselect", "update_all"
         ] and
       result = call.getArgument(0)
       or
       methodName = "calculate" and result = call.getArgument(1)
       or
+      methodName in ["average", "count", "maximum", "minimum", "sum"] and
+      result = call.getArgument(0)
+      or
       // 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")
     )
   )
 }
@@ -118,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

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

@@ -13,8 +13,8 @@ actionControllerActionMethods
 | app/controllers/foo/bars_controller.rb:15:3:19:5 | show |
 paramsCalls
 | ActiveRecordInjection.rb:35:30:35:35 | call to params |
-| ActiveRecordInjection.rb:39:30:39:35 | call to params |
-| ActiveRecordInjection.rb:43:32:43:37 | 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 |
@@ -24,16 +24,16 @@ paramsCalls
 | 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:22:94:27 | call to params |
+| ActiveRecordInjection.rb:94:21:94:26 | call to params |
 | app/controllers/foo/bars_controller.rb:8:21:8:26 | call to params |
 | app/controllers/foo/bars_controller.rb:9:10:9:15 | call to params |
 | app/controllers/foo/bars_controller.rb:16:21:16:26 | call to params |
 | app/controllers/foo/bars_controller.rb:17:10:17: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:30:39:35 | call to params |
-| ActiveRecordInjection.rb:43:32:43:37 | 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 |
@@ -43,7 +43,7 @@ paramsSources
 | 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:22:94:27 | call to params |
+| ActiveRecordInjection.rb:94:21:94:26 | call to params |
 | app/controllers/foo/bars_controller.rb:8:21:8:26 | call to params |
 | app/controllers/foo/bars_controller.rb:9:10:9:15 | call to params |
 | app/controllers/foo/bars_controller.rb:16:21:16:26 | call to params |

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

@@ -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 |

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

@@ -17,10 +17,10 @@ def self.from(user_group_id)
 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 @@ def some_request_handler
 
     # 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 @@ def some_other_request_handler
 
     # 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 @@ def safe_paths
 
 class BazController < BarController
   def yet_another_handler
-    Admin.delete_all(params[:admin_condition])
+    Admin.delete_by(params[:admin_condition])
   end
 end

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

@@ -17,10 +17,10 @@ def self.from(user_group_id)
 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
 
@@ -34,12 +34,26 @@ def some_request_handler
     # where `params[:column]` is unsanitized
     User.calculate(:average, params[:column])
 
+    # BAD: executes `SELECT MAX(#{params[:column]}) FROM "users"`
+    # where `params[:column]` is unsanitized
+    User.maximum(params[:column])
+
+    # BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
+    # where `params[:id]` is unsanitized
+    User.delete_by("id = '#{params[:id]}'")
+
     # BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
     # where `params[:id]` is unsanitized
+    # (in Rails < 4.0)
     User.delete_all("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 = '#{params[:id]}')`
+    # where `params[:id]` is unsanitized
+    # (in Rails < 4.0)
     User.destroy_all(["id = '#{params[:id]}'"])
 
     # BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
@@ -54,6 +68,28 @@ def some_request_handler
     User.where.not("user.id = '#{params[:id]}'")
 
     User.authenticate(params[:name], params[:pass])
+
+    # 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
 
@@ -65,7 +101,7 @@ def some_other_request_handler
 
     # 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
@@ -98,6 +134,6 @@ def safe_paths
 
 class BazController < BarController
   def yet_another_handler
-    Admin.delete_all(params[:admin_condition])
+    Admin.delete_by(params[:admin_condition])
   end
 end