diff --git a/CHANGES b/CHANGES index 6d70ea159d6ed9c5f7c6eaad8ae57b0039d9b001..3e10227b2033f02a5a076476f8f3c9461620db8d 100644 --- a/CHANGES +++ b/CHANGES @@ -5,6 +5,12 @@ * Skip identically rendered templates * Fix HAML template processing +# 2.4.1 + + * Add check for CVE-2014-0082 + * Add check for CVE-2014-0081, replaces CVE-2013-6415 + * Add check for CVE-2014-0080 + # 2.4.0 * Detect Rails LTS versions 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/checks/check_render_dos.rb b/lib/brakeman/checks/check_render_dos.rb new file mode 100644 index 0000000000000000000000000000000000000000..57d1e846a84917da55b5a866be9f6283481387eb --- /dev/null +++ b/lib/brakeman/checks/check_render_dos.rb @@ -0,0 +1,37 @@ +require 'brakeman/checks/base_check' + +class Brakeman::CheckRenderDoS < Brakeman::BaseCheck + Brakeman::Checks.add self + + @description = "Warn about denial of service with render :text (CVE-2014-0082)" + + def run_check + if version_between? "3.0.0", "3.0.20" or + version_between? "3.1.0", "3.1.12" or + version_between? "3.2.0", "3.2.16" + + tracker.find_call(:target => nil, :method => :render).each do |result| + if text_render? result + warn_about_text_render + break + end + end + end + end + + def text_render? result + node_type? result[:call], :render and + result[:call].render_type == :text + end + + def warn_about_text_render + message = "Rails #{tracker.config[:rails_version]} has a denial of service vulnerability (CVE-2014-0082). Upgrade to Rails version 3.2.17" + + warn :warning_type => "Denial of Service", + :warning_code => :CVE_2014_0082, + :message => message, + :confidence => CONFIDENCE[:high], + :link_path => "https://groups.google.com/d/msg/rubyonrails-security/LMxO_3_eCuc/ozGBEhKaJbIJ", + :file => gemfile_or_environment + end +end diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index c8cd11e256823f981d7b0d76ba2d15dfd1d4cf09..29a7da4a967fd35be67738554db69960c091401d 100644 --- a/lib/brakeman/checks/check_sql.rb +++ b/lib/brakeman/checks/check_sql.rb @@ -51,6 +51,8 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck Brakeman.debug "Processing possible SQL calls" calls.each { |call| process_result call } + + check_CVE_2014_0080 end #Find calls to named_scope() or scope() in models @@ -638,6 +640,19 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck end end + # TODO: Move all SQL CVE checks to separate class + def check_CVE_2014_0080 + return unless version_between? "4.0.0", "4.0.2" and + @tracker.config[:gems].include? :pg + + warn :warning_type => 'SQL Injection', + :warning_code => :CVE_2014_0080, + :message => "Rails #{tracker.config[:rails_version]} contains a SQL injection vulnerability (CVE-2014-0080) with PostgreSQL. Upgrade to 4.0.3", + :confidence => CONFIDENCE[:high], + :file => gemfile_or_environment, + :link_path => "https://groups.google.com/d/msg/rubyonrails-security/Wu96YkTUR6s/pPLBMZrlwvYJ" + end + def upgrade_version? versions versions.each do |low, high, upgrade| return upgrade if version_between? low, high diff --git a/lib/brakeman/version.rb b/lib/brakeman/version.rb index fdd73953375495d5344a569d46791850bece836b..9729589996dafd8a2930965a637bff0fe80ff216 100644 --- a/lib/brakeman/version.rb +++ b/lib/brakeman/version.rb @@ -1,3 +1,3 @@ module Brakeman - Version = "2.4.0" + Version = "2.4.1" end diff --git a/lib/brakeman/warning_codes.rb b/lib/brakeman/warning_codes.rb index 454e3017945b5d1ac5558fd5d92b65c97cf4c9ab..6fb1b1a0fb83ece6583897121aca44d30bfab585 100644 --- a/lib/brakeman/warning_codes.rb +++ b/lib/brakeman/warning_codes.rb @@ -71,7 +71,11 @@ module Brakeman::WarningCodes :CVE_2013_6416_call => 68, :CVE_2013_6417 => 69, :mass_assign_permit! => 70, - :ssl_verification_bypass => 71 + :ssl_verification_bypass => 71, + :CVE_2014_0080 => 72, + :CVE_2014_0081 => 73, + :CVE_2014_0081_call => 74, + :CVE_2014_0082 => 75, } def self.code name diff --git a/test/apps/rails3.2/app/controllers/users_controller.rb b/test/apps/rails3.2/app/controllers/users_controller.rb index 7914c966c2b2e9ebb334f2309f34e47bcf23111c..4f782bec7875e6b9c0d3c5000e8a21fad7edfa3e 100644 --- a/test/apps/rails3.2/app/controllers/users_controller.rb +++ b/test/apps/rails3.2/app/controllers/users_controller.rb @@ -92,4 +92,8 @@ class UsersController < ApplicationController def show_detailed_exceptions? false # no warning end + + def render_text + render :text => "oh noes my service" + end end diff --git a/test/apps/rails3/app/controllers/products_controller.rb b/test/apps/rails3/app/controllers/products_controller.rb index 8c771612471fbcd46cb83d32a225e79113899984..9804f6a6875273e69e8c9def3eebb74baab2f053 100644 --- a/test/apps/rails3/app/controllers/products_controller.rb +++ b/test/apps/rails3/app/controllers/products_controller.rb @@ -80,4 +80,8 @@ class ProductsController < ApplicationController format.xml { head :ok } end end + + def render_some_text + render :text => "jello" + end end diff --git a/test/apps/rails4/Gemfile b/test/apps/rails4/Gemfile index d22b9e15ad3b96e503876b17bf2258f4d83fe6e4..a12d260db06e89120b4f6d1f830e64f5ea1e8bb4 100644 --- a/test/apps/rails4/Gemfile +++ b/test/apps/rails4/Gemfile @@ -3,7 +3,7 @@ source 'https://rubygems.org' # Bundle edge Rails instead: gem 'rails', github: 'rails/rails' gem 'rails', '4.0.0' -gem 'sqlite3' +gem 'pg' # Gems used only for assets and not required # in production environments by default. 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 f42c9818b0b6cec79a962c012c3279251aef6518..c4b99874f83c211b841718207245291b18b9a2ab 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..82fdc466d55533c60b7fcbb844e495a9a91364af 100644 --- a/test/tests/rails3.rb +++ b/test/tests/rails3.rb @@ -16,7 +16,7 @@ class Rails3Tests < Test::Unit::TestCase :controller => 1, :model => 8, :template => 38, - :generic => 70 + :generic => 71 } if RUBY_PLATFORM == 'java' @@ -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/, @@ -1173,6 +1173,18 @@ class Rails3Tests < Test::Unit::TestCase :user_input => nil end + def test_denial_of_service_CVE_2014_0082 + assert_warning :type => :warning, + :warning_code => 75, + :fingerprint => "403a72d08a90043384fe56d3a6bc3e255b8799b380693914143d403607433db7", + :warning_type => "Denial of Service", + :line => nil, + :message => /^Rails\ 3\.0\.3\ has\ a\ denial\ of\ service\ vuln/, + :confidence => 0, + :relative_path => "Gemfile", + :user_input => nil + end + def test_http_only_session_setting assert_warning :type => :warning, :warning_type => "Session Setting", 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..4bf07a897528c00666d118c9a971e73a4f3e66b8 100644 --- a/test/tests/rails32.rb +++ b/test/tests/rails32.rb @@ -11,7 +11,7 @@ class Rails32Tests < Test::Unit::TestCase :controller => 0, :model => 5, :template => 11, - :generic => 10 } + :generic => 11 } if RUBY_PLATFORM == 'java' @expected[:generic] += 1 @@ -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 @@ -123,6 +123,18 @@ class Rails32Tests < Test::Unit::TestCase :user_input => nil end + def test_denial_of_service_CVE_2014_0082 + assert_warning :type => :warning, + :warning_code => 75, + :fingerprint => "403a72d08a90043384fe56d3a6bc3e255b8799b380693914143d403607433db7", + :warning_type => "Denial of Service", + :line => nil, + :message => /^Rails\ 3\.2\.9\.rc2\ has\ a\ denial\ of\ service\ /, + :confidence => 0, + :relative_path => "Gemfile", + :user_input => nil + end + def test_redirect_1 assert_warning :type => :warning, :warning_type => "Redirect", diff --git a/test/tests/rails4.rb b/test/tests/rails4.rb index b88d9a9790571771536f8d6eb04e685b69a1afba..285dd12a8eff0ed1e2ead9a6ba85b8b87a5aec45 100644 --- a/test/tests/rails4.rb +++ b/test/tests/rails4.rb @@ -14,8 +14,8 @@ class Rails4Tests < Test::Unit::TestCase @expected ||= { :controller => 0, :model => 1, - :template => 1, - :generic => 18 + :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 @@ -260,6 +270,18 @@ class Rails4Tests < Test::Unit::TestCase :user_input => nil end + def test_sql_injection_CVE_2014_0080 + assert_warning :type => :warning, + :warning_code => 72, + :fingerprint => "0ba20216bdda1cc067f9e4795bdb0d9224fd23c58317ecc09db67b6b38a2d0f0", + :warning_type => "SQL Injection", + :line => nil, + :message => /^Rails\ 4\.0\.0\ contains\ a\ SQL\ injection\ vul/, + :confidence => 0, + :relative_path => "Gemfile", + :user_input => nil + end + def test_mass_assignment_with_permit! assert_warning :type => :warning, :warning_code => 70, 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 601ddfad5a33e0c15aa065e57948ae92ce641e8a..70c6f28454bd9cc0f73e48a7f1d1f907fba1f1ee 100644 --- a/test/tests/rails_with_xss_plugin.rb +++ b/test/tests/rails_with_xss_plugin.rb @@ -381,10 +381,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..30e01f6faac8d529d4d12fb44e63c2877c8a4201 100644 --- a/test/tests/rescanner.rb +++ b/test/tests/rescanner.rb @@ -265,6 +265,25 @@ class RescannerTests < Test::Unit::TestCase assert_reindex :none assert_changes assert_new 0 - assert_fixed 3 + assert_fixed 2 + end + + def test_gemfile_rails_version_fix_CVE_2014_0082 + gemfile = "Gemfile.lock" + + before_rescan_of gemfile do + replace gemfile, "rails (3.2.9.rc2)", "rails (3.2.17)" + end + + #@original is actually modified + assert @original.config[:rails_version], "3.2.17" + assert_reindex :none + assert_changes + assert_new 0 + if RUBY_PLATFORM == "java" + assert_fixed 10 + else + assert_fixed 9 + end end end