Merge branch 'master' of github.com:presidentbeef/brakeman

lib/brakeman/checks/check_file_access.rb

@@ -9,7 +9,7 @@ class Brakeman::CheckFileAccess < Brakeman::BaseCheck
 
   def run_check
     Brakeman.debug "Finding possible file access"
-    methods = tracker.find_call :targets => [:Dir, :File, :IO, :Kernel, :"Net::FTP", :"Net::HTTP", :PStore, :Pathname, :Shell, :YAML], :methods => [:[], :chdir, :chroot, :delete, :entries, :foreach, :glob, :install, :lchmod, :lchown, :link, :load, :load_file, :makedirs, :move, :new, :open, :read, :read_lines, :rename, :rmdir, :safe_unlink, :symlink, :syscopy, :sysopen, :truncate, :unlink]
+    methods = tracker.find_call :targets => [:Dir, :File, :IO, :Kernel, :"Net::FTP", :"Net::HTTP", :PStore, :Pathname, :Shell, :YAML], :methods => [:[], :chdir, :chroot, :delete, :entries, :foreach, :glob, :install, :lchmod, :lchown, :link, :load, :load_file, :makedirs, :move, :new, :open, :read, :readlines, :rename, :rmdir, :safe_unlink, :symlink, :syscopy, :sysopen, :truncate, :unlink]
 
     Brakeman.debug "Finding calls to load()"
     methods.concat tracker.find_call :target => false, :method => :load
@@ -24,32 +24,48 @@ class Brakeman::CheckFileAccess < Brakeman::BaseCheck
   end
 
   def process_result result
+    return if duplicate? result
+    add_result result
     call = result[:call]
-
     file_name = call[3][1]
 
-    if input = include_user_input?(file_name)
-      unless duplicate? result
-        add_result result
-
-        case input.type
-        when :params
-          message = "Parameter"
-        when :cookies
-          message = "Cookie"
-        else
-          message = "User input"
-        end
-
-        message << " value used in file name"
-
-        warn :result => result,
-          :warning_type => "File Access",
-          :message => message, 
-          :confidence => CONFIDENCE[:high],
-          :code => call,
-          :user_input => input.match
+    if match = has_immediate_user_input?(file_name)
+      confidence = CONFIDENCE[:high]
+    elsif match = has_immediate_model?(file_name)
+      confidence = CONFIDENCE[:med]
+    elsif tracker.options[:check_arguments] and
+      match = include_user_input?(file_name)
+
+      #Check for string building in file name
+      if call?(file_name) and (file_name[2] == :+ or file_name[2] == :<<)
+        confidence = CONFIDENCE[:high]
+      else
+        confidence = CONFIDENCE[:low]
       end
     end
+
+    if match
+      case match.type
+      when :params
+        message = "Parameter"
+      when :cookies
+        message = "Cookie"
+      when :request
+        message = "Request"
+      when :model
+        message = "Model attribute"
+      else
+        message = "User input"
+      end
+
+      message << " value used in file name"
+
+      warn :result => result,
+        :warning_type => "File Access",
+        :message => message,
+        :confidence => confidence,
+        :code => call,
+        :user_input => match.match
+    end
   end
 end

test/apps/rails3.1/app/controllers/users_controller.rb

@@ -102,5 +102,12 @@ class UsersController < ApplicationController
     redirect_to User.find_by_name(params[:name])
   end
 
+  def test_file_access_params
+    File.unlink(blah(params[:file]))
+    Pathname.readlines("blah/#{cookies[:file]}")
+    File.delete(params[:file])
+    IO.read(User.find_by_name('bob').file_path)
+  end
+
   include UserMixin
 end

test/tests/test_rails31.rb

@@ -15,7 +15,7 @@ class Rails31Tests < Test::Unit::TestCase
       :model => 0,
       :template => 4,
       :controller => 1,
-      :warning => 40 }
+      :warning => 44 }
   end
 
   def test_without_protection
@@ -454,4 +454,41 @@ class Rails31Tests < Test::Unit::TestCase
       :confidence => 0,
       :file => /users\/mixin_default\.html\.erb/
   end
+
+
+  def test_file_access_indirect_user_input
+    assert_warning :type => :warning,
+      :warning_type => "File Access",
+      :line => 106,
+      :message => /^Parameter\ value\ used\ in\ file\ name/,
+      :confidence => 2,
+      :file => /users_controller\.rb/
+  end
+
+  def test_file_access_in_string_interpolation
+    assert_warning :type => :warning,
+      :warning_type => "File Access",
+      :line => 107,
+      :message => /^Cookie\ value\ used\ in\ file\ name/,
+      :confidence => 0,
+      :file => /users_controller\.rb/
+  end
+
+  def test_file_access_direct_user_input
+    assert_warning :type => :warning,
+      :warning_type => "File Access",
+      :line => 108,
+      :message => /^Parameter\ value\ used\ in\ file\ name/,
+      :confidence => 0,
+      :file => /users_controller\.rb/
+  end
+
+  def test_file_access_model_attribute
+    assert_warning :type => :warning,
+      :warning_type => "File Access",
+      :line => 109,
+      :message => /^User\ input\ value\ used\ in\ file\ name/,
+      :confidence => 1,
+      :file => /users_controller\.rb/
+  end
 end