Browse files

stop calling to_sym when building arel nodes [CVE-2013-1854]

  • Loading branch information...
1 parent 3f8eb4e commit 5ff6012dda560ac2bf7d25cba11422edde7c11c3 @tenderlove tenderlove committed Mar 5, 2013
2 activerecord/lib/active_record/relation.rb
@@ -403,7 +403,7 @@ def where_values_hash == table_name
- Hash[ { |where| [, where.right] }]
+ Hash[ { |where| [, where.right] }].with_indifferent_access
def scope_for_create
2 activerecord/lib/active_record/relation/predicate_builder.rb
@@ -20,7 +20,7 @@ def self.build_from_hash(engine, attributes, default_table, allow_table_name = t
table =, engine)
- attribute = table[column.to_sym]
+ attribute = table[column]
case value
when ActiveRecord::Relation
10 activerecord/test/cases/method_scoping_test.rb
@@ -212,14 +212,14 @@ def test_scope_for_create_only_uses_equal
table = VerySpecialComment.arel_table
relation = VerySpecialComment.scoped
relation.where_values << table[:id].not_eq(1)
- assert_equal({:type => "VerySpecialComment"}, relation.send(:scope_for_create))
+ assert_equal({'type' => "VerySpecialComment"}, relation.send(:scope_for_create))
def test_scoped_create
new_comment = nil
VerySpecialComment.send(:with_scope, :create => { :post_id => 1 }) do
- assert_equal({:post_id => 1, :type => 'VerySpecialComment' }, VerySpecialComment.scoped.send(:scope_for_create))
+ assert_equal({'post_id' => 1, 'type' => 'VerySpecialComment' }, VerySpecialComment.scoped.send(:scope_for_create))
new_comment = VerySpecialComment.create :body => "Wonderful world"
@@ -228,7 +228,7 @@ def test_scoped_create
def test_scoped_create_with_join_and_merge
Comment.where(:body => "but Who's Buying?").joins(:post).merge(Post.where(:body => 'Peace Sells...')).with_scope do
- assert_equal({:body => "but Who's Buying?"}, Comment.scoped.scope_for_create)
+ assert_equal({'body' => "but Who's Buying?"}, Comment.scoped.scope_for_create)
@@ -441,7 +441,7 @@ def test_nested_scoped_create
comment = nil
Comment.send(:with_scope, :create => { :post_id => 1}) do
Comment.send(:with_scope, :create => { :post_id => 2}) do
- assert_equal({:post_id => 2}, Comment.scoped.send(:scope_for_create))
+ assert_equal({'post_id' => 2}, Comment.scoped.send(:scope_for_create))
comment = Comment.create :body => "Hey guys, nested scopes are broken. Please fix!"
@@ -453,7 +453,7 @@ def test_nested_exclusive_scope_for_create
Comment.send(:with_scope, :create => { :body => "Hey guys, nested scopes are broken. Please fix!" }) do
Comment.send(:with_exclusive_scope, :create => { :post_id => 1 }) do
- assert_equal({:post_id => 1}, Comment.scoped.send(:scope_for_create))
+ assert_equal({'post_id' => 1}, Comment.scoped.send(:scope_for_create))
comment = Comment.create :body => "Hey guys"
6 activerecord/test/cases/relation_test.rb
@@ -71,7 +71,7 @@ def test_empty_where_values_hash
def test_has_values
relation = Post, Post.arel_table
relation.where_values << relation.table[:id].eq(10)
- assert_equal({:id => 10}, relation.where_values_hash)
+ assert_equal({'id' => 10}, relation.where_values_hash)
def test_values_wrong_table
@@ -101,7 +101,7 @@ def test_scope_for_create
def test_create_with_value
relation = Post, Post.arel_table
- hash = { :hello => 'world' }
+ hash = { 'hello' => 'world' }
relation.create_with_value = hash
assert_equal hash, relation.scope_for_create
@@ -110,7 +110,7 @@ def test_create_with_value_with_wheres
relation = Post, Post.arel_table
relation.where_values << relation.table[:id].eq(10)
relation.create_with_value = {:hello => 'world'}
- assert_equal({:hello => 'world', :id => 10}, relation.scope_for_create)
+ assert_equal({'hello' => 'world', 'id' => 10}, relation.scope_for_create)
# FIXME: is this really wanted or expected behavior?

4 comments on commit 5ff6012


I'm thinking that a hypothetical HashEnforcingSymbolKeys and a with_enforced_symbol_keys would have prevented the unintended side effect of the scopes overwriting other scopes bug in #9813 :)

It just seems like more, instead of less, "key discipline" is needed. This is not the first time, btw, that I've seen weird bugs after manipulation of hash objects between string and symbol keys (or agnosticizing via .with_indifferent_access) or, for example, wrapping objects mixing both hash types in a wrapper class which one then tries to set values through (we specifically saw a nasty bug in our own code which only occurred when wrapping a middleware env hash in an ActionDispatch::Request and then setting values through it, for example... Entire sessions got lost, randomly!). I have become very suspicious of HashWithIndifferentAccess due to that ambiguity. The cost of access convenience (coupled with mutability) seems to be a major recipe for unintended side effects.

I put some corner-case examples demonstrating my concern here:
Notice how Hash[] is not overwritten correctly by HashWithIndifferentAccess[]=, allowing symbol keys to sneak in, probably directly leading to this bug. But I also have general concerns around merges.


@pmarreck solution sounds good. Demanding that the keys in a hash style query would actually make things a lot simpler.

  • the framework will not call .to_sym any longer, so no DOS attack
  • if the key is not a Symbol, it's not a column, it's a value or it errors. This avoids SQL injection

Well, browsers can't send symbols in forms :) , so a conversion would still have to happen somewhere, but it should happen once and once only and in one location and not be scattered all willy-nilly around the code haphazardly (first it's a symbol, then it's a string, then it doesn't care because it's with_indifferent_access, then it's merged with a hash with symbol keys, or string keys, etc.)

I am thinking that once you do that conversion, in one place, that's it. Everything's either symbols or strings from there on out and raises otherwise if you try to merge with a hash that contains keys of a differing key class or you try to set or get via the wrong key class (to be strict, and avoid this entire class of bugs).

Considerations: weird symbol possibilities like :"a[weird_symbol]" and the fact that symbols are never garbage-collected.


Well, that's the thing. If we assume the browser doesn't send symbols ( they can, we've sen that ) and we demand that Hash type where's should use symbols for column names; then the developer has to do the casting.
This makes it safe from the frameworks point of view.

Please sign in to comment.