Merge pull request #1619 from presidentbeef/rails_6_sqli_more

SQL injection updates for Rails 6

lib/brakeman/checks/check_sql.rb

@@ -22,7 +22,19 @@ def run_check
                     :find_by_sql, :maximum, :minimum, :pluck, :sum, :update_all]
     @sql_targets.concat [:from, :group, :having, :joins, :lock, :order, :reorder, :where] if tracker.options[:rails3]
     @sql_targets.concat [:find_by, :find_by!, :find_or_create_by, :find_or_create_by!, :find_or_initialize_by, :not] if tracker.options[:rails4]
-    @sql_targets << :delete_by << :destroy_by if tracker.options[:rails6]
+
+    if tracker.options[:rails6]
+      @sql_targets.concat [:delete_by, :destroy_by, :rewhere, :reselect]
+
+      @sql_targets.delete :delete_all
+      @sql_targets.delete :destroy_all
+    end
+
+    if version_between?("6.1.0", "9.9.9")
+      @sql_targets.delete :order
+      @sql_targets.delete :reorder
+      @sql_targets.delete :pluck
+    end
 
     if version_between?("2.0.0", "3.9.9") or tracker.config.rails_version.nil?
       @sql_targets << :first << :last << :all
@@ -185,7 +197,7 @@ def process_result result
                         else
                           check_find_arguments call.last_arg
                         end
-                      when :where, :having, :find_by, :find_by!, :find_or_create_by, :find_or_create_by!, :find_or_initialize_by,:not, :delete_by, :destroy_by
+                      when :where, :rewhere, :having, :find_by, :find_by!, :find_or_create_by, :find_or_create_by!, :find_or_initialize_by,:not, :delete_by, :destroy_by
                         check_query_arguments call.arglist
                       when :order, :group, :reorder
                         check_order_arguments call.arglist
@@ -199,7 +211,7 @@ def process_result result
                         unsafe_sql? call.first_arg
                       when :sql
                         unsafe_sql? call.first_arg
-                      when :update_all, :select
+                      when :update_all, :select, :reselect
                         check_update_all_arguments call.args
                       when *@connection_calls
                         check_by_sql_arguments call.first_arg

test/apps/rails6/app/controllers/groups_controller.rb

@@ -61,4 +61,12 @@ def scope_with_custom_sanitization
   def sanitize_s(input)
     input
   end
+
+  def test_rails6_sqli
+    User.select("stuff").reselect(params[:columns])
+    User.where("x = 1").rewhere("x = #{params[:x]}")
+    User.pluck(params[:column]) # Warn in 6.0, not in 6.1
+    User.order("name #{params[:direction]}") # Warn in 6.0, not in 6.1
+    User.order(:name).reorder(params[:column]) # Warn in 6.0, not in 6.1
+  end
 end

test/tests/github_output.rb

@@ -6,7 +6,7 @@ def setup
   end
 
   def test_report_format
-    assert_equal 35, @@report.lines.count
+    assert_equal 40, @@report.lines.count, "Did you add vulnerabilities to the Rails 6 app? Update this test please!"
     @@report.lines.each do |line|
       assert line.start_with?('::'), 'Every line must start with `::`'
       assert_equal 2, line.scan('::').count, 'Every line must have exactly 2 `::`'

test/tests/rails6.rb

@@ -13,7 +13,7 @@ def expected
       :controller => 0,
       :model => 0,
       :template => 4,
-      :generic => 29
+      :generic => 34
     }
   end
 
@@ -186,6 +186,74 @@ def test_sql_injection_with_date
       :user_input => s(:call, s(:call, s(:const, :Date), :today), :-, s(:lit, 1))
   end
 
+  def test_sql_injection_rewhere
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "7f5154ba5124c5ae26ec23f364239311df959acb9b2e4d09f4867c2fbd954dd6",
+      :warning_type => "SQL Injection",
+      :line => 67,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/groups_controller.rb",
+      :code => s(:call, s(:call, s(:const, :User), :where, s(:str, "x = 1")), :rewhere, s(:dstr, "x = ", s(:evstr, s(:call, s(:params), :[], s(:lit, :x))))),
+      :user_input => s(:call, s(:params), :[], s(:lit, :x))
+  end
+
+  def test_sql_injection_reselect
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "e4fdd9614cff8e8f8a70cd983c55d36acd6da219048faf1530de9dc43d58aa71",
+      :warning_type => "SQL Injection",
+      :line => 66,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/groups_controller.rb",
+      :code => s(:call, s(:call, s(:const, :User), :select, s(:str, "stuff")), :reselect, s(:call, s(:params), :[], s(:lit, :columns))),
+      :user_input => s(:call, s(:params), :[], s(:lit, :columns))
+  end
+
+  def test_sql_injection_pluck
+    # Not in Rails 6.1 though
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "69a7e516b2b409dc8d74f6a26b44d62f4b842ce9c73e96c3910f9206c6fc50f5",
+      :warning_type => "SQL Injection",
+      :line => 68,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/groups_controller.rb",
+      :code => s(:call, s(:const, :User), :pluck, s(:call, s(:params), :[], s(:lit, :column))),
+      :user_input => s(:call, s(:params), :[], s(:lit, :column))
+  end
+
+  def test_sql_injection_order
+    # Not in Rails 6.1 though
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "47e9c6316ae9b2937121298ebc095bac4c4c8682779a0be95ce32c3fc4ba3118",
+      :warning_type => "SQL Injection",
+      :line => 69,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/groups_controller.rb",
+      :code => s(:call, s(:const, :User), :order, s(:dstr, "name ", s(:evstr, s(:call, s(:params), :[], s(:lit, :direction))))),
+      :user_input => s(:call, s(:params), :[], s(:lit, :direction))
+  end
+
+  def test_sql_injection_reorder
+    # Not in Rails 6.1 though
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "c6b303b67a5de261d9faaa84e02b29987b57fb443691d7ad77956bbecf41a1d0",
+      :warning_type => "SQL Injection",
+      :line => 70,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/groups_controller.rb",
+      :code => s(:call, s(:call, s(:const, :User), :order, s(:lit, :name)), :reorder, s(:call, s(:params), :[], s(:lit, :column))),
+      :user_input => s(:call, s(:params), :[], s(:lit, :column))
+  end
+
   def test_cross_site_scripting_sanity
     assert_warning :type => :template,
       :warning_code => 2,

test/tests/rails_61_sql.rb

@@ -0,0 +1,17 @@
+require_relative '../test'
+require 'brakeman/rescanner'
+
+class Rails61SQLTests < Minitest::Test
+  include BrakemanTester::RescanTestHelper
+
+  # Test that `pluck`/`order`/`reorder` no longer warn about SQL injection
+  # in Rails 6.1
+  def test_pluck_safe_in_rails_6_1
+    before_rescan_of ['app/controllers/groups_controller.rb', 'Gemfile'], 'rails6' do
+      replace "Gemfile", "gem 'rails', '~> 6.0.0.beta2'", "gem 'rails', '6.1.0'"
+    end
+
+    assert_equal '6.1.0', @rescanner.tracker.config.rails_version
+    assert_fixed 3
+  end
+end