Merge pull request #15671 from joefarebrother/ruby-activerecord-extra-args

Ruby: Consider additional arguments to certain `ActiveRecord` methods as sql injection sinks.

Author: joefarebrother

ruby/ql/lib/change-notes/2024-02-20-activerecord-sql-sink-arguments.md

@@ -0,0 +1,5 @@
+
+---
+category: minorAnalysis
+---
+* Additional arguments beyond the first of calls to the  `ActiveRecord` methods `select`, `reselect`, `order`, `reorder`, `joins`, `group`, and `pluck` are now recognized as sql injection sinks. 

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

@@ -176,11 +176,16 @@ private predicate sqlFragmentArgumentInner(DataFlow::CallNode call, DataFlow::No
     activeRecordQueryBuilderCall([
         "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", "reorder", "pluck", "where", "rewhere",
-        "select", "reselect"
+        "having", "lock", "not", "where", "rewhere"
       ]) and
   sink = call.getArgument(0)
   or
+  call =
+    activeRecordQueryBuilderCall([
+        "group", "joins", "order", "reorder", "pluck", "select", "reselect"
+      ]) and
+  sink = call.getArgument(_)
+  or
   call = activeRecordQueryBuilderCall("calculate") and
   sink = call.getArgument(1)
   or

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

@@ -105,6 +105,14 @@ def some_request_handler
 
     User.reorder(params[:direction])
 
+    User.select('a','b', params[:column])
+    User.reselect('a','b', params[:column])
+    User.order('a ASC', "b #{params[:direction]}")
+    User.reorder('a ASC', "b #{params[:direction]}")
+    User.group('a', params[:column])
+    User.pluck('a', params[:column])
+    User.joins(:a, params[:column])
+
     User.count_by_sql(params[:custom_sql_query])
   end
 end