Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

predicate builder should not recurse for determining where columns.

Thanks to Ben Murphy for reporting this

CVE-2012-2661
  • Loading branch information...
commit 9340f89849606dba02f44038171f3837f883fd4e 1 parent 344ea04
@tenderlove tenderlove authored
View
19 activerecord/lib/active_record/associations/association_scope.rb
@@ -96,7 +96,7 @@ def add_constraints(scope)
conditions.each do |condition|
if options[:through] && condition.is_a?(Hash)
- condition = { table.name => condition }
+ condition = disambiguate_condition(table, condition)
end
scope = scope.where(interpolate(condition))
@@ -113,7 +113,7 @@ def add_constraints(scope)
conditions.each do |condition|
condition = interpolate(condition)
- condition = { (table.table_alias || table.name) => condition } unless i == 0
+ condition = disambiguate_condition(table, condition) unless i == 0
scope = scope.where(condition)
end
@@ -138,6 +138,21 @@ def table_name_for(reflection)
end
end
+ def disambiguate_condition(table, condition)
+ if condition.is_a?(Hash)
+ Hash[
+ condition.map do |k, v|
+ if v.is_a?(Hash)
+ [k, v]
+ else
+ [table.table_alias || table.name, { k => v }]
+ end
+ end
+ ]
+ else
+ condition
+ end
+ end
end
end
end
View
2  activerecord/lib/active_record/relation/predicate_builder.rb
@@ -6,7 +6,7 @@ def self.build_from_hash(engine, attributes, default_table)
if value.is_a?(Hash)
table = Arel::Table.new(column, engine)
- build_from_hash(engine, value, table)
+ value.map { |k,v| build(table[k.to_sym], v) }
else
column = column.to_s
View
19 activerecord/test/cases/relation/where_test.rb
@@ -0,0 +1,19 @@
+require "cases/helper"
+require 'models/post'
+
+module ActiveRecord
+ class WhereTest < ActiveRecord::TestCase
+ fixtures :posts
+
+ def test_where_error
+ assert_raises(ActiveRecord::StatementInvalid) do
+ Post.where(:id => { 'posts.author_id' => 10 }).first
+ end
+ end
+
+ def test_where_with_table_name
+ post = Post.first
+ assert_equal post, Post.where(:posts => { 'id' => post.id }).first
+ end
+ end
+end

2 comments on commit 9340f89

@joernchen

So the fix for this CVE could need some more additon IMHO.

I observed the following:

Rails < 3.2.4 in some random sample blog app:

Post.where(:id=>{'posts.id'=>1}) gives me a Post back

Rails 3.2.5 won't so that's where i assume this fix comes into play.

But in 3.2.5 and earlier Post.where(:id=>{}) gives me all the posts in an array.

So whenever someone puts up a controller for instance for password reset and does the following

User.where(:token => params[:token])

this might acutally go wrong when someone else posts an empty hash as token.

I hope this makes sense somehow ;)

@NZKoz
Owner

@joernchen the issue with the previous code was that it broke people's expectations because the common practice of:

  if !params[:token].blank?
    user = User.where(:token=>params[:token])
  end

Attackers can make you pass all sorts of values you don't intend, you'll always need to have some checks in your code. We simply addressed this one because it was such a common piece of code, and it was broken.

Please sign in to comment.
Something went wrong with that request. Please try again.