Merge pull request #414 from presidentbeef/check_for_permit_bang

Check for uses of Parameters#permit!

lib/brakeman/checks/check_mass_assignment.rb

@@ -10,7 +10,12 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
   @description = "Finds instances of mass assignment"
 
   def run_check
-    return if mass_assign_disabled?
+    check_mass_assignment
+    check_permit!
+  end
+
+  def find_mass_assign_calls
+    return @mass_assign_calls if @mass_assign_calls
 
     models = []
     tracker.models.each do |name, m|
@@ -19,13 +24,12 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
       end
     end
 
-    return if models.empty?
-
+    return [] if models.empty?
 
     Brakeman.debug "Finding possible mass assignment calls on #{models.length} models"
-    calls = tracker.find_call :chained => true, :targets => models, :methods => [:new,
-      :attributes=, 
-      :update_attributes, 
+    @mass_assign_calls = tracker.find_call :chained => true, :targets => models, :methods => [:new,
+      :attributes=,
+      :update_attributes,
       :update_attributes!,
       :create,
       :create!,
@@ -36,9 +40,13 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
       :assign_attributes,
       :update
     ]
+  end
+
+  def check_mass_assignment
+    return if mass_assign_disabled?
 
     Brakeman.debug "Processing possible mass assignment calls"
-    calls.each do |result|
+    find_mass_assign_calls.each do |result|
       process_result result
     end
   end
@@ -78,12 +86,12 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
         confidence = CONFIDENCE[:low]
         user_input = nil
       end
-      
-      warn :result => res, 
-        :warning_type => "Mass Assignment", 
+
+      warn :result => res,
+        :warning_type => "Mass Assignment",
         :warning_code => :mass_assign_call,
         :message => "Unprotected mass assignment",
-        :code => call, 
+        :code => call,
         :user_input => user_input,
         :confidence => confidence
     end
@@ -140,4 +148,43 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
       true
     end
   end
+
+  # Look for and warn about uses of Parameters#permit! for mass assignment
+  def check_permit!
+    tracker.find_call(:method => :permit!).each do |result|
+      if params? result[:target]
+        warn_on_permit! result
+      end
+    end
+  end
+
+  # Look for actual use of params in mass assignment to avoid
+  # warning about uses of Parameters#permit! without any mass assignment
+  # or when mass assignment is restricted by model instead.
+  def subsequent_mass_assignment? result
+    location = result[:location]
+    line = result[:call].line
+    find_mass_assign_calls.any? do |call|
+      call[:location] == location and
+      params? call[:call].first_arg and
+      call[:call].line >= line
+    end
+  end
+
+  def warn_on_permit! result
+    return if duplicate? result or result[:call].original_line
+    add_result result
+
+    confidence = if subsequent_mass_assignment? result
+                   CONFIDENCE[:high]
+                 else
+                   CONFIDENCE[:med]
+                 end
+
+    warn :result => result,
+      :warning_type => "Mass Assignment",
+      :warning_code => :mass_assign_permit!,
+      :message => "Parameters should be whitelisted for mass assignment",
+      :confidence => confidence
+  end
 end

lib/brakeman/warning_codes.rb

@@ -70,6 +70,7 @@ module Brakeman::WarningCodes
     :CVE_2013_6416 => 67,
     :CVE_2013_6416_call => 68,
     :CVE_2013_6417 => 69,
+    :mass_assign_permit! => 70,
   }
 
   def self.code name

test/apps/rails4/app/controllers/friendly_controller.rb

@@ -16,4 +16,27 @@ class FriendlyController
     User.stuff.try(:where, params[:query])
     User.send(:from, params[:table]).all
   end
+
+  def mass_assign_user
+    # Should warn about permit!
+    x = params.permit!
+    @user = User.new(x)
+  end
+
+  def mass_assign_protected_model
+    # Warns with medium confidence because Account uses attr_accessible
+    params.permit!
+    Account.new(params)
+  end
+
+  def permit_without_usage
+    # Warns with medium confidence because there is no mass assignment
+    params.permit!
+  end
+
+  def permit_after_usage
+    # Warns with medium confidence because permit! is called after mass assignment
+    User.new(params)
+    params.permit!
+  end
 end

test/apps/rails4/app/models/account.rb

+class Account < ActiveRecord::Base
+  attr_accessible :name
+end

test/tests/rails4.rb

@@ -15,7 +15,7 @@ class Rails4Tests < Test::Unit::TestCase
       :controller => 0,
       :model => 0,
       :template => 1,
-      :generic => 9
+      :generic => 13
     }
   end
 
@@ -199,4 +199,46 @@ class Rails4Tests < Test::Unit::TestCase
       :relative_path => "Gemfile",
       :user_input => nil
   end
+
+  def test_mass_assignment_with_permit!
+    assert_warning :type => :warning,
+      :warning_code => 70,
+      :fingerprint => "c2fdd36441441ef7d2aed764731c36fb9f16939ed4df582705f27d46c02fcbe3",
+      :warning_type => "Mass Assignment",
+      :line => 22,
+      :message => /^Parameters\ should\ be\ whitelisted\ for\ mas/,
+      :confidence => 0,
+      :relative_path => "app/controllers/friendly_controller.rb",
+      :user_input => nil
+
+    assert_warning :type => :warning,
+      :warning_code => 70,
+      :fingerprint => "2f2df4aef71799a6a441783b50e7a43a9bed7da6c8d50e07e73d9d165065ceec",
+      :warning_type => "Mass Assignment",
+      :line => 28,
+      :message => /^Parameters\ should\ be\ whitelisted\ for\ mas/,
+      :confidence => 1,
+      :relative_path => "app/controllers/friendly_controller.rb",
+      :user_input => nil
+
+    assert_warning :type => :warning,
+      :warning_code => 70,
+      :fingerprint => "4f6a0d82f6ddf5528f3d50545ce353f2f1658d5102a745107ea572af5c2eee4b",
+      :warning_type => "Mass Assignment",
+      :line => 34,
+      :message => /^Parameters\ should\ be\ whitelisted\ for\ mas/,
+      :confidence => 1,
+      :relative_path => "app/controllers/friendly_controller.rb",
+      :user_input => nil
+
+    assert_warning :type => :warning,
+      :warning_code => 70,
+      :fingerprint => "947bddec4cdd3ff8b2485eec1bd0078352c182a3bca18a5f68da0a64e87d4e80",
+      :warning_type => "Mass Assignment",
+      :line => 40,
+      :message => /^Parameters\ should\ be\ whitelisted\ for\ mas/,
+      :confidence => 1,
+      :relative_path => "app/controllers/friendly_controller.rb",
+      :user_input => nil
+  end
 end