diff --git a/lib/brakeman/checks/base_check.rb b/lib/brakeman/checks/base_check.rb index 48cb86f00bda70691b22e2f4234d1583a6a3eac4..c1856888878a547dce28707ffdc114f032ec94ec 100644 --- a/lib/brakeman/checks/base_check.rb +++ b/lib/brakeman/checks/base_check.rb @@ -181,9 +181,30 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor #May need to revisit dependng on what Rails 4 actually does/has @mass_assign_disabled = true else - matches = tracker.check_initializers(:"ActiveRecord::Base", :send) + #Check for ActiveRecord::Base.send(:attr_accessible, nil) + tracker.check_initializers(:"ActiveRecord::Base", :attr_accessible).each do |result| + call = result.call + if call? call + if call.first_arg == Sexp.new(:nil) + @mass_assign_disabled = true + break + end + end + end - if matches.empty? + unless @mass_assign_disabled + tracker.check_initializers(:"ActiveRecord::Base", :send).each do |result| + call = result.call + if call? call + if call.first_arg == Sexp.new(:lit, :attr_accessible) and call.second_arg == Sexp.new(:nil) + @mass_assign_disabled = true + break + end + end + end + end + + unless @mass_assign_disabled #Check for # class ActiveRecord::Base # attr_accessible nil @@ -200,17 +221,6 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor end end end - else - #Check for ActiveRecord::Base.send(:attr_accessible, nil) - matches.each do |result| - call = result.call - if call? call - if call.first_arg == Sexp.new(:lit, :attr_accessible) and call.second_arg == Sexp.new(:nil) - @mass_assign_disabled = true - break - end - end - end end end @@ -229,10 +239,11 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor end unless @mass_assign_disabled - matches = tracker.check_initializers(:"ActiveRecord::Base", :send) + matches = tracker.check_initializers(:"ActiveRecord::Base", [:send, :include]) matches.each do |result| - if call? result.call and result.call.second_arg == forbidden_protection + call = result.call + if call? call and (call.first_arg == forbidden_protection or call.second_arg == forbidden_protection) @mass_assign_disabled = true end end diff --git a/lib/brakeman/processors/alias_processor.rb b/lib/brakeman/processors/alias_processor.rb index 97bde7986aff3f4398dd887e7c4a39647de9b316..87989338b2c0576a345b105d6e0227b68391c355 100644 --- a/lib/brakeman/processors/alias_processor.rb +++ b/lib/brakeman/processors/alias_processor.rb @@ -88,6 +88,10 @@ class Brakeman::AliasProcessor < Brakeman::SexpProcessor method = exp.method first_arg = exp.first_arg + if method == :send or method == :try + collapse_send_call exp, first_arg + end + if node_type? target, :or and [:+, :-, :*, :/].include? method res = process_or_simple_operation(exp) return res if res @@ -543,6 +547,17 @@ class Brakeman::AliasProcessor < Brakeman::SexpProcessor end end + # Change x.send(:y, 1) to x.y(1) + def collapse_send_call exp, first_arg + return unless symbol? first_arg or string? first_arg + exp.method = first_arg.value.to_sym + args = exp.args + exp.pop # remove last arg + if args.length > 1 + exp.arglist = args[1..-1] + end + end + #Returns a new SexpProcessor::Environment containing only instance variables. #This is useful, for example, when processing views. def only_ivars include_request_vars = false, lenv = nil diff --git a/lib/ruby_parser/bm_sexp.rb b/lib/ruby_parser/bm_sexp.rb index 0b67855773977d17795a1b1bae271e4f5ab4c486..e855398fd88bd409058684ca86707acf355e7d15 100644 --- a/lib/ruby_parser/bm_sexp.rb +++ b/lib/ruby_parser/bm_sexp.rb @@ -163,6 +163,12 @@ class Sexp end end + def method= name + expect :call + + self[2] = name + end + #Sets the arglist in a method call. def arglist= exp expect :call, :attrasgn diff --git a/test/apps/rails4/app/controllers/friendly_controller.rb b/test/apps/rails4/app/controllers/friendly_controller.rb index 6ff2ebdda73f61d43f1d931f1089de05fd37fc41..09ae445ca9af39518771dc4db3c7a8d14cacf90a 100644 --- a/test/apps/rails4/app/controllers/friendly_controller.rb +++ b/test/apps/rails4/app/controllers/friendly_controller.rb @@ -11,4 +11,9 @@ class FriendlyController def some_user_thing redirect_to @user.url end + + def try_and_send + User.stuff.try(:where, params[:query]) + User.send(:from, params[:table]).all + end end diff --git a/test/tests/alias_processor.rb b/test/tests/alias_processor.rb index af18fc513fba7789aa3eead55051982882534127..35a91e49068a8dfa77649e65f99828bed879e94c 100644 --- a/test/tests/alias_processor.rb +++ b/test/tests/alias_processor.rb @@ -156,8 +156,29 @@ class AliasProcessorTests < Test::Unit::TestCase def test_addition_chained assert_alias 'y + 5', <<-RUBY - x = y + 2 + 3 - x + x = y + 2 + 3 + x + RUBY + end + + def test_send_collapse + assert_alias 'x.y(1)', <<-RUBY + z = x.send(:y, 1) + z + RUBY + end + + def test_send_collapse_with_no_target + assert_alias 'y(1)', <<-RUBY + x = send(:y, 1) + x + RUBY + end + + def test_try_collapse + assert_alias 'x.y', <<-RUBY + z = x.try(:y) + z RUBY end diff --git a/test/tests/rails4.rb b/test/tests/rails4.rb index e9db1ba062bd764aeebfa9ad278aa9ec925fb62a..70207d69d30bad82b983cc9e91f92f798bac1b4d 100644 --- a/test/tests/rails4.rb +++ b/test/tests/rails4.rb @@ -15,7 +15,7 @@ class Rails4Tests < Test::Unit::TestCase :controller => 0, :model => 0, :template => 0, - :generic => 3 + :generic => 5 } end @@ -121,4 +121,26 @@ class Rails4Tests < Test::Unit::TestCase :confidence => 0, :relative_path => "app/controllers/friendly_controller.rb" end + + def test_try_and_send_collapsing_with_sqli + assert_warning :type => :warning, + :warning_code => 0, + :fingerprint => "c96c2984c1ce4f9a0f1205c9e7ac4707253a0553ecb6c7e9d6d4b88c92db7098", + :warning_type => "SQL Injection", + :line => 17, + :message => /^Possible\ SQL\ injection/, + :confidence => 0, + :relative_path => "app/controllers/friendly_controller.rb", + :user_input => s(:call, s(:params), :[], s(:lit, :table)) + + assert_warning :type => :warning, + :warning_code => 0, + :fingerprint => "004e5d6afb7ce520f1a67b65ace238f763ca2feb6a7f552f7dcc86ed3f67a189", + :warning_type => "SQL Injection", + :line => 16, + :message => /^Possible\ SQL\ injection/, + :confidence => 0, + :relative_path => "app/controllers/friendly_controller.rb", + :user_input => s(:call, s(:params), :[], s(:lit, :query)) + end end diff --git a/test/tests/sexp.rb b/test/tests/sexp.rb index d4f23e245cab1dbb522b38943e988ce0a3227c84..f164ec0562008bf96d73f7b1ee35c6019c5b3603 100644 --- a/test/tests/sexp.rb +++ b/test/tests/sexp.rb @@ -76,6 +76,16 @@ class SexpTests < Test::Unit::TestCase assert_equal s(:lit, 2), exp.last_arg end + def test_method_call_set_method + exp = parse "x.y" + + assert_equal :y, exp.method + + exp.method = :z + + assert_equal :z, exp.method + end + def test_method_call_with_block exp = parse "x do |z|; blah z; end" block = exp.block