From 70adb9613e4a40c5645c99da374639c41012e4fc Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Sat, 5 Jan 2013 17:46:26 -0700 Subject: [PATCH] CVE-2013-0156: Safe XML params parsing. Doesn't allow symbols or yaml. --- actionpack/test/controller/webservice_test.rb | 13 ++++++++ activesupport/CHANGELOG | 6 ++++ .../active_support/core_ext/hash/conversions.rb | 31 +++++++++++++++----- activesupport/test/core_ext/hash_ext_test.rb | 30 ++++++++++++++----- 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb index e89d6bb..9fea20d 100644 --- a/actionpack/test/controller/webservice_test.rb +++ b/actionpack/test/controller/webservice_test.rb @@ -121,6 +121,19 @@ class WebServiceTest < ActionController::IntegrationTest end end + def test_post_xml_using_a_disallowed_type_attribute + $stderr = StringIO.new + with_test_route_set do + post '/', 'value', 'CONTENT_TYPE' => 'application/xml' + assert_response 500 + + post '/', 'value', 'CONTENT_TYPE' => 'application/xml' + assert_response 500 + end + ensure + $stderr = STDERR + end + def test_register_and_use_yaml with_test_route_set do ActionController::Base.param_parsers[Mime::YAML] = Proc.new { |d| YAML.load(d) } diff --git a/activesupport/CHANGELOG b/activesupport/CHANGELOG index 600b570..eb2efaa 100644 --- a/activesupport/CHANGELOG +++ b/activesupport/CHANGELOG @@ -1,5 +1,11 @@ +## Rails 2.3.15 (Jan 8, 2012) ## + +* Hash.from_xml raises when it encounters type="symbol" or type="yaml". Use Hash.from_trusted_xml to parse this XML. CVE-2013-0156 [Jeremy Kemper] + + *2.3.11 (February 9, 2011)* + *2.3.10 (October 15, 2010)* diff --git a/activesupport/lib/active_support/core_ext/hash/conversions.rb b/activesupport/lib/active_support/core_ext/hash/conversions.rb index a43763f..d7a8c1e 100644 --- a/activesupport/lib/active_support/core_ext/hash/conversions.rb +++ b/activesupport/lib/active_support/core_ext/hash/conversions.rb @@ -26,6 +26,13 @@ module ActiveSupport #:nodoc: end end + DISALLOWED_XML_TYPES = %w(symbol yaml) + class DisallowedType < StandardError #:nodoc: + def initialize(type) + super "Disallowed type attribute: #{type.inspect}" + end + end + XML_TYPE_NAMES = { "Symbol" => "symbol", "Fixnum" => "integer", @@ -160,14 +167,24 @@ module ActiveSupport #:nodoc: end module ClassMethods - def from_xml(xml) - typecast_xml_value(unrename_keys(XmlMini.parse(xml))) + def from_xml(xml, disallowed_types = nil) + typecast_xml_value(unrename_keys(XmlMini.parse(xml)), disallowed_types) + end + + def from_trusted_xml(xml) + from_xml xml, [] end private - def typecast_xml_value(value) + def typecast_xml_value(value, disallowed_types = nil) + disallowed_types ||= DISALLOWED_XML_TYPES + case value.class.to_s when 'Hash' + if value.include?('type') && !value['type'].is_a?(Hash) && disallowed_types.include?(value['type']) + raise DisallowedType, value['type'] + end + if value['type'] == 'array' child_key, entries = value.detect { |k,v| k != 'type' } # child_key is throwaway if entries.nil? || (c = value['__content__'] && c.blank?) @@ -175,9 +192,9 @@ module ActiveSupport #:nodoc: else case entries.class.to_s # something weird with classes not matching here. maybe singleton methods breaking is_a? when "Array" - entries.collect { |v| typecast_xml_value(v) } + entries.collect { |v| typecast_xml_value(v, disallowed_types) } when "Hash" - [typecast_xml_value(entries)] + [typecast_xml_value(entries, disallowed_types)] else raise "can't typecast #{entries.inspect}" end @@ -205,7 +222,7 @@ module ActiveSupport #:nodoc: nil else xml_value = value.inject({}) do |h,(k,v)| - h[k] = typecast_xml_value(v) + h[k] = typecast_xml_value(v, disallowed_types) h end @@ -214,7 +231,7 @@ module ActiveSupport #:nodoc: xml_value["file"].is_a?(StringIO) ? xml_value["file"] : xml_value end when 'Array' - value.map! { |i| typecast_xml_value(i) } + value.map! { |i| typecast_xml_value(i, disallowed_types) } case value.length when 0 then nil when 1 then value.first diff --git a/activesupport/test/core_ext/hash_ext_test.rb b/activesupport/test/core_ext/hash_ext_test.rb index 44308c3..80873b4 100644 --- a/activesupport/test/core_ext/hash_ext_test.rb +++ b/activesupport/test/core_ext/hash_ext_test.rb @@ -575,12 +575,10 @@ class HashToXmlTest < Test::Unit::TestCase 2592000000 2003-07-16 2003-07-16T09:28:00+0000 - --- \n1: should be an integer\n:message: Have a nice day\narray: \n- should-have-dashes: true\n should_have_underscores: true\n david@loudthinking.com 1.5 135 - yes EOT @@ -593,12 +591,10 @@ class HashToXmlTest < Test::Unit::TestCase :replies_close_in => 2592000000, :written_on => Date.new(2003, 7, 16), :viewed_at => Time.utc(2003, 7, 16, 9, 28), - :content => { :message => "Have a nice day", 1 => "should be an integer", "array" => [{ "should-have-dashes" => true, "should_have_underscores" => true }] }, :author_email_address => "david@loudthinking.com", :parent_id => nil, :ad_revenue => BigDecimal("1.50"), :optimum_viewing_angle => 135.0, - :resident => :yes }.stringify_keys assert_equal expected_topic_hash, Hash.from_xml(topic_xml)["topic"] @@ -612,7 +608,6 @@ class HashToXmlTest < Test::Unit::TestCase - EOT @@ -623,7 +618,6 @@ class HashToXmlTest < Test::Unit::TestCase :approved => nil, :written_on => nil, :viewed_at => nil, - :content => nil, :parent_id => nil }.stringify_keys @@ -833,6 +827,28 @@ class HashToXmlTest < Test::Unit::TestCase assert_equal expected_product_hash, Hash.from_xml(product_xml)["product"] end + def test_from_xml_raises_on_disallowed_type_attributes + assert_raise Hash::DisallowedType do + Hash.from_xml 'value', %w(foo) + end + end + + def test_from_xml_disallows_symbol_and_yaml_types_by_default + assert_raise Hash::DisallowedType do + Hash.from_xml 'value' + end + + assert_raise Hash::DisallowedType do + Hash.from_xml 'value' + end + end + + def test_from_trusted_xml_allows_symbol_and_yaml_types + expected = { 'product' => { 'name' => :value }} + assert_equal expected, Hash.from_trusted_xml('value') + assert_equal expected, Hash.from_trusted_xml(':value') + end + def test_should_use_default_value_for_unknown_key hash_wia = HashWithIndifferentAccess.new(3) assert_equal 3, hash_wia[:new_key] @@ -867,7 +883,7 @@ class HashToXmlTest < Test::Unit::TestCase def test_empty_string_works_for_typecast_xml_value assert_nothing_raised do - Hash.__send__(:typecast_xml_value, "") + Hash.__send__(:typecast_xml_value, "", []) end end -- 1.7.10.2 (Apple Git-33)