Merge pull request #953 from presidentbeef/jsyeo-jsyeo-direct-subclas…

…s-actioncontroller

Check forgery setting in all direct subclasses of ActionController::Base (updated)

lib/brakeman/checks/check_forgery_setting.rb

@@ -7,41 +7,41 @@
 class Brakeman::CheckForgerySetting < Brakeman::BaseCheck
   Brakeman::Checks.add self
 
-  @description = "Verifies that protect_from_forgery is enabled in ApplicationController"
+  @description = "Verifies that protect_from_forgery is enabled in direct subclasses of ActionController::Base"
 
   def run_check
-    app_controller = tracker.controllers[:ApplicationController]
-    return unless app_controller and app_controller.ancestor? :"ActionController::Base"
-
-    if tracker.config.allow_forgery_protection?
-      csrf_warning :warning_code => :csrf_protection_disabled,
-        :message => "Forgery protection is disabled"
-
-    elsif app_controller and not app_controller.protect_from_forgery?
-      csrf_warning :warning_code => :csrf_protection_missing,
-        :message => "'protect_from_forgery' should be called in ApplicationController",
-        :line => app_controller.top_line
-
-    elsif version_between? "2.1.0", "2.3.10"
-      cve_2011_0447 "2.3.11"
-
-    elsif version_between? "3.0.0", "3.0.3"
-      cve_2011_0447 "3.0.4"
-
-    elsif version_between? "4.0.0", "100.0.0" and forgery_opts = app_controller.options[:protect_from_forgery]
-      unless forgery_opts.is_a?(Array) and sexp?(forgery_opts.first) and
+    tracker.controllers
+    .select { |_, controller| controller.parent == :"ActionController::Base" }
+    .each do |name, controller|
+      if controller and not controller.protect_from_forgery?
+        csrf_warning :controller => name,
+          :warning_code => :csrf_protection_missing,
+          :message => "'protect_from_forgery' should be called in #{name}",
+          :file => controller.file,
+          :line => controller.top_line
+      elsif version_between? "4.0.0", "100.0.0" and forgery_opts = controller.options[:protect_from_forgery]
+        unless forgery_opts.is_a?(Array) and sexp?(forgery_opts.first) and
           access_arg = hash_access(forgery_opts.first.first_arg, :with) and symbol? access_arg and
           access_arg.value == :exception
 
-        args = {
-          :warning_code => :csrf_not_protected_by_raising_exception,
-          :message => "protect_from_forgery should be configured with 'with: :exception'",
-          :confidence => CONFIDENCE[:med]
-        }
+          args = {
+            :controller => name,
+            :warning_type => "Cross-Site Request Forgery",
+            :warning_code => :csrf_not_protected_by_raising_exception,
+            :message => "protect_from_forgery should be configured with 'with: :exception'",
+            :confidence => CONFIDENCE[:med],
+            :file => controller.file
+          }
+
+          args.merge!(:code => forgery_opts.first) if forgery_opts.is_a?(Array)
 
-        args.merge!(:code => forgery_opts.first) if forgery_opts.is_a?(Array)
+          csrf_warning args
+        end
 
-        csrf_warning args
+      end
+
+      if controller.options[:protect_from_forgery]
+        check_cve_2011_0447
       end
     end
   end
@@ -50,14 +50,26 @@ def csrf_warning opts
     opts = {
       :controller => :ApplicationController,
       :warning_type => "Cross-Site Request Forgery",
-      :confidence => CONFIDENCE[:high],
-      :file => tracker.controllers[:ApplicationController].file
+      :confidence => CONFIDENCE[:high]
     }.merge opts
 
     warn opts
   end
 
-  def cve_2011_0447 new_version
+  def check_cve_2011_0447
+    @warned_cve_2011_0447 ||= false
+    return if @warned_cve_2011_0447
+
+    if version_between? "2.1.0", "2.3.10"
+      new_version = "2.3.11"
+    elsif version_between? "3.0.0", "3.0.3"
+      new_version = "3.0.4"
+    else
+      return
+    end
+
+    @warned_cve_2011_0447 = true # only warn once
+
     csrf_warning :warning_code => :CVE_2011_0447,
       :message => "CSRF protection is flawed in unpatched versions of Rails #{rails_version} (CVE-2011-0447). Upgrade to #{new_version} or apply patches as needed",
       :gem_info => gemfile_or_environment,

test/apps/rails4_with_engines/engines/user_removal/app/controllers/base_controller.rb

@@ -0,0 +1,3 @@
+class BaseController < ActionController::Base
+  # missing protect_from_forgery call
+end

test/tests/rails3.rb

@@ -3,7 +3,7 @@
 class Rails3Tests < Minitest::Test
   include BrakemanTester::FindWarning
   include BrakemanTester::CheckExpected
