From 76419e62fc1020eaa11e1599f06f9e60582c48b5 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Sun, 11 Jul 2021 16:03:47 -0700 Subject: [PATCH 1/3] SQL Injection for Rails 6 --- lib/brakeman/checks/check_sql.rb | 18 ++++++++++-- .../app/controllers/groups_controller.rb | 5 ++++ test/tests/github_output.rb | 2 +- test/tests/rails6.rb | 28 ++++++++++++++++++- 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index 8b67e4694a..fd77ea95a6 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 + @sql_targets.delete :order + @sql_targets.delete :reorder + end + + if version_between?("6.1.0", "9.9.9") + @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..4b43e9eb39 100644 --- a/test/apps/rails6/app/controllers/groups_controller.rb +++ b/test/apps/rails6/app/controllers/groups_controller.rb @@ -61,4 +61,9 @@ 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]}") + end end diff --git a/test/tests/github_output.rb b/test/tests/github_output.rb index c57bf8d34b..003c653002 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 37, @@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..08e5e260d8 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 => 31 } end @@ -186,6 +186,32 @@ 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_cross_site_scripting_sanity assert_warning :type => :template, :warning_code => 2, From 26aa5eabb82aeab5185cae03c73c730f63f628f1 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Sun, 11 Jul 2021 22:07:35 -0700 Subject: [PATCH 2/3] Add test for `pluck` --- .../rails6/app/controllers/groups_controller.rb | 1 + test/tests/github_output.rb | 2 +- test/tests/pluck.rb | 17 +++++++++++++++++ test/tests/rails6.rb | 15 ++++++++++++++- 4 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 test/tests/pluck.rb diff --git a/test/apps/rails6/app/controllers/groups_controller.rb b/test/apps/rails6/app/controllers/groups_controller.rb index 4b43e9eb39..d8be999334 100644 --- a/test/apps/rails6/app/controllers/groups_controller.rb +++ b/test/apps/rails6/app/controllers/groups_controller.rb @@ -65,5 +65,6 @@ def sanitize_s(input) 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 end end diff --git a/test/tests/github_output.rb b/test/tests/github_output.rb index 003c653002..c2334b7dc5 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 37, @@report.lines.count, "Did you add vulnerabilities to the Rails 6 app? Update this test please!" + assert_equal 38, @@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/pluck.rb b/test/tests/pluck.rb new file mode 100644 index 0000000000..c5ee5dfa76 --- /dev/null +++ b/test/tests/pluck.rb @@ -0,0 +1,17 @@ +require_relative '../test' +require 'brakeman/rescanner' + +class PluckTests < Minitest::Test + include BrakemanTester::RescanTestHelper + + # Test that `pluck` no longer warns 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 1 + end +end diff --git a/test/tests/rails6.rb b/test/tests/rails6.rb index 08e5e260d8..e32821801f 100644 --- a/test/tests/rails6.rb +++ b/test/tests/rails6.rb @@ -13,7 +13,7 @@ def expected :controller => 0, :model => 0, :template => 4, - :generic => 31 + :generic => 32 } end @@ -212,6 +212,19 @@ def test_sql_injection_reselect :user_input => s(:call, s(:params), :[], s(:lit, :columns)) end + def test_sql_injection_pluck + 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_cross_site_scripting_sanity assert_warning :type => :template, :warning_code => 2, From b00241ed9ecf15af569f1a884d3665424e7921e1 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Thu, 15 Jul 2021 12:22:48 -0700 Subject: [PATCH 3/3] Order/reorder changed in Rails 6.1 --- lib/brakeman/checks/check_sql.rb | 4 +-- .../app/controllers/groups_controller.rb | 2 ++ test/tests/github_output.rb | 2 +- test/tests/rails6.rb | 31 ++++++++++++++++++- test/tests/{pluck.rb => rails_61_sql.rb} | 6 ++-- 5 files changed, 38 insertions(+), 7 deletions(-) rename test/tests/{pluck.rb => rails_61_sql.rb} (75%) diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index fd77ea95a6..be0bc26b9f 100644 --- a/lib/brakeman/checks/check_sql.rb +++ b/lib/brakeman/checks/check_sql.rb @@ -28,11 +28,11 @@ def run_check @sql_targets.delete :delete_all @sql_targets.delete :destroy_all - @sql_targets.delete :order - @sql_targets.delete :reorder end if version_between?("6.1.0", "9.9.9") + @sql_targets.delete :order + @sql_targets.delete :reorder @sql_targets.delete :pluck end diff --git a/test/apps/rails6/app/controllers/groups_controller.rb b/test/apps/rails6/app/controllers/groups_controller.rb index d8be999334..2510140123 100644 --- a/test/apps/rails6/app/controllers/groups_controller.rb +++ b/test/apps/rails6/app/controllers/groups_controller.rb @@ -66,5 +66,7 @@ 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 c2334b7dc5..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 38, @@report.lines.count, "Did you add vulnerabilities to the Rails 6 app? Update this test please!" + 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 e32821801f..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 => 32 + :generic => 34 } end @@ -213,6 +213,7 @@ def test_sql_injection_reselect end def test_sql_injection_pluck + # Not in Rails 6.1 though assert_warning :type => :warning, :warning_code => 0, :fingerprint => "69a7e516b2b409dc8d74f6a26b44d62f4b842ce9c73e96c3910f9206c6fc50f5", @@ -225,6 +226,34 @@ def test_sql_injection_pluck :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/pluck.rb b/test/tests/rails_61_sql.rb similarity index 75% rename from test/tests/pluck.rb rename to test/tests/rails_61_sql.rb index c5ee5dfa76..e4493d7a69 100644 --- a/test/tests/pluck.rb +++ b/test/tests/rails_61_sql.rb @@ -1,10 +1,10 @@ require_relative '../test' require 'brakeman/rescanner' -class PluckTests < Minitest::Test +class Rails61SQLTests < Minitest::Test include BrakemanTester::RescanTestHelper - # Test that `pluck` no longer warns about SQL injection + # 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 @@ -12,6 +12,6 @@ def test_pluck_safe_in_rails_6_1 end assert_equal '6.1.0', @rescanner.tracker.config.rails_version - assert_fixed 1 + assert_fixed 3 end end