Merge pull request #448 from presidentbeef/CVE_2014_0081

Add check for CVE-2014-0081

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

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

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]) %>

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

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

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/,

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/,

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

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

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/,

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/,

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