Merge remote-tracking branch 'themetric/check_model_attr_accessible'

Add check for potentially dangerous attributes in attr_accessible

CHANGES

 # Unreleased
 
+ * Check for dangerous model attributes defined in attr_accessible (Paul Deardorff)
  * Update to ruby_parser 3.2.2
  * Add brakeman-min gemspec
  * Load gem dependencies on-demand 

lib/brakeman/checks/check_model_attr_accessible.rb

+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. 
+
+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 
+  }
+
+  def run_check
+    check_models do |name, model|
+      accessible_attrs = model[:attr_accessible]
+      accessible_attrs.each do |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 
+    end
+  end
+
+  def check_models
+    tracker.models.each do |name, model|
+      if !model[:attr_accessible].nil?
+        yield name, model
+      end
+    end
+  end
+ 
+end

test/apps/rails3.2/app/models/account.rb

 class Account < ActiveRecord::Base
+  attr_accessible :plan_id, :banned 
 end

test/apps/rails3.2/app/models/no_protection.rb

+class NoProtection < ActiveRecord::Base
+  # Leave this class empty for Rescanner tests
+end

test/apps/rails3.2/app/models/user.rb

 class User < ActiveRecord::Base
-  attr_accessible :bio, :name
+  attr_accessible :bio, :name, :account_id, :admin, :status_id 
 end

test/tests/rails32.rb

@@ -9,7 +9,7 @@ class Rails32Tests < Test::Unit::TestCase
   def expected
     @expected ||= {
       :controller => 0,
-      :model => 0,
+      :model => 5,
       :template => 11,
       :warning => 7 }
 
@@ -212,4 +212,45 @@ class Rails32Tests < Test::Unit::TestCase
       :confidence => 0,
       :file => /secret_token\.rb/
   end
+
+  def test_model_attr_accessible_admin
+    assert_warning :type => :model,
+      :warning_type => "Mass Assignment",
+      :message => /^Potentially\ dangerous\ attribute\ admin/,
+      :confidence => 0, #HIGH
+      :file => /user\.rb/
+  end 
+
+  def test_model_attr_accessible_account_id
+    assert_warning :type => :model,
+      :warning_type => "Mass Assignment",
+      :message => /^Potentially\ dangerous\ attribute\ account_id/,
+      :confidence => 0, 
+      :file => /user\.rb/
+  end 
+
+  def test_model_attr_accessible_account_banned
+    assert_warning :type => :model,
+      :warning_type => "Mass Assignment",
+      :message => /^Potentially\ dangerous\ attribute\ banned/,
+      :confidence => 1, #MED
+      :file => /account\.rb/
+  end 
+
+  def test_model_attr_accessible_status_id
+    assert_warning :type => :model,
+      :warning_type => "Mass Assignment",
+      :message => /^Potentially\ dangerous\ attribute\ status_id/,
+      :confidence => 2, #LOW
+      :file => /user\.rb/
+  end 
+
+  def test_model_attr_accessible_plan_id
+    assert_warning :type => :model,
+      :warning_type => "Mass Assignment",
+      :message => /^Potentially\ dangerous\ attribute\ plan_id/,
+      :confidence => 2, 
+      :file => /account\.rb/
+  end 
+
 end

test/tests/rescanner.rb

@@ -151,7 +151,7 @@ class RescannerTests < Test::Unit::TestCase
     assert_reindex :templates, :models, :controllers
     assert_changes
     assert_new 7 #User is no longer a model, causing MORE warnings
-    assert_fixed 4
+    assert_fixed 7
   end
 
   def test_add_method_to_model