Merge branch 'master' into support_lts_versions

Conflicts:
	lib/brakeman/checks/check_sql.rb

lib/brakeman/checks/check_sql.rb

@@ -16,20 +16,32 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
   def run_check
     @rails_version = tracker.config[:rails_version]
 
-    @sql_targets = [:all, :average, :calculate, :count, :count_by_sql, :exists?,
+    @sql_targets = [:all, :average, :calculate, :count, :count_by_sql, :exists?, :delete_all, :destroy_all,
       :find, :find_by_sql, :first, :last, :maximum, :minimum, :pluck, :sum, :update_all]
     @sql_targets.concat [:from, :group, :having, :joins, :lock, :order, :reorder, :select, :where] if tracker.options[:rails3]
 
+    @connection_calls = [:delete, :execute, :insert, :select_all, :select_one,
+      :select_rows, :select_value, :select_values]
+
+    if tracker.options[:rails3]
+      @connection_calls.concat [:exec_delete, :exec_insert, :exec_query, :exec_update]
+    else
+      @connection_calls.concat [:add_limit!, :add_offset_limit!, :add_lock!]
+    end
+
     Brakeman.debug "Finding possible SQL calls on models"
     calls = tracker.find_call :targets => active_record_models.keys,
       :methods => @sql_targets,
       :chained => true
 
     Brakeman.debug "Finding possible SQL calls with no target"
-    calls.concat tracker.find_call(:target => nil, :method => @sql_targets)
+    calls.concat tracker.find_call(:target => nil, :methods => @sql_targets)
 
     Brakeman.debug "Finding possible SQL calls using constantized()"
-    calls.concat tracker.find_call(:method => @sql_targets).select { |result| constantize_call? result }
+    calls.concat tracker.find_call(:methods => @sql_targets).select { |result| constantize_call? result }
+
+    connect_targets = active_record_models.keys + [nil, :"ActiveRecord::Base"]
+    calls.concat tracker.find_call(:targets => connect_targets, :methods => @connection_calls, :chained => true).select { |result| connect_call? result }
 
     Brakeman.debug "Finding calls to named_scope or scope"
     calls.concat find_scope_calls
@@ -136,13 +148,14 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
     return if duplicate?(result) or result[:call].original_line
     return if result[:target].nil? && !active_record_models.include?(result[:location][:class])
 
+
     call = result[:call]
     method = call.method
 
     dangerous_value = case method
                       when :find
                         check_find_arguments call.second_arg
-                      when :exists?
+                      when :exists?, :delete_all, :destroy_all
                         check_find_arguments call.first_arg
                       when :named_scope, :scope
                         check_scope_arguments call
@@ -172,6 +185,8 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
                         unsafe_sql? call.first_arg
                       when :update_all
                         check_update_all_arguments call.args
+                      when *@connection_calls
+                        check_by_sql_arguments call.first_arg
                       else
                         Brakeman.debug "Unhandled SQL method: #{method}"
                       end
@@ -465,7 +480,8 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
     when :str, :lit, :const, :colon2, :nil, :true, :false
       true
     when :call
-      IGNORE_METHODS_IN_SQL.include? exp.method
+      IGNORE_METHODS_IN_SQL.include? exp.method or
+      quote_call? exp
     when :if
       safe_value? exp.then_clause and safe_value? exp.else_clause
     when :block, :rlist
@@ -477,6 +493,16 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
     end
   end
 
+  QUOTE_METHODS = [:quote, :quote_column_name, :quoted_date, :quote_string, :quote_table_name]
+
+  def quote_call? exp
+    if call? exp.target
+      exp.target.method == :connection and QUOTE_METHODS.include? exp.method
+    elsif exp.target.nil?
+      exp.method == :quote_value
+    end
+  end
+
   #Check call for string building
   def check_call exp
     return unless call? exp
@@ -522,6 +548,24 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
     call? call.target and call.target.method == :constantize
   end
 
+  SELF_CLASS = s(:call, s(:self), :class)
+
+  def connect_call? result
+    call = result[:call]
+    target = call.target
+
+    if call? target and target.method == :connection
+      target = target.target
+      klass = class_name(target)
+
+      target.nil? or
+      target == SELF_CLASS or
+      node_type? target, :self or
+      klass == :"ActiveRecord::Base" or
+      active_record_models.include? klass
+    end
+  end
+
   def upgrade_version? versions
     versions.each do |low, high, upgrade|
       return upgrade if version_between? low, high
@@ -530,7 +574,7 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
     false
   end
 
-def check_rails_versions_against_cve_issues
+  def check_rails_versions_against_cve_issues
     issues = [
       {
         :cve => "CVE-2012-2660",

test/apps/rails2/app/models/user.rb

@@ -21,4 +21,13 @@ class User < ActiveRecord::Base
     User.find(:all, :conditions => self.merge_conditions(some_conditions))
     find(:all, :conditions => User.merge_conditions(some_conditions))
   end
+
+  def self.some_method(value)
+    results = ActiveRecord::Base.connection.execute(%Q(SELECT
+        id
+      FROM
+        table t
+      WHERE
+        t.something = '#{value}'))
+  end
 end

test/apps/rails3/app/controllers/other_controller.rb

@@ -52,4 +52,9 @@ class OtherController < ApplicationController
   def test_mass_assign_with_strong_params
     Bill.create(params[:charge])
   end
+
+  def test_sql_deletes
+    User.delete_all("name = #{params[:name]}")
+    User.destroy_all("human = #{User.current.humanity}")
+  end
 end

test/apps/rails4/app/controllers/friendly_controller.rb

@@ -39,4 +39,18 @@ class FriendlyController
     User.new(params)
     params.permit!
   end
+
+  def sql_with_exec
+    User.connection.select_values <<-SQL
+      SELECT id FROM collection_items
+        WHERE id > #{last_collection_item.id}
+          AND collection_id IN (#{destinations.map { |d| d.id}.join(',')})"
+    SQL
+
+    Account.connection.select_rows("select thing.id, count(*) from things_stuff toc
+                                     join things dotoc on (toc.id=dotoc.toc_id)
+                                     join things do on (dotoc.data_object_id=do.id)
+                                     join thing_entries dohe on do.id = dohe.data_object_id
+                                     where do.published=#{params[:published]} and dohe.visibility_id=#{something.id} group by toc.id")
+  end
 end

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

 class Account < ActiveRecord::Base
   attr_accessible :name, :account_id, :admin
+
+  def sql_it_up_yeah
+    connection.exec_update "UPDATE `purchases` SET type = '#{self.type}' WHERE id = '#{self.id}'"
+
+    sql = "UPDATE #{self.class.table_name} SET latest_version = #{version} where id = #{self.id}"
+    self.class.connection.execute sql
+  end
+
+  def self.more_sql_connection
+    self.connection.exec_query "UPDATE `purchases` SET type = '#{self.type}' WHERE id = '#{self.id}'"
+  end
+
+  def safe_sql_should_not_warn
+    self.class.connection.execute "DESCRIBE  #{self.business_object.table_name}"
+    connection.select_one "SELECT * FROM somewhere WHERE x=#{connection.quote(params[:x])}"
+    connection.execute "DELETE FROM stuff WHERE id=#{self.id}"
+  end
 end

test/apps/rails_with_xss_plugin/app/controllers/users_controller.rb

@@ -129,4 +129,10 @@ class UsersController < ApplicationController
   def to_json
 
   end
+
+  def delete_them_all
+    if User.connection.select_value("SELECT * from users WHERE name='#{params[:name]}'").nil? #should warn
+      User.connection.execute("TRUNCATE users") #shouldn't warn
+    end
+  end
 end

test/tests/rails2.rb

@@ -12,13 +12,13 @@ class Rails2Tests < Test::Unit::TestCase
         :controller => 1,
         :model => 3,
         :template => 45,
-        :generic => 48 }
+        :generic => 49 }
     else
       @expected ||= {
         :controller => 1,
         :model => 3,
         :template => 45,
-        :generic => 49 }
+        :generic => 50 }
     end
   end
 
@@ -611,6 +611,18 @@ class Rails2Tests < Test::Unit::TestCase
       :file => /user\.rb/,
       :relative_path => "app/models/user.rb"
   end
+ 
+  def test_sql_injection_active_record_base_connection
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "37885d589fc5c41553dcc38b36b506c2e508d1f37ce040eb6dca92a958f858fb",
+      :warning_type => "SQL Injection",
+      :line => 26,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 1,
+      :relative_path => "app/models/user.rb",
+      :user_input => s(:lvar, :value)
+  end
 
   def test_escape_once
     results = find :type => :template,
@@ -1252,13 +1264,13 @@ class Rails2WithOptionsTests < Test::Unit::TestCase
         :controller => 1,
         :model => 4,
         :template => 45,
-        :generic => 48 }
+        :generic => 49 }
     else
       @expected ||= {
         :controller => 1,
         :model => 4,
         :template => 45,
-        :generic => 49 }
+        :generic => 50 }
     end
   end
 

