Attachment 674512 Details for Bug 892870 – actionpack-CVE-2012-0156-3-2-xml_parsing.patch

[patch] actionpack-CVE-2012-0156-3-2-xml_parsing.patch

actionpack-CVE-2012-0156-3-2-xml_parsing.patch (text/plain), 6.35 KB, created by Kurt Seifried on 2013-01-08 06:04:05 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kurt Seifried

Created: 2013-01-08 06:04:05 UTC

Size: 6.35 KB

patch

obsolete

>From 6934e74e6b8f8d3ffc7a170ae79a346f813c6cc9 Mon Sep 17 00:00:00 2001
>From: Jeremy Kemper <jeremy@bitsweat.net>
>Date: Sat, 5 Jan 2013 17:46:26 -0700
>Subject: [PATCH 2/2] Safe XML params parsing. Doesn't allow symbols or yaml.
> [CVE-2013-156]
>
>---
> .../action_dispatch/middleware/params_parser.rb    |    2 +-
> actionpack/test/controller/webservice_test.rb      |   10 +++++++
> activesupport/CHANGELOG.md                         |    4 +++
> .../active_support/core_ext/hash/conversions.rb    |   28 +++++++++++++++-----
> activesupport/test/core_ext/hash_ext_test.rb       |   16 +++++++++++
> 5 files changed, 52 insertions(+), 8 deletions(-)
>
>diff --git a/actionpack/lib/action_dispatch/middleware/params_parser.rb b/actionpack/lib/action_dispatch/middleware/params_parser.rb
>index ac72689..3a68335 100644
>--- a/actionpack/lib/action_dispatch/middleware/params_parser.rb
>+++ b/actionpack/lib/action_dispatch/middleware/params_parser.rb
>@@ -38,7 +38,7 @@ module ActionDispatch
>         when Proc
>           strategy.call(request.raw_post)
>         when :xml_simple, :xml_node
>-          data = request.deep_munge(Hash.from_xml(request.body.read) || {})
>+          data = request.deep_munge(Hash.from_safe_xml(request.body.read) || {})
>           request.body.rewind if request.body.respond_to?(:rewind)
>           data.with_indifferent_access
>         when :yaml
>diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb
>index ae8588c..d2b2c80 100644
>--- a/actionpack/test/controller/webservice_test.rb
>+++ b/actionpack/test/controller/webservice_test.rb
>@@ -118,6 +118,16 @@ class WebServiceTest < ActionDispatch::IntegrationTest
>     end
>   end
> 
>+  def test_post_xml_using_a_disallowed_type_attribute
>+    with_test_route_set do
>+      post '/', '<foo type="symbol">value</foo>', 'CONTENT_TYPE' => 'application/xml'
>+      assert_response 500
>+
>+      post '/', '<foo type="yaml">value</foo>', 'CONTENT_TYPE' => 'application/xml'
>+      assert_response 500
>+    end
>+  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..e161490 100644
>--- a/activesupport/CHANGELOG.md
>+++ b/activesupport/CHANGELOG.md
>@@ -1,3 +1,7 @@
>+## Rails 3.2.10 ##
>+
>+* Safe XML params parsing. Doesn't allow symbols or yaml. [CVE-2013-156]
>+
> ## 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..e41fa78 100644
>--- a/activesupport/lib/active_support/core_ext/hash/conversions.rb
>+++ b/activesupport/lib/active_support/core_ext/hash/conversions.rb
>@@ -85,15 +85,29 @@ class Hash
>     end
>   end
> 
>+  class DisallowedType < StandardError #:nodoc:
>+    def initialize(type)
>+      super "Disallowed type attribute: #{type.inspect}"
>+    end
>+  end
>+
>   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_safe_xml(xml)
>+      from_xml xml, %w(symbol yaml)
>     end
> 
>     private
>-      def typecast_xml_value(value)
>+      def typecast_xml_value(value, disallowed_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 +115,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 +141,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 => #<StringIO> } into { :files => #<StringIO> } 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..981fbaf 100644
>--- a/activesupport/test/core_ext/hash_ext_test.rb
>+++ b/activesupport/test/core_ext/hash_ext_test.rb
>@@ -1008,6 +1008,22 @@ 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 '<product><name type="foo">value</name></product>', %w(foo)
>+    end
>+  end
>+
>+  def test_from_safe_xml_raises_on_disallowed_type_attributes
>+    assert_raise Hash::DisallowedType do
>+      Hash.from_safe_xml '<product><name type="symbol">value</name></product>'
>+    end
>+
>+    assert_raise Hash::DisallowedType do
>+      Hash.from_safe_xml '<product><name type="yaml">value</name></product>'
>+    end
>+  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)
>

Actions: View | Diff

Attachments on bug 892870: 674509 | 674510 | 674511 | 674512 | 675064 | 675066 | 675067 | 675068 | 675069 | 675070 | 675071 | 675072 | 675077 | 675078 | 675079 | 675080