From 0aff15a51a1d44d0750d4eb9984175ff0261d46c Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Tue, 3 Dec 2013 12:50:32 -0800 Subject: [PATCH 1/2] Fix for CVE-2013-6415 https://groups.google.com/d/msg/ruby-security-ann/9WiRn2nhfq0/2K2KRB4LwCMJ --- actionpack/lib/action_view/helpers/number_helper.rb | 7 +++++-- actionpack/test/template/number_helper_test.rb | 5 +++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_view/helpers/number_helper.rb b/actionpack/lib/action_view/helpers/number_helper.rb index 4c0f5776014c0..312892c80f286 100644 --- a/actionpack/lib/action_view/helpers/number_helper.rb +++ b/actionpack/lib/action_view/helpers/number_helper.rb @@ -85,11 +85,14 @@ def number_to_currency(number, options = {}) separator = '' if precision == 0 begin - format.gsub(/%n/, number_with_precision(number, + value = number_with_precision(number, :precision => precision, :delimiter => delimiter, :separator => separator) - ).gsub(/%u/, unit).html_safe + + return nil if value.nil? + + format.gsub(/%n/, ERB::Util.html_escape(value)).gsub(/%u/, ERB::Util.html_escape(unit)).html_safe rescue number end diff --git a/actionpack/test/template/number_helper_test.rb b/actionpack/test/template/number_helper_test.rb index b6542ef29d4ce..a7dde41a073c9 100644 --- a/actionpack/test/template/number_helper_test.rb +++ b/actionpack/test/template/number_helper_test.rb @@ -24,9 +24,10 @@ def test_number_to_currency assert_equal("$1,234,567,890.51", number_to_currency(1234567890.506)) assert_equal("$1,234,567,892", number_to_currency(1234567891.50, {:precision => 0})) assert_equal("$1,234,567,890.5", number_to_currency(1234567890.50, {:precision => 1})) - assert_equal("£1234567890,50", number_to_currency(1234567890.50, {:unit => "£", :separator => ",", :delimiter => ""})) + assert_equal("£1234567890,50", number_to_currency(1234567890.50, {:unit => raw("£"), :separator => ",", :delimiter => ""})) + assert_equal("£1234567890,50", number_to_currency(1234567890.50, {:unit => "£", :separator => ",", :delimiter => ""})) 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 => "Kč", :format => "%n %u"})) + assert_equal("1,234,567,890.50 Kč", number_to_currency("1234567890.50", {:unit => raw("Kč"), :format => "%n %u"})) #assert_equal("$x.", number_to_currency("x")) # fails due to API consolidation assert_equal("$x", number_to_currency("x")) assert_nil number_to_currency(nil) From 527f11e3ac44696833e2e25da849d39d60e498e3 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Tue, 3 Dec 2013 14:07:10 -0800 Subject: [PATCH 2/2] Apply fix for CVE-2013-6417 https://groups.google.com/d/msg/ruby-security-ann/niK4drpSHT4/g8JW8ZsayRkJ --- actionpack/lib/action_controller/request.rb | 4 ++-- .../request/query_string_parsing_test.rb | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb index 81817d02fb5da..58d4306b36112 100755 --- a/actionpack/lib/action_controller/request.rb +++ b/actionpack/lib/action_controller/request.rb @@ -423,13 +423,13 @@ def form_data? # Override Rack's GET method to support indifferent access def GET - @env["action_controller.request.query_parameters"] ||= normalize_parameters(super) + @env["action_controller.request.query_parameters"] ||= deep_munge(normalize_parameters(super)) end alias_method :query_parameters, :GET # Override Rack's POST method to support indifferent access def POST - @env["action_controller.request.request_parameters"] ||= normalize_parameters(super) + @env["action_controller.request.request_parameters"] ||= deep_munge(normalize_parameters(super)) end alias_method :request_parameters, :POST diff --git a/actionpack/test/controller/request/query_string_parsing_test.rb b/actionpack/test/controller/request/query_string_parsing_test.rb index f60ea221f052d..d9fdd3182a397 100644 --- a/actionpack/test/controller/request/query_string_parsing_test.rb +++ b/actionpack/test/controller/request/query_string_parsing_test.rb @@ -12,6 +12,18 @@ def parse end end + class EarlyParse + def initialize(app) + @app = app + end + + def call(env) + # Trigger a Rack parse so that env caches the query params + Rack::Request.new(env).params + @app.call(env) + end + end + def teardown TestController.last_query_parameters = nil end @@ -121,6 +133,8 @@ def assert_parses(expected, actual) map.connect ':action', :controller => "query_string_parsing_test/test" end + ActionController::Dispatcher.middleware.use(EarlyParse) unless ActionController::Dispatcher.middleware.include? EarlyParse + get "/parse", actual assert_response :ok assert_equal(expected, TestController.last_query_parameters)