Merge pull request #528 from presidentbeef/fix_sqli_detection_in_nested_string_building

Fix SQLi detection in deeply nested string building

lib/brakeman/checks/check_sql.rb

@@ -496,23 +496,32 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
     arg = exp.first_arg
 
     if STRING_METHODS.include? method
-      if string? target
-        check_string_arg arg
-      elsif string? arg
-        check_string_arg target
-      elsif call? target
-        check_for_string_building target
-      elsif node_type? target, :string_interp, :dstr or
-            node_type? arg, :string_interp, :dstr
-
-        check_string_arg target and
-        check_string_arg arg
-      end
+      check_str_target_or_arg(target, arg) or
+      check_interp_target_or_arg(target, arg) or
+      check_for_string_building(target) or
+      check_for_string_building(arg)
     else
       nil
     end
   end
 
+  def check_str_target_or_arg target, arg
+    if string? target
+      check_string_arg arg
+    elsif string? arg
+      check_string_arg target
+    end
+  end
+
+  def check_interp_target_or_arg target, arg
+    if node_type? target, :string_interp, :dstr or
+      node_type? arg, :string_interp, :dstr
+
+      check_string_arg target and
+      check_string_arg arg
+    end
+  end
+
   def check_string_arg exp
     if safe_value? exp
       nil

test/apps/rails4/app/models/account.rb

@@ -17,4 +17,20 @@ class Account < ActiveRecord::Base
     connection.select_one "SELECT * FROM somewhere WHERE x=#{connection.quote(params[:x])}"
     connection.execute "DELETE FROM stuff WHERE id=#{self.id}"
   end
+
+  def lots_of_string_building_sql
+    sql =
+      'SELECT count(*) as account_count, '+
+      'FROM account_links stuff_links '+
+      "WHERE account_links.stuff_id = #{@stuff.id} "
+
+    if params[:more_ids]
+      sql += " AND stuff IN "+
+        "(SELECT something_id "+
+        "FROM some_join_thing "+
+        "WHERE something_id IN (#{params[:more_ids].join(',')}))"
+    end
+    sql += "GROUP BY title, id "
+    Account.connection.select_all(sql)
+  end
 end

test/tests/rails4.rb

@@ -15,7 +15,7 @@ class Rails4Tests < Test::Unit::TestCase
       :controller => 0,
       :model => 1,
       :template => 2,
-      :generic => 31
+      :generic => 32
     }
   end
 
@@ -440,6 +440,18 @@ class Rails4Tests < Test::Unit::TestCase
       :user_input => nil
   end
 
+  def test_sql_injection_in_chained_string_building
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "6109b24aecaa204463bcebeb594becfe06d5f3c40cfe092b54a4c5146273e134",
+      :warning_type => "SQL Injection",
+      :line => 34,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/models/account.rb",
+      :user_input => s(:call, s(:call, s(:params), :[], s(:lit, :more_ids)), :join, s(:str, ","))
+  end
+
   def test_sql_injection_CVE_2013_6417
     assert_warning :type => :warning,
       :warning_code => 69,