diff --git a/lib/brakeman/checks/check_mail_to.rb b/lib/brakeman/checks/check_mail_to.rb index 3b1d671aa6635294eb664ed61160a5a626090b97..9c97e3d7e81b35636144dfe2f776bce92b406c97 100644 --- a/lib/brakeman/checks/check_mail_to.rb +++ b/lib/brakeman/checks/check_mail_to.rb @@ -23,7 +23,8 @@ class Brakeman::CheckMailTo < Brakeman::BaseCheck :warning_type => "Mail Link", :message => message, :confidence => CONFIDENCE[:high], - :file => gemfile_or_environment + :file => gemfile_or_environment, + :link_path => "https://groups.google.com/d/topic/rubyonrails-security/8CpI7egxX4E/discussion" end end @@ -33,13 +34,10 @@ class Brakeman::CheckMailTo < Brakeman::BaseCheck Brakeman.debug "Checking calls to mail_to for javascript encoding" tracker.find_call(:target => false, :method => :mail_to).each do |result| - call = result[:call] - args = call.args - - args.each do |arg| + result[:call].arglist.each do |arg| if hash? arg - if hash_access(arg, :javascript) - return result + if option = hash_access(arg, :encode) + return result if symbol? option and option.value == :javascript end end end diff --git a/test/apps/rails3/Gemfile b/test/apps/rails3/Gemfile index 3a47f2f8a19ef63a9982b35ad1d7ba103cd883c1..53db870ec5a8640a8a5e952569bbe38776924a73 100644 --- a/test/apps/rails3/Gemfile +++ b/test/apps/rails3/Gemfile @@ -1,6 +1,6 @@ source 'http://rubygems.org' -gem 'rails', '3.0.5' +gem 'rails', '3.0.3' # Bundle edge Rails instead: # gem 'rails', :git => 'git://github.com/rails/rails.git' diff --git a/test/apps/rails3/Gemfile.lock b/test/apps/rails3/Gemfile.lock index 6e927eb2e5765f98985095bab38d53a85f5e0c3b..c0042365f21605610a948025e5f51b4030b78867 100644 --- a/test/apps/rails3/Gemfile.lock +++ b/test/apps/rails3/Gemfile.lock @@ -2,12 +2,12 @@ GEM remote: http://rubygems.org/ specs: abstract (1.0.0) - actionmailer (3.0.5) - actionpack (= 3.0.5) + actionmailer (3.0.3) + actionpack (= 3.0.3) mail (~> 2.2.15) - actionpack (3.0.5) - activemodel (= 3.0.5) - activesupport (= 3.0.5) + actionpack (3.0.3) + activemodel (= 3.0.3) + activesupport (= 3.0.3) builder (~> 2.1.2) erubis (~> 2.6.6) i18n (~> 0.4) @@ -15,19 +15,19 @@ GEM rack-mount (~> 0.6.13) rack-test (~> 0.5.7) tzinfo (~> 0.3.23) - activemodel (3.0.5) - activesupport (= 3.0.5) + activemodel (3.0.3) + activesupport (= 3.0.3) builder (~> 2.1.2) i18n (~> 0.4) - activerecord (3.0.5) - activemodel (= 3.0.5) - activesupport (= 3.0.5) + activerecord (3.0.3) + activemodel (= 3.0.3) + activesupport (= 3.0.3) arel (~> 2.0.2) tzinfo (~> 0.3.23) - activeresource (3.0.5) - activemodel (= 3.0.5) - activesupport (= 3.0.5) - activesupport (3.0.5) + activeresource (3.0.3) + activemodel (= 3.0.3) + activesupport (= 3.0.3) + activesupport (3.0.3) arel (2.0.10) builder (2.1.2) erubis (2.6.6) @@ -45,17 +45,17 @@ GEM rack (>= 1.0.0) rack-test (0.5.7) rack (>= 1.0) - rails (3.0.5) - actionmailer (= 3.0.5) - actionpack (= 3.0.5) - activerecord (= 3.0.5) - activeresource (= 3.0.5) - activesupport (= 3.0.5) + rails (3.0.3) + actionmailer (= 3.0.3) + actionpack (= 3.0.3) + activerecord (= 3.0.3) + activeresource (= 3.0.3) + activesupport (= 3.0.3) bundler (~> 1.0) - railties (= 3.0.5) - railties (3.0.5) - actionpack (= 3.0.5) - activesupport (= 3.0.5) + railties (= 3.0.3) + railties (3.0.3) + actionpack (= 3.0.3) + activesupport (= 3.0.3) rake (>= 0.8.7) thor (~> 0.14.4) rake (0.9.2) @@ -70,5 +70,5 @@ PLATFORMS ruby DEPENDENCIES - rails (= 3.0.5) + rails (= 3.0.3) sqlite3 diff --git a/test/apps/rails3/app/controllers/other_controller.rb b/test/apps/rails3/app/controllers/other_controller.rb index a11b7f79774d886b055ac1fecf8a151e537bed2f..57f3cec463ab9fdc2f1dbc6b8358faffb19d1b28 100644 --- a/test/apps/rails3/app/controllers/other_controller.rb +++ b/test/apps/rails3/app/controllers/other_controller.rb @@ -39,4 +39,8 @@ class OtherController < ApplicationController def test_render_with_nonsymbol_key render x => :y end + + def test_mail_to + @user = User.find(current_user) + end end diff --git a/test/apps/rails3/app/views/other/test_mail_to.html.erb b/test/apps/rails3/app/views/other/test_mail_to.html.erb new file mode 100644 index 0000000000000000000000000000000000000000..3fcca627816eb2a2cf5be10828feaea85e4a9d31 --- /dev/null +++ b/test/apps/rails3/app/views/other/test_mail_to.html.erb @@ -0,0 +1,4 @@ +<%= mail_to @user.email, @user.name, :encode => :javascript %> + +Should not warn: +<%= mail_to @user.email, @user.name, :encode => :hex %> diff --git a/test/apps/rails3/config/routes.rb b/test/apps/rails3/config/routes.rb index 472547b473bd214028ebc45ee835f8248f6de5ac..8f72e9172b67a3cf3d61cfd299b17747d8ac097e 100644 --- a/test/apps/rails3/config/routes.rb +++ b/test/apps/rails3/config/routes.rb @@ -11,6 +11,8 @@ Rails3::Application.routes.draw do get "other/test_send_file" + get "other/test_mail_to" + get "home/index" get "home/test_params" diff --git a/test/tests/test_rails3.rb b/test/tests/test_rails3.rb index 6cacdcb0802efd228aa14fa98c0271a8aecc4caf..d5b761f4e7f6dff072448445269197f9e90b7e02 100644 --- a/test/tests/test_rails3.rb +++ b/test/tests/test_rails3.rb @@ -14,7 +14,7 @@ class Rails3Tests < Test::Unit::TestCase @expected ||= { :controller => 1, :model => 5, - :template => 29, + :template => 30, :warning => 30 } end @@ -532,7 +532,7 @@ class Rails3Tests < Test::Unit::TestCase def test_default_routes assert_warning :warning_type => "Default Routes", - :line => 95, + :line => 97, :message => /All public methods in controllers are available as actions/, :file => /routes\.rb/ end @@ -572,7 +572,7 @@ class Rails3Tests < Test::Unit::TestCase def test_string_buffer_manipulation_bug assert_warning :type => :warning, :warning_type => "Cross Site Scripting", - :message => /^Rails 3.0.5 has a vulnerabilty in SafeBuffer. Upgrade to 3.0.12/, + :message => /^Rails 3\.\d\.\d has a vulnerabilty in SafeBuffer. Upgrade to 3.0.12/, :confidence => 1, :file => /Gemfile/ end @@ -653,7 +653,7 @@ class Rails3Tests < Test::Unit::TestCase assert_warning :type => :template, :warning_type => "Cross Site Scripting", :line => 3, - :message => /^Upgrade\ to\ Rails\ 3\.0\.17,\ 3\.0\.5\ select_ta/, + :message => /^Upgrade\ to\ Rails\ 3\.0\.17,\ 3\.0\.3\ select_ta/, :confidence => 0, :file => /test_select_tag\.html\.erb/ end @@ -661,7 +661,7 @@ class Rails3Tests < Test::Unit::TestCase def test_cross_site_scripting_single_quotes_CVE_2012_3464 assert_warning :type => :warning, :warning_type => "Cross Site Scripting", - :message => /^Rails\ 3\.0\.5\ does\ not\ escape\ single\ quote/, + :message => /^Rails\ 3\.0\.3\ does\ not\ escape\ single\ quote/, :confidence => 1, :file => /Gemfile/ end @@ -681,4 +681,13 @@ class Rails3Tests < Test::Unit::TestCase :confidence => 0, :file => /Gemfile/ end + + def test_mail_link_CVE_2011_0446 + assert_warning :type => :template, + :warning_type => "Mail Link", + :line => 1, + :message => /^Vulnerability\ in\ mail_to\ using\ javascrip/, + :confidence => 0, + :file => /Gemfile/ + end end