[prev in list] [next in list] [prev in thread] [next in thread]
List: oss-security
Subject: [oss-security] Unsafe Query Generation Risk in Ruby on Rails (CVE-2012-2660)
From: Aaron Patterson <tenderlove () ruby-lang ! org>
Date: 2012-05-31 19:15:29
Message-ID: 20120531191529.GB79783 () higgins ! local
[Download RAW message or body]
[Attachment #2 (multipart/mixed)]
Unsafe Query Generation Risk in Ruby on Rails
There is a vulnerability when Active Record is used in conjunction with parameter \
parsing from Rack via Action Pack. This vulnerability has been assigned the CVE \
identifier CVE-2012-2660.
Versions Affected: ALL versions
Not affected: NONE
Fixed Versions: 3.2.4, 3.1.5, 3.0.13
Impact
------
Due to the way Active Record interprets parameters in combination with the way that \
Rack parses query parameters, it is possible for an attacker to issue unexpected \
database queries with "IS NULL" where clauses. This issue does *not* let an attacker \
insert arbitrary values into an SQL query, however they can cause the query to check \
for NULL where most users wouldn't expect it.
For example, a system has password reset with token functionality:
unless params[:token].nil?
user = User.find_by_token(params[:token])
user.reset_password!
end
An attacker can craft a request such that `params[:token]` will return `[nil]`. The \
`[nil]` value will bypass the test for nil, but will still add an "IS NULL" clause to \
the SQL query.
All users running an affected release should either upgrade or use one of the work \
arounds immediately.
Releases
--------
The FIXED releases are available at the normal locations.
Workarounds
-----------
This problem can be mitigated by testing for `[nil]`. For example:
unless params[:token].nil? || params[:token] == [nil]
user = User.find_by_token(params[:token])
user.reset_password!
end
Another possible workaround is to cast to a known type and test against that type. \
For example:
unless params[:token].to_s.empty?
user = User.find_by_token(params[:token])
user.reset_password!
end
Patches
-------
To aid users who aren't able to upgrade immediately we have provided patches for the \
two supported release series. They are in git-am format and consist of a single \
changeset.
* 3-0-null_param.patch - Patch for 3.0 series
* 3-1-null_param.patch - Patch for 3.1 series
* 3-2-null_param.patch - Patch for 3.2 series
Please note that only the 3.1.x and 3.2.x series are supported at present. Users of \
earlier unsupported releases are advised to upgrade as soon as possible as we cannot \
guarantee the continued availability of security fixes for unsupported releases.
Credits
-------
Thanks to Ben Murphy for reporting the vulnerability to us, and to Chad Pyne of \
thoughtbot for helping us verify the fix.
--
Aaron Patterson
http://tenderlovemaking.com/
["3-0-null_param.patch" (text/plain)]
From c202638225519b5e1a03ebe523b109c948fb0e52 Mon Sep 17 00:00:00 2001
From: Aaron Patterson <aaron.patterson@gmail.com>
Date: Wed, 30 May 2012 15:13:03 -0700
Subject: [PATCH] Strip [nil] from parameters hash. Thanks to Ben Murphy for
reporting this!
CVE-2012-2660
Conflicts:
actionpack/lib/action_dispatch/http/request.rb
---
actionpack/lib/action_dispatch/http/request.rb | 22 ++++++++++++++++++++
.../dispatch/request/query_string_parsing_test.rb | 7 +++++-
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/actionpack/lib/action_dispatch/http/request.rb \
b/actionpack/lib/action_dispatch/http/request.rb index 7c8557b..985b730 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -257,5 +257,27 @@ module ActionDispatch
def local?
LOCALHOST.any? { |local_ip| local_ip === remote_addr && local_ip === remote_ip \
} end
+
+ protected
+
+ # Remove nils from the params hash
+ def deep_munge(hash)
+ hash.each_value do |v|
+ case v
+ when Array
+ v.grep(Hash) { |x| deep_munge(x) }
+ when Hash
+ deep_munge(v)
+ end
+ end
+
+ keys = hash.keys.find_all { |k| hash[k] == [nil] }
+ keys.each { |k| hash[k] = nil }
+ hash
+ end
+
+ def parse_query(qs)
+ deep_munge(super)
+ end
end
end
diff --git a/actionpack/test/dispatch/request/query_string_parsing_test.rb \
b/actionpack/test/dispatch/request/query_string_parsing_test.rb index \
071d80c..c7ab700 100644
--- a/actionpack/test/dispatch/request/query_string_parsing_test.rb
+++ b/actionpack/test/dispatch/request/query_string_parsing_test.rb
@@ -81,7 +81,12 @@ class QueryStringParsingTest < ActionController::IntegrationTest
end
test "query string without equal" do
- assert_parses({ "action" => nil }, "action")
+ assert_parses({"action" => nil}, "action")
+ assert_parses({"action" => {"foo" => nil}}, "action[foo]")
+ assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar]")
+ assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar][]")
+ assert_parses({"action" => {"foo" => nil}}, "action[foo][]")
+ assert_parses({"action"=>{"foo"=>[{"bar"=>nil}]}}, "action[foo][][bar]")
end
test "query string with empty key" do
--
1.7.5.4
["3-1-null_param.patch" (text/plain)]
From 5b83bbfab7d5770ed56366d739ff62ac70425008 Mon Sep 17 00:00:00 2001
From: Aaron Patterson <aaron.patterson@gmail.com>
Date: Wed, 30 May 2012 15:13:03 -0700
Subject: [PATCH] Strip [nil] from parameters hash. Thanks to Ben Murphy for
reporting this!
CVE-2012-2660
---
actionpack/lib/action_dispatch/http/request.rb | 22 ++++++++++++++++++++
.../dispatch/request/query_string_parsing_test.rb | 7 +++++-
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/actionpack/lib/action_dispatch/http/request.rb \
b/actionpack/lib/action_dispatch/http/request.rb index c49cc56..9e1dc4b 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -267,6 +267,28 @@ 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)
+ hash.each_value do |v|
+ case v
+ when Array
+ v.grep(Hash) { |x| deep_munge(x) }
+ when Hash
+ deep_munge(v)
+ end
+ end
+
+ keys = hash.keys.find_all { |k| hash[k] == [nil] }
+ keys.each { |k| hash[k] = nil }
+ hash
+ end
+
+ def parse_query(qs)
+ deep_munge(super)
+ end
+
private
def check_method(name)
diff --git a/actionpack/test/dispatch/request/query_string_parsing_test.rb \
b/actionpack/test/dispatch/request/query_string_parsing_test.rb index \
f6a1475..181f51a 100644
--- a/actionpack/test/dispatch/request/query_string_parsing_test.rb
+++ b/actionpack/test/dispatch/request/query_string_parsing_test.rb
@@ -81,7 +81,12 @@ class QueryStringParsingTest < ActionDispatch::IntegrationTest
end
test "query string without equal" do
- assert_parses({ "action" => nil }, "action")
+ assert_parses({"action" => nil}, "action")
+ assert_parses({"action" => {"foo" => nil}}, "action[foo]")
+ assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar]")
+ assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar][]")
+ assert_parses({"action" => {"foo" => nil}}, "action[foo][]")
+ assert_parses({"action"=>{"foo"=>[{"bar"=>nil}]}}, "action[foo][][bar]")
end
test "query string with empty key" do
--
1.7.5.4
["3-2-null_param.patch" (text/plain)]
From dff6db18840e2fd1dd3f3e4ef0ae7a9a3986d01d Mon Sep 17 00:00:00 2001
From: Aaron Patterson <aaron.patterson@gmail.com>
Date: Wed, 30 May 2012 15:13:03 -0700
Subject: [PATCH] Strip [nil] from parameters hash. Thanks to Ben Murphy for
reporting this!
CVE-2012-2660
---
actionpack/lib/action_dispatch/http/request.rb | 22 ++++++++++++++++++++
.../dispatch/request/query_string_parsing_test.rb | 7 +++++-
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/actionpack/lib/action_dispatch/http/request.rb \
b/actionpack/lib/action_dispatch/http/request.rb index 8209212..adbb5d1 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -247,6 +247,28 @@ 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)
+ hash.each_value do |v|
+ case v
+ when Array
+ v.grep(Hash) { |x| deep_munge(x) }
+ when Hash
+ deep_munge(v)
+ end
+ end
+
+ keys = hash.keys.find_all { |k| hash[k] == [nil] }
+ keys.each { |k| hash[k] = nil }
+ hash
+ end
+
+ def parse_query(qs)
+ deep_munge(super)
+ end
+
private
def check_method(name)
diff --git a/actionpack/test/dispatch/request/query_string_parsing_test.rb \
b/actionpack/test/dispatch/request/query_string_parsing_test.rb index \
f6a1475..181f51a 100644
--- a/actionpack/test/dispatch/request/query_string_parsing_test.rb
+++ b/actionpack/test/dispatch/request/query_string_parsing_test.rb
@@ -81,7 +81,12 @@ class QueryStringParsingTest < ActionDispatch::IntegrationTest
end
test "query string without equal" do
- assert_parses({ "action" => nil }, "action")
+ assert_parses({"action" => nil}, "action")
+ assert_parses({"action" => {"foo" => nil}}, "action[foo]")
+ assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar]")
+ assert_parses({"action" => {"foo" => { "bar" => nil }}}, "action[foo][bar][]")
+ assert_parses({"action" => {"foo" => nil}}, "action[foo][]")
+ assert_parses({"action"=>{"foo"=>[{"bar"=>nil}]}}, "action[foo][][bar]")
end
test "query string with empty key" do
--
1.7.5.4
[Attachment #8 (application/pgp-signature)]
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic