From 43109ecb986470ef023a7e91beb9812718f000fe 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.md | 9 ++++++ .../active_support/core_ext/hash/conversions.rb | 32 +++++++++++++++----- activesupport/test/core_ext/hash_ext_test.rb | 28 +++++++++++++---- 4 files changed, 69 insertions(+), 13 deletions(-) diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb index ae8588c..13b6f4f 100644 --- a/actionpack/test/controller/webservice_test.rb +++ b/actionpack/test/controller/webservice_test.rb @@ -118,6 +118,19 @@ class WebServiceTest < ActionDispatch::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 with_params_parsers Mime::YAML => Proc.new { |d| YAML.load(d) } do diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 7faf55b..ec19805 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,12 @@ +## Rails 3.2.10 (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* + ## Rails 3.2.9 (Nov 12, 2012) ## * Add logger.push_tags and .pop_tags to complement logger.tagged: diff --git a/activesupport/lib/active_support/core_ext/hash/conversions.rb b/activesupport/lib/active_support/core_ext/hash/conversions.rb index 5f07bb4..b820a16 100644 --- a/activesupport/lib/active_support/core_ext/hash/conversions.rb +++ b/activesupport/lib/active_support/core_ext/hash/conversions.rb @@ -85,15 +85,33 @@ class Hash end end + class DisallowedType < StandardError #:nodoc: + def initialize(type) + super "Disallowed type attribute: #{type.inspect}" + end + end + + DISALLOWED_XML_TYPES = %w(symbol yaml) + class << self - def from_xml(xml) - typecast_xml_value(unrename_keys(ActiveSupport::XmlMini.parse(xml))) + def from_xml(xml, disallowed_types = nil) + typecast_xml_value(unrename_keys(ActiveSupport::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' _, entries = Array.wrap(value.detect { |k,v| not v.is_a?(String) }) if entries.nil? || (c = value['__content__'] && c.blank?) @@ -101,9 +119,9 @@ class Hash 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 @@ -127,14 +145,14 @@ class Hash elsif value['type'] && value.size == 1 && !value['type'].is_a?(::Hash) nil else - xml_value = Hash[value.map { |k,v| [k, typecast_xml_value(v)] }] + xml_value = Hash[value.map { |k,v| [k, typecast_xml_value(v, disallowed_types)] }] # Turn { :files => { :file => # } into { :files => # } so it is compatible with # how multipart uploaded files from HTML appear 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) } value.length > 1 ? value : value.first when 'String' value diff --git a/activesupport/test/core_ext/hash_ext_test.rb b/activesupport/test/core_ext/hash_ext_test.rb index b5eb049..c3a5954 100644 --- a/activesupport/test/core_ext/hash_ext_test.rb +++ b/activesupport/test/core_ext/hash_ext_test.rb @@ -733,12 +733,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 @@ -751,12 +749,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"] @@ -770,7 +766,6 @@ class HashToXmlTest < Test::Unit::TestCase - EOT @@ -781,7 +776,6 @@ class HashToXmlTest < Test::Unit::TestCase :approved => nil, :written_on => nil, :viewed_at => nil, - :content => nil, :parent_id => nil }.stringify_keys @@ -1008,6 +1002,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] -- 1.7.10.2 (Apple Git-33)