Attachment 675074 Details for Bug 892866 – actionpack-CVE-2013-0155-3-0-null_array_param.patch

[patch] actionpack-CVE-2013-0155-3-0-null_array_param.patch

actionpack-CVE-2013-0155-3-0-null_array_param.patch (text/plain), 7.67 KB, created by Kurt Seifried on 2013-01-08 21:24:42 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kurt Seifried

Created: 2013-01-08 21:24:42 UTC

Size: 7.67 KB

patch

obsolete

>From f943e386039e0f28e777e2cf7ec39a7dbe24c040 Mon Sep 17 00:00:00 2001
>From: Aaron Patterson <aaron.patterson@gmail.com>
>Date: Fri, 4 Jan 2013 12:02:22 -0800
>Subject: [PATCH 1/2] * Strip nils from collections on JSON and XML posts.
> [CVE-2013-0155] * dealing with empty hashes. Thanks
> Damien Mathieu
>
>Conflicts:
>	actionpack/CHANGELOG.md
>	activerecord/CHANGELOG.md
>
>Conflicts:
>	actionpack/CHANGELOG.md
>	activerecord/CHANGELOG.md
>	activerecord/lib/active_record/relation/predicate_builder.rb
>---
> actionpack/CHANGELOG                                  |    4 ++++
> actionpack/lib/action_dispatch/http/request.rb        |   10 ++++------
> .../lib/action_dispatch/middleware/params_parser.rb   |    4 ++--
> .../test/dispatch/request/json_params_parsing_test.rb |   15 +++++++++++++++
> .../test/dispatch/request/xml_params_parsing_test.rb  |   17 +++++++++++++++++
> activerecord/CHANGELOG                                |    4 ++++
> .../lib/active_record/relation/predicate_builder.rb   |    7 ++++++-
> activerecord/test/cases/relation/where_test.rb        |   16 +++++++++++++++-
> 8 files changed, 67 insertions(+), 10 deletions(-)
>
>diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
>index 37d94e3..0092d0d 100644
>--- a/actionpack/CHANGELOG
>+++ b/actionpack/CHANGELOG
>@@ -1,3 +1,7 @@
>+## Rails 3.0.19
>+
>+* Strip nils from collections on JSON and XML posts. [CVE-2013-0155]
>+
> ## Rails 3.0.18
> 
> ## Rails 3.0.17 (Aug 9, 2012)
>diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
>index 04b4a21..8767acb 100644
>--- a/actionpack/lib/action_dispatch/http/request.rb
>+++ b/actionpack/lib/action_dispatch/http/request.rb
>@@ -258,18 +258,14 @@ module ActionDispatch
>       LOCALHOST.any? { |local_ip| local_ip === remote_addr && local_ip === remote_ip }
>     end
> 
>-    protected
>-
>     # Remove nils from the params hash
>     def deep_munge(hash)
>-      keys = hash.keys.find_all { |k| hash[k] == [nil] }
>-      keys.each { |k| hash[k] = nil }
>-
>-      hash.each_value do |v|
>+      hash.each do |k, v|
>         case v
>         when Array
>           v.grep(Hash) { |x| deep_munge(x) }
>           v.compact!
>+          hash[k] = nil if v.empty?
>         when Hash
>           deep_munge(v)
>         end
>@@ -278,6 +274,8 @@ module ActionDispatch
>       hash
>     end
> 
>+    protected
>+
>     def parse_query(qs)
>       deep_munge(super)
>     end
>diff --git a/actionpack/lib/action_dispatch/middleware/params_parser.rb b/actionpack/lib/action_dispatch/middleware/params_parser.rb
>index d4208ca..aaf9680 100644
>--- a/actionpack/lib/action_dispatch/middleware/params_parser.rb
>+++ b/actionpack/lib/action_dispatch/middleware/params_parser.rb
>@@ -38,13 +38,13 @@ module ActionDispatch
>         when Proc
>           strategy.call(request.raw_post)
>         when :xml_simple, :xml_node
>-          data = Hash.from_xml(request.body.read) || {}
>+          data = request.deep_munge(Hash.from_xml(request.body.read) || {})
>           request.body.rewind if request.body.respond_to?(:rewind)
>           data.with_indifferent_access
>         when :yaml
>           YAML.load(request.raw_post)
>         when :json
>-          data = ActiveSupport::JSON.decode(request.body)
>+          data = request.deep_munge ActiveSupport::JSON.decode(request.body)
>           request.body.rewind if request.body.respond_to?(:rewind)
>           data = {:_json => data} unless data.is_a?(Hash)
>           data.with_indifferent_access
>diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb
>index 0faa99a..407fbe1 100644
>--- a/actionpack/test/dispatch/request/json_params_parsing_test.rb
>+++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb
>@@ -30,6 +30,21 @@ class JsonParamsParsingTest < ActionController::IntegrationTest
>     )
>   end
> 
>+  test "nils are stripped from collections" do
>+    assert_parses(
>+      {"person" => nil},
>+      "{\"person\":[null]}", { 'CONTENT_TYPE' => 'application/json' }
>+    )
>+    assert_parses(
>+      {"person" => ['foo']},
>+      "{\"person\":[\"foo\",null]}", { 'CONTENT_TYPE' => 'application/json' }
>+    )
>+    assert_parses(
>+      {"person" => nil},
>+      "{\"person\":[null, null]}", { 'CONTENT_TYPE' => 'application/json' }
>+    )
>+  end
>+
>   test "logs error if parsing unsuccessful" do
>     with_test_routing do
>       begin
>diff --git a/actionpack/test/dispatch/request/xml_params_parsing_test.rb b/actionpack/test/dispatch/request/xml_params_parsing_test.rb
>index d44c642..f58c8fb 100644
>--- a/actionpack/test/dispatch/request/xml_params_parsing_test.rb
>+++ b/actionpack/test/dispatch/request/xml_params_parsing_test.rb
>@@ -29,6 +29,23 @@ class XmlParamsParsingTest < ActionController::IntegrationTest
>     assert_equal "<ok>bar</ok>", resp.body
>   end
> 
>+  def assert_parses(expected, xml)
>+    with_test_routing do
>+      post "/parse", xml, default_headers
>+      assert_response :ok
>+      assert_equal(expected, TestController.last_request_parameters)
>+    end
>+  end
>+
>+  test "nils are stripped from collections" do
>+    assert_parses(
>+      {"hash" => { "person" => nil} },
>+      "<hash><person type=\"array\"><person nil=\"true\"/></person></hash>")
>+    assert_parses(
>+      {"hash" => { "person" => ['foo']} },
>+      "<hash><person type=\"array\"><person>foo</person><person nil=\"true\"/></person>\n</hash>")
>+  end
>+
>   test "parses hash params" do
>     with_test_routing do
>       xml = "<person><name>David</name></person>"
>diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
>index 0ee87af..a8aa147 100644
>--- a/activerecord/CHANGELOG
>+++ b/activerecord/CHANGELOG
>@@ -1,3 +1,7 @@
>+## Rails 3.0.19
>+
>+*   Fix querying with an empty hash *Damien Mathieu* [CVE-2013-0155]
>+
> ## Rails 3.0.18
> 
> * CVE-2012-5664 ensure that options are never taken from the first parameter
>diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb
>index e74ba73..71eabcb 100644
>--- a/activerecord/lib/active_record/relation/predicate_builder.rb
>+++ b/activerecord/lib/active_record/relation/predicate_builder.rb
>@@ -11,7 +11,12 @@ module ActiveRecord
> 
>         if allow_table_name && value.is_a?(Hash)
>           table = Arel::Table.new(column, :engine => @engine)
>-          build_from_hash(value, table, false)
>+
>+          if value.empty?
>+            '1 = 2'
>+          else
>+            build_from_hash(value, table, false)
>+          end
>         else
>           column = column.to_s
> 
>diff --git a/activerecord/test/cases/relation/where_test.rb b/activerecord/test/cases/relation/where_test.rb
>index b9eef1d..8015833 100644
>--- a/activerecord/test/cases/relation/where_test.rb
>+++ b/activerecord/test/cases/relation/where_test.rb
>@@ -1,9 +1,11 @@
> require "cases/helper"
> require 'models/post'
>+require 'models/comment'
>+require 'models/edge'
> 
> module ActiveRecord
>   class WhereTest < ActiveRecord::TestCase
>-    fixtures :posts
>+    fixtures :posts, :edges
> 
>     def test_where_error
>       assert_raises(ActiveRecord::StatementInvalid) do
>@@ -21,5 +23,17 @@ module ActiveRecord
>       post = Post.first
>       assert_equal post, Post.where(:posts => { 'id' => post.id }).first
>     end
>+
>+    def test_where_with_table_name_and_empty_hash
>+      assert_equal 0, Post.where(:posts => {}).count
>+    end
>+
>+    def test_where_with_table_name_and_empty_array
>+      assert_equal 0, Post.where(:id => []).count
>+    end
>+
>+    def test_where_with_empty_hash_and_no_foreign_key
>+      assert_equal 0, Edge.where(:sink => {}).count
>+    end
>   end
> end
>-- 
>1.7.10.2 (Apple Git-33)
>

Actions: View | Diff

Attachments on bug 892866: 674505 | 674506 | 674507 | 674508 | 675074 | 675075 | 675076