From eaa2101b294ef546cc3fb35cc3f49c73849ac470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Feb 2014 23:29:27 -0200 Subject: [PATCH 1/3] Escape format, negative_format and units options of number helpers Previously the values of these options were trusted leading to potential XSS vulnerabilities. Fixes: CVE-2014-0081 --- .../lib/action_view/helpers/number_helper.rb | 14 ++++- .../test/template/number_helper_test.rb | 51 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/helpers/number_helper.rb b/actionpack/lib/action_view/helpers/number_helper.rb index eee9e59a24..91f60434b1 100644 --- a/actionpack/lib/action_view/helpers/number_helper.rb +++ b/actionpack/lib/action_view/helpers/number_helper.rb @@ -138,12 +138,18 @@ module ActionView options.symbolize_keys! + options[:delimiter] = ERB::Util.html_escape(options[:delimiter]) if options[:delimiter] + options[:separator] = ERB::Util.html_escape(options[:separator]) if options[:separator] + options[:format] = ERB::Util.html_escape(options[:format]) if options[:format] + options[:negative_format] = ERB::Util.html_escape(options[:negative_format]) if options[:negative_format] + defaults = I18n.translate(:'number.format', :locale => options[:locale], :default => {}) currency = I18n.translate(:'number.currency.format', :locale => options[:locale], :default => {}) currency[:negative_format] ||= "-" + currency[:format] if currency[:format] defaults = DEFAULT_CURRENCY_VALUES.merge(defaults).merge!(currency) defaults[:negative_format] = "-" + options[:format] if options[:format] + options = defaults.merge!(options) unit = options.delete(:unit) @@ -206,6 +212,9 @@ module ActionView options.symbolize_keys! + options[:delimiter] = ERB::Util.html_escape(options[:delimiter]) if options[:delimiter] + options[:separator] = ERB::Util.html_escape(options[:separator]) if options[:separator] + defaults = I18n.translate(:'number.format', :locale => options[:locale], :default => {}) percentage = I18n.translate(:'number.percentage.format', :locale => options[:locale], :default => {}) defaults = defaults.merge(percentage) @@ -255,6 +264,9 @@ module ActionView def number_with_delimiter(number, options = {}) options.symbolize_keys! + options[:delimiter] = ERB::Util.html_escape(options[:delimiter]) if options[:delimiter] + options[:separator] = ERB::Util.html_escape(options[:separator]) if options[:separator] + begin Float(number) rescue ArgumentError, TypeError @@ -578,7 +590,7 @@ module ActionView units = options.delete :units unit_exponents = case units when Hash - units + units = Hash[units.map { |k, v| [k, ERB::Util.html_escape(v)] }] when String, Symbol I18n.translate(:"#{units}", :locale => options[:locale], :raise => true) when nil diff --git a/actionpack/test/template/number_helper_test.rb b/actionpack/test/template/number_helper_test.rb index 22da7e26e1..7f78b52b34 100644 --- a/actionpack/test/template/number_helper_test.rb +++ b/actionpack/test/template/number_helper_test.rb @@ -19,6 +19,27 @@ class NumberHelperTest < ActionView::TestCase gigabytes(number) * 1024 end + def test_number_helpers_escape_delimiter_and_separator + assert_equal "111<script></script>111<script></script>1111", number_to_phone(1111111111, :delimiter => "") + + assert_equal "$1<script></script>01", number_to_currency(1.01, :separator => "") + assert_equal "$1<script></script>000.00", number_to_currency(1000, :delimiter => "") + + assert_equal "1<script></script>010%", number_to_percentage(1.01, :separator => "") + assert_equal "1<script></script>000.000%", number_to_percentage(1000, :delimiter => "") + + assert_equal "1<script></script>01", number_with_delimiter(1.01, :separator => "") + assert_equal "1<script></script>000", number_with_delimiter(1000, :delimiter => "") + + assert_equal "1<script></script>010", number_with_precision(1.01, :separator => "") + assert_equal "1<script></script>000.000", number_with_precision(1000, :delimiter => "") + + assert_equal "9<script></script>86 KB", number_to_human_size(10100, :separator => "") + + assert_equal "1<script></script>01", number_to_human(1.01, :separator => "") + assert_equal "100<script></script>000 Quadrillion", number_to_human(10**20, :delimiter => "") + end + def test_number_to_phone assert_equal("555-1234", number_to_phone(5551234)) assert_equal("800-555-1212", number_to_phone(8005551212)) @@ -33,6 +54,8 @@ class NumberHelperTest < ActionView::TestCase assert_equal("+18005551212", number_to_phone(8005551212, :country_code => 1, :delimiter => '')) assert_equal("22-555-1212", number_to_phone(225551212)) assert_equal("+45-22-555-1212", number_to_phone(225551212, :country_code => 45)) + assert_equal "+<script></script>8005551212", number_to_phone(8005551212, :country_code => "", :delimiter => "") + assert_equal "8005551212 x <script></script>", number_to_phone(8005551212, :extension => "", :delimiter => "") end def test_number_to_currency @@ -48,6 +71,9 @@ class NumberHelperTest < ActionView::TestCase assert_equal("$1,234,567,890.50", number_to_currency("1234567890.50")) assert_equal("1,234,567,890.50 Kč", number_to_currency("1234567890.50", {:unit => raw("Kč"), :format => "%n %u"})) assert_equal("1,234,567,890.50 - Kč", number_to_currency("-1234567890.50", {:unit => raw("Kč"), :format => "%n %u", :negative_format => "%n - %u"})) + assert_equal "<b>1,234,567,890.50</b> $", number_to_currency("1234567890.50", :format => "%n %u") + assert_equal "<b>1,234,567,890.50</b> $", number_to_currency("-1234567890.50", :negative_format => "%n %u") + assert_equal "<b>1,234,567,890.50</b> $", number_to_currency("-1234567890.50", 'negative_format' => "%n %u") end def test_number_to_percentage @@ -252,6 +278,31 @@ class NumberHelperTest < ActionView::TestCase assert_equal '4.5 tens', number_to_human(45, :units => {:unit => "", :ten => ' tens '}) end + def test_number_to_human_escape_units + volume = { :unit => "ml", :thousand => "lt", :million => "m3", :trillion => "km3", :quadrillion => "Pl" } + assert_equal '123 <b>lt</b>', number_to_human(123456, :units => volume) + assert_equal '12 <b>ml</b>', number_to_human(12, :units => volume) + assert_equal '1.23 <b>m3</b>', number_to_human(1234567, :units => volume) + assert_equal '1.23 <b>km3</b>', number_to_human(1_234_567_000_000, :units => volume) + assert_equal '1.23 <b>Pl</b>', number_to_human(1_234_567_000_000_000, :units => volume) + + #Including fractionals + distance = { :mili => "mm", :centi => "cm", :deci => "dm", :unit => "m", + :ten => "dam", :hundred => "hm", :thousand => "km", + :micro => "um", :nano => "nm", :pico => "pm", :femto => "fm"} + assert_equal '1.23 <b>mm</b>', number_to_human(0.00123, :units => distance) + assert_equal '1.23 <b>cm</b>', number_to_human(0.0123, :units => distance) + assert_equal '1.23 <b>dm</b>', number_to_human(0.123, :units => distance) + assert_equal '1.23 <b>m</b>', number_to_human(1.23, :units => distance) + assert_equal '1.23 <b>dam</b>', number_to_human(12.3, :units => distance) + assert_equal '1.23 <b>hm</b>', number_to_human(123, :units => distance) + assert_equal '1.23 <b>km</b>', number_to_human(1230, :units => distance) + assert_equal '1.23 <b>um</b>', number_to_human(0.00000123, :units => distance) + assert_equal '1.23 <b>nm</b>', number_to_human(0.00000000123, :units => distance) + assert_equal '1.23 <b>pm</b>', number_to_human(0.00000000000123, :units => distance) + assert_equal '1.23 <b>fm</b>', number_to_human(0.00000000000000123, :units => distance) + end + def test_number_to_human_with_custom_units_that_are_missing_the_needed_key assert_equal '123', number_to_human(123, :units => {:thousand => 'k'}) assert_equal '123', number_to_human(123, :units => {}) From 388d2f88886e4da8cc9fd9e14c80a4021ef47da1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Feb 2014 22:56:50 -0200 Subject: [PATCH 2/3] Use the reference for the mime type to get the format Before we were calling to_sym in the mime type, even when it is unknown what can cause denial of service since symbols are not removed by the garbage collector. Fixes: CVE-2014-0082 --- actionpack/lib/action_view/template/text.rb | 2 +- actionpack/test/template/text_test.rb | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 actionpack/test/template/text_test.rb diff --git a/actionpack/lib/action_view/template/text.rb b/actionpack/lib/action_view/template/text.rb index 4261c3b5e2..d90e43b8f1 100644 --- a/actionpack/lib/action_view/template/text.rb +++ b/actionpack/lib/action_view/template/text.rb @@ -23,7 +23,7 @@ module ActionView #:nodoc: end def formats - [@mime_type.to_sym] + [@mime_type.respond_to?(:ref) ? @mime_type.ref : @mime_type.to_s] end end end diff --git a/actionpack/test/template/text_test.rb b/actionpack/test/template/text_test.rb new file mode 100644 index 0000000000..d899d54589 --- /dev/null +++ b/actionpack/test/template/text_test.rb @@ -0,0 +1,17 @@ +require 'abstract_unit' + +class TextTest < ActiveSupport::TestCase + test 'formats returns symbol for recognized MIME type' do + assert_equal [:text], ActionView::Template::Text.new('', :text).formats + end + + test 'formats returns string for recognized MIME type when MIME does not have symbol' do + foo = Mime::Type.lookup("foo") + assert_nil foo.to_sym + assert_equal ['foo'], ActionView::Template::Text.new('', foo).formats + end + + test 'formats returns string for unknown MIME type' do + assert_equal ['foo'], ActionView::Template::Text.new('', 'foo').formats + end +end From 666e9f65bdfeb6cc5aa80b6254608adc3d7845ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 18 Feb 2014 15:08:16 -0300 Subject: [PATCH 3/3] Preparing for 3.2.17 release --- RAILS_VERSION | 2 +- actionmailer/lib/action_mailer/version.rb | 2 +- actionpack/CHANGELOG.md | 10 ++++++++++ actionpack/lib/action_pack/version.rb | 2 +- activemodel/lib/active_model/version.rb | 2 +- activerecord/lib/active_record/version.rb | 2 +- activeresource/lib/active_resource/version.rb | 2 +- activesupport/lib/active_support/version.rb | 2 +- railties/lib/rails/version.rb | 2 +- version.rb | 2 +- 10 files changed, 19 insertions(+), 9 deletions(-) diff --git a/RAILS_VERSION b/RAILS_VERSION index c0f5d08c10..ff8001a0f1 100644 --- a/RAILS_VERSION +++ b/RAILS_VERSION @@ -1 +1 @@ -3.2.16 +3.2.17 diff --git a/actionmailer/lib/action_mailer/version.rb b/actionmailer/lib/action_mailer/version.rb index ab2d6c39c3..e33d01ac4c 100644 --- a/actionmailer/lib/action_mailer/version.rb +++ b/actionmailer/lib/action_mailer/version.rb @@ -2,7 +2,7 @@ module ActionMailer module VERSION #:nodoc: MAJOR = 3 MINOR = 2 - TINY = 16 + TINY = 17 PRE = nil STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index ff72af724b..6269123de3 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,13 @@ +* Use the reference for the mime type to get the format + + Fixes: CVE-2014-0082 + +* Escape format, negative_format and units options of number helpers + + Fixes: CVE-2014-0081 + +## Rails 3.2.16 (Dec 12, 2013) ## + * Deep Munge the parameters for GET and POST Fixes CVE-2013-6417 * Stop using i18n's built in HTML error handling. Fixes: CVE-2013-4491 diff --git a/actionpack/lib/action_pack/version.rb b/actionpack/lib/action_pack/version.rb index 33d221e091..4d278814c8 100644 --- a/actionpack/lib/action_pack/version.rb +++ b/actionpack/lib/action_pack/version.rb @@ -2,7 +2,7 @@ module ActionPack module VERSION #:nodoc: MAJOR = 3 MINOR = 2 - TINY = 16 + TINY = 17 PRE = nil STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') diff --git a/activemodel/lib/active_model/version.rb b/activemodel/lib/active_model/version.rb index d53cb89fa5..08d437cbc2 100644 --- a/activemodel/lib/active_model/version.rb +++ b/activemodel/lib/active_model/version.rb @@ -2,7 +2,7 @@ module ActiveModel module VERSION #:nodoc: MAJOR = 3 MINOR = 2 - TINY = 16 + TINY = 17 PRE = nil STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') diff --git a/activerecord/lib/active_record/version.rb b/activerecord/lib/active_record/version.rb index 1210df34f6..cced9eae8f 100644 --- a/activerecord/lib/active_record/version.rb +++ b/activerecord/lib/active_record/version.rb @@ -2,7 +2,7 @@ module ActiveRecord module VERSION #:nodoc: MAJOR = 3 MINOR = 2 - TINY = 16 + TINY = 17 PRE = nil STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') diff --git a/activeresource/lib/active_resource/version.rb b/activeresource/lib/active_resource/version.rb index 4efb6f07f5..ea9b7a51e9 100644 --- a/activeresource/lib/active_resource/version.rb +++ b/activeresource/lib/active_resource/version.rb @@ -2,7 +2,7 @@ module ActiveResource module VERSION #:nodoc: MAJOR = 3 MINOR = 2 - TINY = 16 + TINY = 17 PRE = nil STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') diff --git a/activesupport/lib/active_support/version.rb b/activesupport/lib/active_support/version.rb index 6ef2d9d357..95faab1dd6 100644 --- a/activesupport/lib/active_support/version.rb +++ b/activesupport/lib/active_support/version.rb @@ -2,7 +2,7 @@ module ActiveSupport module VERSION #:nodoc: MAJOR = 3 MINOR = 2 - TINY = 16 + TINY = 17 PRE = nil STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') diff --git a/railties/lib/rails/version.rb b/railties/lib/rails/version.rb index 2cfdeca2f6..38890e162d 100644 --- a/railties/lib/rails/version.rb +++ b/railties/lib/rails/version.rb @@ -2,7 +2,7 @@ module Rails module VERSION #:nodoc: MAJOR = 3 MINOR = 2 - TINY = 16 + TINY = 17 PRE = nil STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') diff --git a/version.rb b/version.rb index 2cfdeca2f6..38890e162d 100644 --- a/version.rb +++ b/version.rb @@ -2,7 +2,7 @@ module Rails module VERSION #:nodoc: MAJOR = 3 MINOR = 2 - TINY = 16 + TINY = 17 PRE = nil STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.')