diff --git a/lib/brakeman/checks/check_file_access.rb b/lib/brakeman/checks/check_file_access.rb index 7794fb7d86a78b42f74b36698bd84030838897d9..fecc0ec82265e8264ff78a410f515f1a266e3327 100644 --- a/lib/brakeman/checks/check_file_access.rb +++ b/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 diff --git a/test/apps/rails3.1/app/controllers/users_controller.rb b/test/apps/rails3.1/app/controllers/users_controller.rb index c8aeae8d758f07e8a56ce8820ecc8bff6a0d12d6..b87cec3d75a27127ee8c8d33c21fcf021891f55a 100644 --- a/test/apps/rails3.1/app/controllers/users_controller.rb +++ b/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 diff --git a/test/tests/test_rails31.rb b/test/tests/test_rails31.rb index 54077285a3b89d42a8df5a467c11e1dd9e46f01d..5c7fbda7a067d4d54b36639d11f8d076afd70839 100644 --- a/test/tests/test_rails31.rb +++ b/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