diff --git a/lib/brakeman/checks/check_model_attr_accessible.rb b/lib/brakeman/checks/check_model_attr_accessible.rb index af655bbf6e9bba25bc7f8e271876c7c98f4a98e2..7825f7041ca1eccc486cbd21e6acb6c2a411fa7f 100644 --- a/lib/brakeman/checks/check_model_attr_accessible.rb +++ b/lib/brakeman/checks/check_model_attr_accessible.rb @@ -1,22 +1,22 @@ require 'brakeman/checks/base_check' -# Author: Paul Deardorff (themetric) -# Checks models to see if important foreign keys -# or attributes are exposed as attr_accessible when -# they probably shouldn't be. +# Author: Paul Deardorff (themetric) +# Checks models to see if important foreign keys +# or attributes are exposed as attr_accessible when +# they probably shouldn't be. class Brakeman::CheckModelAttrAccessible < Brakeman::BaseCheck Brakeman::Checks.add self @description = "Reports models which have dangerous attributes defined under the attr_accessible whitelist." - SUSP_ATTRS = { - /admin/ => CONFIDENCE[:high], # Very dangerous unless some Rails authorization used - /role/ => CONFIDENCE[:med], - /banned/ => CONFIDENCE[:med], - :account_id => CONFIDENCE[:high], - /\S*_id(s?)\z/ => CONFIDENCE[:low] # All other foreign keys have weak/low confidence - } + SUSP_ATTRS = [ + [/admin/, CONFIDENCE[:high]], # Very dangerous unless some Rails authorization used + [/role/, CONFIDENCE[:med]], + [/banned/, CONFIDENCE[:med]], + [:account_id, CONFIDENCE[:high]], + [/\S*_id(s?)\z/, CONFIDENCE[:low]] # All other foreign keys have weak/low confidence + ] def run_check check_models do |name, model| @@ -24,17 +24,17 @@ class Brakeman::CheckModelAttrAccessible < Brakeman::BaseCheck next if role_limited? model, attribute SUSP_ATTRS.each do |susp_attr, confidence| - if susp_attr.is_a?(Regexp) and susp_attr =~ attribute.to_s or susp_attr == attribute - warn :model => name, - :file => model[:file], - :warning_type => "Mass Assignment", - :warning_code => :mass_assign_call, - :message => "Potentially dangerous attribute #{attribute} available for mass assignment.", - :confidence => confidence - break # Prevent from matching single attr multiple times - end - end - end + if susp_attr.is_a?(Regexp) and susp_attr =~ attribute.to_s or susp_attr == attribute + warn :model => name, + :file => model[:file], + :warning_type => "Mass Assignment", + :warning_code => :dangerous_attr_accessible, + :message => "Potentially dangerous attribute #{attribute} available for mass assignment.", + :confidence => confidence + break # Prevent from matching single attr multiple times + end + end + end end end diff --git a/lib/brakeman/warning_codes.rb b/lib/brakeman/warning_codes.rb index a8332d1ac8d8c23d482c413846c65aa806e4cb9f..8e11f16834a885c0763c25615e0566127e6cf053 100644 --- a/lib/brakeman/warning_codes.rb +++ b/lib/brakeman/warning_codes.rb @@ -59,7 +59,8 @@ module Brakeman::WarningCodes :CVE_2013_1855 => 56, :CVE_2013_1856 => 57, :CVE_2013_1857 => 58, - :unsafe_symbol_creation => 59 + :unsafe_symbol_creation => 59, + :dangerous_attr_accessible => 60 } def self.code name diff --git a/test/tests/rails32.rb b/test/tests/rails32.rb index 5edfdb656735cf85f07d9b6c9f0f817d0af4ec7e..c8db6471abd124de2cdc5f0f050258d88bdbab63 100644 --- a/test/tests/rails32.rb +++ b/test/tests/rails32.rb @@ -215,6 +215,7 @@ class Rails32Tests < Test::Unit::TestCase def test_model_attr_accessible_admin assert_warning :type => :model, + :warning_code => 60, :warning_type => "Mass Assignment", :message => /^Potentially\ dangerous\ attribute\ admin/, :confidence => 0, #HIGH @@ -223,14 +224,17 @@ class Rails32Tests < Test::Unit::TestCase def test_model_attr_accessible_account_id assert_warning :type => :model, + :warning_code => 60, + :fingerprint => "1d6615676c39afae6d749891e45d7351423542b3fe71a6eaf088bf7573e5c4b0", :warning_type => "Mass Assignment", - :message => /^Potentially\ dangerous\ attribute\ account_id/, - :confidence => 0, - :file => /user\.rb/ + :message => /^Potentially\ dangerous\ attribute\ account_/, + :confidence => 0, + :relative_path => "app/models/user.rb" end def test_model_attr_accessible_account_banned assert_warning :type => :model, + :warning_code => 60, :warning_type => "Mass Assignment", :message => /^Potentially\ dangerous\ attribute\ banned/, :confidence => 1, #MED @@ -239,6 +243,7 @@ class Rails32Tests < Test::Unit::TestCase def test_model_attr_accessible_status_id assert_warning :type => :model, + :warning_code => 60, :warning_type => "Mass Assignment", :message => /^Potentially\ dangerous\ attribute\ status_id/, :confidence => 2, #LOW