test/tests/rails3.rb

@@ -16,7 +16,7 @@ class Rails3Tests < Test::Unit::TestCase
       :controller => 1,
       :model => 8,
       :template => 38,
-      :generic => 66
+      :generic => 68
     }
 
     if RUBY_PLATFORM == 'java'
@@ -671,6 +671,30 @@ class Rails3Tests < Test::Unit::TestCase
       :file => /underline_model\.rb/
   end
 
+  def test_sql_injection_delete_all
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "6ae0b599e368b7658cfe3772ab0823d68247796b3718eaa6c1228897d633e0a2",
+      :warning_type => "SQL Injection",
+      :line => 57,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/other_controller.rb",
+      :user_input => s(:call, s(:params), :[], s(:lit, :name))
+  end
+
+  def test_sql_injection_destroy_all
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "0631b0564dfe4bb760c250e1de7f0678dd28e5be5c54841fa8581ac3bf2ffaaf",
+      :warning_type => "SQL Injection",
+      :line => 58,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/other_controller.rb",
+      :user_input => s(:call, s(:call, s(:const, :User), :current), :humanity)
+  end
+
   def test_escape_once
     results = find :type => :template,
       :warning_type => "Cross Site Scripting",

test/tests/rails4.rb

@@ -15,7 +15,7 @@ class Rails4Tests < Test::Unit::TestCase
       :controller => 0,
       :model => 1,
       :template => 1,
