diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index 8b67e4694a..be0bc26b9f 100644 --- a/lib/brakeman/checks/check_sql.rb +++ b/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 diff --git a/test/apps/rails6/app/controllers/groups_controller.rb b/test/apps/rails6/app/controllers/groups_controller.rb index 3a1babc320..2510140123 100644 --- a/test/apps/rails6/app/controllers/groups_controller.rb +++ b/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 diff --git a/test/tests/github_output.rb b/test/tests/github_output.rb index c57bf8d34b..056bc86c80 100644 --- a/test/tests/github_output.rb +++ b/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 `::`' diff --git a/test/tests/rails6.rb b/test/tests/rails6.rb index bff45eddda..3323cd16e0 100644 --- a/test/tests/rails6.rb +++ b/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, diff --git a/test/tests/rails_61_sql.rb b/test/tests/rails_61_sql.rb new file mode 100644 index 0000000000..e4493d7a69 --- /dev/null +++ b/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