Merge pull request #16185 from alexrford/rb/conditions-arr0

Ruby: ActiveRecord - refine `conditions` argument as an SQLi sink

Author: alexrford

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

@@ -195,7 +195,14 @@ private predicate sqlFragmentArgumentInner(DataFlow::CallNode call, DataFlow::No
   or
   // This format was supported until Rails 2.3.8
   call = activeRecordQueryBuilderCall(["all", "find", "first", "last"]) and
-  sink = call.getKeywordArgument("conditions")
+  exists(DataFlow::LocalSourceNode sn |
+    sn = call.getKeywordArgument("conditions").getALocalSource()
+  |
+    sink = sn.(DataFlow::ArrayLiteralNode).getElement(0)
+    or
+    sn.(DataFlow::LiteralNode).asLiteralAstNode() instanceof StringlikeLiteral and
+    sink = sn
+  )
   or
   call = activeRecordQueryBuilderCall("reload") and
   sink = call.getKeywordArgument("lock")

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

@@ -8,6 +8,13 @@ class User < ApplicationRecord
   def self.authenticate(name, pass)
     # BAD: possible untrusted input interpolated into SQL fragment
     find(:first, :conditions => "name='#{name}' and pass='#{pass}'")
+    # BAD: interpolation in array argument
+    find(:first, conditions: ["name='#{name}' and pass='#{pass}'"])
+    # GOOD: using SQL parameters
+    find(:first, conditions: ["name = ? and pass = ?", name, pass])
+    # BAD: interpolation with flow
+    conds = "name=#{name}"
+    find(:first, conditions: conds)
   end
 
   def self.from(user_group_id)
@@ -117,7 +124,7 @@ def some_request_handler
 
     # BAD: executes `SELECT users.* FROM #{params[:tab]}`
     # where `params[:tab]` is unsanitized
-    User.all.from(params[:tab]) 
+    User.all.from(params[:tab])
     # BAD: executes `SELECT "users".* FROM (SELECT "users".* FROM "users") #{params[:sq]}
     User.all.from(User.all, params[:sq])
   end