-      :generic => 13
+      :generic => 18
     }
   end
 
@@ -144,6 +144,66 @@ class Rails4Tests < Test::Unit::TestCase
       :user_input => s(:call, s(:params), :[], s(:lit, :query))
   end
 
+  def test_sql_injection_connection_execute
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "4efbd2d2fc76d30296c8aa66368ddaf337b4c677778f36cddfa2290da2ec514b",
+      :warning_type => "SQL Injection",
+      :line => 8,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 1,
+      :relative_path => "app/models/account.rb",
+      :user_input => s(:call, nil, :version)
+  end
+
+  def test_sql_injection_select_rows
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "2e3c08dfb1e17f7d2e6ee5d142223477b85d27e6aa88d2d06cf0a00d04ed2d5c",
+      :warning_type => "SQL Injection",
+      :line => 50,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/friendly_controller.rb",
+      :user_input => s(:call, s(:params), :[], s(:lit, :published))
+  end
+
+  def test_sql_injection_select_values
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "3538776239f624a1101afe68b2e894424e8ae3f68222a6eec9fb4421d01cc557",
+      :warning_type => "SQL Injection",
+      :line => 44,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 1,
+      :relative_path => "app/controllers/friendly_controller.rb",
+      :user_input => s(:call, s(:call_with_block, s(:call, s(:call, nil, :destinations), :map), s(:args, :d), s(:call, s(:lvar, :d), :id)), :join, s(:str, ","))
+  end
+
+  def test_sql_injection_exec_query
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "59be915e75d6eb88def8fccebae1f9930bb6e50b2e598c7f04bf98c7a3235219",
+      :warning_type => "SQL Injection",
+      :line => 12,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 1,
+      :relative_path => "app/models/account.rb",
+      :user_input => s(:call, s(:self), :type)
+  end
+
+  def test_sql_injection_exec_update
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "29fac7fc616f19edf59cc230f7a86292d6c69234b5879868eaf1d954f033974f",
+      :warning_type => "SQL Injection",
+      :line => 5,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 1,
+      :relative_path => "app/models/account.rb",
+      :user_input => s(:call, s(:self), :type)
+  end
+
   def test_i18n_xss_CVE_2013_4491_workaround
     assert_no_warning :type => :warning,
       :warning_code => 63,

test/tests/rails_with_xss_plugin.rb

@@ -11,7 +11,7 @@ class RailsWithXssPluginTests < Test::Unit::TestCase
       :controller => 1,
       :model => 3,
       :template => 2,
-      :generic => 22 }
+      :generic => 23 }
   end
 
   def report
@@ -210,6 +210,17 @@ class RailsWithXssPluginTests < Test::Unit::TestCase
       :file => /results\.html\.erb/
   end
 
+  def test_sql_injection_select_value
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "e725c387439202f28c1983bf225323d93b5891695c91b9389740e2da3d74855e",
+      :warning_type => "SQL Injection",
+      :line => 134,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/users_controller.rb",
+      :user_input => s(:call, s(:params), :[], s(:lit, :name))
+  end
 
   def test_cross_site_request_forgery_18 
     assert_warning :type => :controller,

test/tests/tabs_output.rb

@@ -3,9 +3,9 @@ class TestTabsOutput < Test::Unit::TestCase
 
   def test_reported_warnings
     if Brakeman::Scanner::RUBY_1_9
-      assert_equal 98, Report.lines.to_a.count
-    else
       assert_equal 99, Report.lines.to_a.count
+    else
+      assert_equal 100, Report.lines.to_a.count
     end
   end
 end