-  
+
   def report
     @@report ||= BrakemanTester.run_scan "rails3", "Rails 3", :rails3 => true,
       :config_file => File.join(TEST_PATH, "apps", "rails3", "config", "brakeman.yml")
@@ -385,10 +385,14 @@ def test_sql_injection_non_active_record_model
 
   def test_csrf_protection
     assert_warning :type => :controller,
+      :warning_code => 7,
+      :fingerprint => "6f5239fb87c64764d0c209014deb5cf504c2c10ee424bd33590f0a4f22e01d8f",
       :warning_type => "Cross-Site Request Forgery",
-      :message => /^'protect_from_forgery' should be called /,
+      :line => 1,
+      :message => /^'protect_from_forgery'\ should\ be\ called\ /,
       :confidence => 0,
-      :file => /application_controller\.rb/
+      :relative_path => "app/controllers/application_controller.rb",
+      :user_input => nil
   end
 
   def test_attribute_restriction
@@ -538,30 +542,30 @@ def test_encoded_href_parameter_in_link_to
       :message => /^Unsafe parameter value in link_to href/,
       :confidence => 0,
       :file => /test_params\.html\.erb/
-  end  
- 
+  end
+
   def test_href_parameter_in_link_to
     assert_warning :type => :template,
       :warning_type => "Cross Site Scripting",
       :line => 14,
       :message => /^Unsafe parameter value in link_to href/,
       :confidence => 0,
       :file => /test_params\.html\.erb/
- 
+
     assert_warning :type => :template,
       :warning_type => "Cross Site Scripting",
       :line => 16,
       :message => /^Unsafe parameter value in link_to href/,
       :confidence => 1,
-      :file => /test_params\.html\.erb/      
- 
+      :file => /test_params\.html\.erb/
+
     assert_warning :type => :template,
       :warning_type => "Cross Site Scripting",
       :line => 18,
       :message => /^Unsafe parameter value in link_to href/,
       :confidence => 1,
-      :file => /test_params\.html\.erb/            
-  end  
+      :file => /test_params\.html\.erb/
+  end
 
   def test_newlines_in_template
     # Brakeman previously handled multiple newlines between nested ruby
@@ -592,14 +596,14 @@ def test_polymorphic_url_in_href
       :line => 10,
       :message => /^Unsafe parameter value in link_to href/,
       :confidence => 1,
-      :file => /test_model\.html\.erb/  
+      :file => /test_model\.html\.erb/
 
     assert_no_warning :type => :template,
       :warning_type => "Cross Site Scripting",
       :line => 12,
       :message => /^Unsafe parameter value in link_to href/,
       :confidence => 1,
-      :file => /test_model\.html\.erb/  
+      :file => /test_model\.html\.erb/
   end
 
   def test_cross_site_scripting_alias_u_for_link_to_href
@@ -981,7 +985,7 @@ def test_xss_content_tag_unescaped_attribute
       :message => /^Unescaped\ model\ attribute\ in\ content_tag/,
       :confidence => 0,
       :file => /test_content_tag\.html\.erb/
-  end 
+  end
 
   def test_xss_content_tag_in_tag_name
     assert_warning :type => :template,
@@ -1197,7 +1201,7 @@ def test_remote_code_execution_CVE_2013_0333
       :message => /^Rails\ 3\.0\.3\ has\ a\ serious\ JSON\ parsing\ v/,
       :confidence => 0,
       :file => /Gemfile/
-  end 
+  end
 
   def test_denial_of_service_CVE_2013_0269
     assert_warning :type => :warning,

test/tests/rails4_with_engines.rb

@@ -6,7 +6,7 @@ class Rails4WithEnginesTests < Minitest::Test
 
   def expected
     @expected ||= {
-      :controller => 1,
+      :controller => 2,
       :model => 5,
       :template => 12,
       :generic => 13 }
@@ -323,6 +323,18 @@ def test_csrf_without_exception
       :relative_path => "app/controllers/application_controller.rb"
   end
 
+  def test_csrf_in_engine
+    assert_warning :type => :controller,
+      :warning_code => 7,
+      :fingerprint => "bdd5f4f1cdd2e9fb24adc4e9333f2b2eb1d0325badcab7c0b89c25952a2454e8",
+      :warning_type => "Cross-Site Request Forgery",
+      :line => 1,
+      :message => /^'protect_from_forgery'\ should\ be\ called\ /,
+      :confidence => 0,
+      :relative_path => "engines/user_removal/app/controllers/base_controller.rb",
+      :user_input => nil
+  end
+
   def test_xml_dos_CVE_2015_3227
     assert_warning :type => :warning,
       :warning_code => 88,