diff --git a/lib/brakeman/checks/check_number_to_currency.rb b/lib/brakeman/checks/check_number_to_currency.rb index 355858954ed2b7baf720b672cca341baf5687c91..62fb2353321f0082cfa76c19e1937ab1d550d596 100644 --- a/lib/brakeman/checks/check_number_to_currency.rb +++ b/lib/brakeman/checks/check_number_to_currency.rb @@ -3,53 +3,64 @@ require 'brakeman/checks/base_check' class Brakeman::CheckNumberToCurrency < Brakeman::BaseCheck Brakeman::Checks.add self - @description = "Checks for number_to_currency XSS vulnerability in certain versions" + @description = "Checks for number helpers XSS vulnerabilities in certain versions" def run_check - return if lts_version? '2.3.18.6' - - if (version_between? "2.0.0", "3.2.15" or version_between? "4.0.0", "4.0.1") - check_number_to_currency_usage + if version_between? "2.0.0", "2.3.18" or + version_between? "3.0.0", "3.2.16" or + version_between? "4.0.0", "4.0.2" + check_number_helper_usage generic_warning unless @found_any end end def generic_warning - message = "Rails #{tracker.config[:rails_version]} has a vulnerability in number_to_currency (CVE-2013-6415). Upgrade to Rails version " + message = "Rails #{tracker.config[:rails_version]} has a vulnerability in number helpers (CVE-2014-0081). Upgrade to Rails version " - if version_between? "2.3.0", "3.2.15" - message << "3.2.16" + if version_between? "2.3.0", "3.2.16" + message << "3.2.17" else - message << "4.0.2" + message << "4.0.3" end warn :warning_type => "Cross Site Scripting", - :warning_code => :CVE_2013_6415, + :warning_code => :CVE_2014_0081, :message => message, :confidence => CONFIDENCE[:med], :file => gemfile_or_environment, :link_path => "https://groups.google.com/d/msg/ruby-security-ann/9WiRn2nhfq0/2K2KRB4LwCMJ" end - def check_number_to_currency_usage - tracker.find_call(:target => false, :method => :number_to_currency).each do |result| + def check_number_helper_usage + number_methods = [:number_to_currency, :number_to_percentage, :number_to_human] + tracker.find_call(:target => false, :methods => number_methods).each do |result| arg = result[:call].second_arg next unless arg - if match = (has_immediate_user_input? arg or has_immediate_model? arg) - match = match.match if match.is_a? Match - @found_any = true - warn_on_number_to_currency result, match + if not check_helper_option(result, arg) and hash? arg + hash_iterate(arg) do |key, value| + break if check_helper_option(result, value) + end end end end - def warn_on_number_to_currency result, match + def check_helper_option result, exp + if match = (has_immediate_user_input? exp or has_immediate_model? exp) + match = match.match if match.is_a? Match + warn_on_number_helper result, match + @found_any = true + else + false + end + end + + def warn_on_number_helper result, match warn :result => result, :warning_type => "Cross Site Scripting", - :warning_code => :CVE_2013_6415_call, - :message => "Currency value in number_to_currency is not safe in Rails #{@tracker.config[:rails_version]}", + :warning_code => :CVE_2014_0081_call, + :message => "Format options in #{result[:call].method} are not safe in Rails #{@tracker.config[:rails_version]}", :confidence => CONFIDENCE[:high], :link_path => "https://groups.google.com/d/msg/ruby-security-ann/9WiRn2nhfq0/2K2KRB4LwCMJ", :user_input => match diff --git a/lib/brakeman/warning_codes.rb b/lib/brakeman/warning_codes.rb index 99a0ac16bbdd436f40d8d25bd1c18e01d27194cd..6c00db866c7600d13a0a7a9fd0ac76dbff32deda 100644 --- a/lib/brakeman/warning_codes.rb +++ b/lib/brakeman/warning_codes.rb @@ -73,6 +73,8 @@ module Brakeman::WarningCodes :mass_assign_permit! => 70, :ssl_verification_bypass => 71, :CVE_2014_0080 => 72, + :CVE_2014_0081 => 73, + :CVE_2014_0081_call => 74, } def self.code name diff --git a/test/apps/rails4/app/views/users/index.html.erb b/test/apps/rails4/app/views/users/index.html.erb index 2e592fdf2a31ed91e6d9de1ca32fece33d59260b..3272866be648535c0e5a7b1d9ba43547c9dd7bb0 100644 --- a/test/apps/rails4/app/views/users/index.html.erb +++ b/test/apps/rails4/app/views/users/index.html.erb @@ -8,4 +8,6 @@ <%= number_to_currency(params[:cost], params[:currency]) %> -<%= number_to_currency(params[:cost], h(params[:currency])) %> Should not warn +<%= number_to_human(params[:cost], format: h(params[:format])) %> Should not warn + +<%= number_to_percentage(params[:cost], negative_format: params[:format]) %> diff --git a/test/tests/only_files_option.rb b/test/tests/only_files_option.rb index 93435b1f56b8c9678d7fd11411d336536010a0f9..c00c29a7766679e991864473295a1ccad01a1645 100644 --- a/test/tests/only_files_option.rb +++ b/test/tests/only_files_option.rb @@ -74,13 +74,13 @@ class OnlyFilesOptionTests < Test::Unit::TestCase :user_input => nil end - def test_number_to_currency_CVE_2013_6415 + def test_number_to_currency_CVE_2014_0081 assert_warning :type => :warning, - :warning_code => 65, - :fingerprint => "813b00b5c58567fb3f32051578b839cb25fc2d827834a30d4b213a4c126202a2", + :warning_code => 73, + :fingerprint => "f6981b9c24727ef45040450a1f4b158ae3bc31b4b0343efe853fe12c64881695", :warning_type => "Cross Site Scripting", :line => nil, - :message => /^Rails\ 3\.2\.9\.rc2 has\ a\ vulnerability\ in\ numbe/, + :message => /^Rails\ 3\.2\.9\.rc2\ has\ a\ vulnerability\ in\ n/, :confidence => 1, :relative_path => "Gemfile", :user_input => nil diff --git a/test/tests/rails2.rb b/test/tests/rails2.rb index e7c0a194d6024e10f77852ef715176ffb4fb79aa..e603ee7c3ff0651f2424713ed03fb2ac7e93d1df 100644 --- a/test/tests/rails2.rb +++ b/test/tests/rails2.rb @@ -991,14 +991,16 @@ class Rails2Tests < Test::Unit::TestCase :relative_path => "config/environment.rb" end - def test_number_to_currency_CVE_2013_6415 + def test_number_to_currency_CVE_2014_0081 assert_warning :type => :warning, - :warning_code => 65, - :fingerprint => "1822c8179beeb0358b71c545bad0dd824104aed8b995fe0781c1b6e324417a91", + :warning_code => 73, + :fingerprint => "dd82650c29c3ec7b77437c32d394641744208b42b2aeb673d54e5f42c51e6c33", :warning_type => "Cross Site Scripting", + :line => nil, :message => /^Rails\ 2\.3\.11\ has\ a\ vulnerability\ in\ numb/, :confidence => 1, - :relative_path => "config/environment.rb" + :relative_path => "config/environment.rb", + :user_input => nil end def test_sql_injection_CVE_2013_6417 diff --git a/test/tests/rails3.rb b/test/tests/rails3.rb index fbe0f308c15e02eb1c92d4fa37499ff42e5b619c..106c7fe2a3524b0573f6370d846c240072abf336 100644 --- a/test/tests/rails3.rb +++ b/test/tests/rails3.rb @@ -1149,10 +1149,10 @@ class Rails3Tests < Test::Unit::TestCase :relative_path => "Gemfile" end - def test_number_to_currency_CVE_2013_6415 + def test_number_to_currency_CVE_2014_0081 assert_warning :type => :warning, - :warning_code => 65, - :fingerprint => "813b00b5c58567fb3f32051578b839cb25fc2d827834a30d4b213a4c126202a2", + :warning_code => 73, + :fingerprint => "f6981b9c24727ef45040450a1f4b158ae3bc31b4b0343efe853fe12c64881695", :warning_type => "Cross Site Scripting", :line => nil, :message => /^Rails\ 3\.0\.3\ has\ a\ vulnerability\ in\ numbe/, diff --git a/test/tests/rails31.rb b/test/tests/rails31.rb index 86e3ba41d113e3a0faa25b89f2e9e4b920466eca..9e44ae389a9b251014ec2c5878061ee045cc176c 100644 --- a/test/tests/rails31.rb +++ b/test/tests/rails31.rb @@ -820,10 +820,10 @@ class Rails31Tests < Test::Unit::TestCase :relative_path => "Gemfile" end - def test_number_to_currency_CVE_2013_6415 + def test_number_to_currency_CVE_2014_0081 assert_warning :type => :warning, - :warning_code => 65, - :fingerprint => "813b00b5c58567fb3f32051578b839cb25fc2d827834a30d4b213a4c126202a2", + :warning_code => 73, + :fingerprint => "f6981b9c24727ef45040450a1f4b158ae3bc31b4b0343efe853fe12c64881695", :warning_type => "Cross Site Scripting", :line => nil, :message => /^Rails\ 3\.1\.0\ has\ a\ vulnerability\ in\ numbe/, diff --git a/test/tests/rails32.rb b/test/tests/rails32.rb index 3aeaa7350443f9c244d7cf718308a0b9d725bb9c..2d29ac90f184c523e12a8264994f857cedd71707 100644 --- a/test/tests/rails32.rb +++ b/test/tests/rails32.rb @@ -99,13 +99,13 @@ class Rails32Tests < Test::Unit::TestCase :relative_path => "Gemfile" end - def test_number_to_currency_CVE_2013_6415 + def test_number_to_currency_CVE_2014_0081 assert_warning :type => :warning, - :warning_code => 65, - :fingerprint => "813b00b5c58567fb3f32051578b839cb25fc2d827834a30d4b213a4c126202a2", + :warning_code => 73, + :fingerprint => "f6981b9c24727ef45040450a1f4b158ae3bc31b4b0343efe853fe12c64881695", :warning_type => "Cross Site Scripting", :line => nil, - :message => /^Rails\ 3\.2\.9\.rc2 has\ a\ vulnerability\ in\ numbe/, + :message => /^Rails\ 3\.2\.9\.rc2\ has\ a\ vulnerability\ in\ n/, :confidence => 1, :relative_path => "Gemfile", :user_input => nil diff --git a/test/tests/rails4.rb b/test/tests/rails4.rb index 0a6401d40e6962e421efa3be907350663bf4b1b1..285dd12a8eff0ed1e2ead9a6ba85b8b87a5aec45 100644 --- a/test/tests/rails4.rb +++ b/test/tests/rails4.rb @@ -14,7 +14,7 @@ class Rails4Tests < Test::Unit::TestCase @expected ||= { :controller => 0, :model => 1, - :template => 1, + :template => 2, :generic => 19 } end @@ -224,16 +224,26 @@ class Rails4Tests < Test::Unit::TestCase :relative_path => "Gemfile" end - def test_number_to_currency_CVE_2013_6415 + def test_number_to_currency_CVE_2014_0081 assert_warning :type => :template, - :warning_code => 66, - :fingerprint => "0fb96b5f4b3a4dcdc677d126f492441e2f7b46880563a977b1246b30d3c117a0", + :warning_code => 74, + :fingerprint => "2d06291f03b443619407093e5921ee1e4eb77b1bf045607d776d9493da4a3f95", :warning_type => "Cross Site Scripting", :line => 9, - :message => /^Currency\ value\ in\ number_to_currency\ is\ /, + :message => /^Format\ options\ in\ number_to_currency\ are/, :confidence => 0, :relative_path => "app/views/users/index.html.erb", :user_input => s(:call, s(:call, nil, :params), :[], s(:lit, :currency)) + + assert_warning :type => :template, + :warning_code => 74, + :fingerprint => "c5f481595217e42fbeaf40f32e6407e66d64d246a9729c2c199053e64365ac96", + :warning_type => "Cross Site Scripting", + :line => 12, + :message => /^Format\ options\ in\ number_to_percentage\ a/, + :confidence => 0, + :relative_path => "app/views/users/index.html.erb", + :user_input => s(:call, s(:call, nil, :params), :[], s(:lit, :format)) end def test_simple_format_xss_CVE_2013_6416 diff --git a/test/tests/rails4_with_engines.rb b/test/tests/rails4_with_engines.rb index f40c0f9b004b86e6a2d04aaca97521fc6baff50e..d17fa5fbbc7606d8b103e65928b867c2ec413ef0 100644 --- a/test/tests/rails4_with_engines.rb +++ b/test/tests/rails4_with_engines.rb @@ -28,10 +28,10 @@ class Rails4WithEnginesTests < Test::Unit::TestCase :relative_path => "Gemfile" end - def test_number_to_currency_CVE_2013_6415 + def test_number_to_currency_CVE_2014_0081 assert_warning :type => :warning, - :warning_code => 65, - :fingerprint => "813b00b5c58567fb3f32051578b839cb25fc2d827834a30d4b213a4c126202a2", + :warning_code => 73, + :fingerprint => "f6981b9c24727ef45040450a1f4b158ae3bc31b4b0343efe853fe12c64881695", :warning_type => "Cross Site Scripting", :line => nil, :message => /^Rails\ 4\.0\.0\ has\ a\ vulnerability\ in\ numbe/, diff --git a/test/tests/rails_with_xss_plugin.rb b/test/tests/rails_with_xss_plugin.rb index 86e02cd24bc8813c4e66849cb5afeabf9d15e119..b1f30b9bf6236404fbba9da04d2b1b37e799f350 100644 --- a/test/tests/rails_with_xss_plugin.rb +++ b/test/tests/rails_with_xss_plugin.rb @@ -358,10 +358,10 @@ class RailsWithXssPluginTests < Test::Unit::TestCase :user_input => nil end - def test_number_to_currency_CVE_2013_6415 + def test_number_to_currency_CVE_2014_0081 assert_warning :type => :warning, - :warning_code => 65, - :fingerprint => "813b00b5c58567fb3f32051578b839cb25fc2d827834a30d4b213a4c126202a2", + :warning_code => 73, + :fingerprint => "f6981b9c24727ef45040450a1f4b158ae3bc31b4b0343efe853fe12c64881695", :warning_type => "Cross Site Scripting", :line => nil, :message => /^Rails\ 2\.3\.14\ has\ a\ vulnerability\ in\ numb/, diff --git a/test/tests/rescanner.rb b/test/tests/rescanner.rb index 5d96819025c48b9187556096818b5d39de28ed4d..0a08348c0dcbf60957675a8dfedbd7eaf267b292 100644 --- a/test/tests/rescanner.rb +++ b/test/tests/rescanner.rb @@ -265,6 +265,6 @@ class RescannerTests < Test::Unit::TestCase assert_reindex :none assert_changes assert_new 0 - assert_fixed 3 + assert_fixed 2 end end