active_record: Quote numeric values compared to string columns.

[dylanahsmith]

Problem

The problem is described in Potential Query Manipulation with Common Rails Practises, which notes that "it is not possible for ActiveRecord to automatically protect against all instances of this attack". However, it is possible to handle the case where the column type is known.

For example:

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

performs the query

SELECT * FROM `users` WHERE `login_token` = 0 LIMIT 1;

where it will match any login_token that starts with a non-digit character. But since active record can determine that login_token is a string type column, it can quote the integer it is being compared with as follows.

SELECT * FROM `users` WHERE `login_token` = '0' LIMIT 1;

which will have the desired affect of only matching the exact string '0'.

Solution

Modify the connection adapter's quote method to quote the numeric value as a SQL string if the column it is being compared with is known to be non-numeric. Boolean values are represented as the integers 0 or 1 for some databases, so booleans are also quoted as SQL strings when comparing against a string type column.

The PredicateBuilder also needed to be modified to ensure that integers are quoted, since Arel treats them as literals. Quoting the integer and passing it to Arel as a Arel::Nodes::SqlLiteral object will ensure that the value is not double quoted.

Limitations

SQL fragments with quoted values have no information about the column type, so the following is still unsafe.

User.where("login_token = ?", params[:token])

The parameters to the SQL fragment should still be converted to the correct type before passing it to active record as follows.

User.where("login_token = ?", params[:token].to_s)

[tenderlove]

👍

[NZKoz]

I'm not sure about the special-casing you're doing here for integers. Ideally every value should be quoted correctly?

[dylanahsmith]

I'm not sure about the special-casing you're doing here for integers. Ideally every value should be quoted correctly?

I'm not sure what you mean by here. In the predicate builder it is because Fixnum and Bignum are handled as literals by Arel (https://github.com/rails/arel/blob/master/lib/arel/visitors/to_sql.rb#L557). The quote method naturally is all special casing.

Perhaps some work can be done on Arel's side to remove the special casing in the predicate builder, specifically to make sure last_column is cleared before reaching actual literal integers like the arguments to LIMIT and OFFSET. After that it may be able to quote integers safely.

[NZKoz]

Mostly I'm referring to say:

WHERE login_count = '0'

We could actually quote that correctly for the column type. All that this pull request has implemented is hardening for the problem we're aware of now, chances are there are other issues lurking in the crazed-mind that created mysql's type casting code.

[dylanahsmith]

@NZKoz converting all values to their correct type if the column type is available would be great. I tried taking the naive approach of doing .to_s on :string and :text fields for non-NULL values, but this will break the case where an attribute hash is passed in for a text field and expects "'#{quote_string(YAML.dump(value))}'" to be done to the value.

I ended up deciding to simplify the scope of the fix to deal with the known security issue.

[willbryant]

I don't think this (or at least the 26e13c3 commit included in 3.1.11) is the right workaround for mysql.

First, looking up columns_hash for all referenced table now means it tries to SHOW FIELDS FROM tables that don't exist whenever you use the join table syntax documented in the 'Table Aliasing' section of the ActiveRecord associations help (look up has_many on api.rubyonrails.org).

Of course these tables don't actually exist, so the lookup fails, and the join query fails.

Secondly, doing it only for columns in table is the wrong fix because it doesn't work for comparisons expressions, so you won't realise that you are only protected some of the time.

IMHO because this is breaking apps upgrading to fix the other security vulnerabilities this should be reverted from 3.1 and 3.2 and have further discussion about the best solution for rails 4. The announcement about the vulnerability said this would not be fixed in these versions, it's a breaking change, and it only comes up as a vuln if you are allowing JSON or XML parsing or otherwise writing vulnerable code.

[NZKoz]

@willbryant I strongly disagree with reverting this, however it's clearly broken things and we'll need to ship fixes which don't.

'Only allowing JSON or XML' includes ~100% of applications, just because you're able to disable it and knew how doesn't mean others can or will and the consequences are too severe to just punt on it till 4.0 ships

Let's make sure we fix the regressions so that we're only quoting when we know it's for a string column, but removing the quoting changes altogether isn't an option

[willbryant]

Frankly I would disable JSON/XML params parsing by default if I had to pick one of the two to spring in an upgrade. A lot fewer people use those than use joins.

The mysql behavior is only an issue when the bind values are params that came in forced to a type you don't expect, right?

This is the only time you'll hear me say this, but: Perl's hacky tainting concept does potentially have some useful ideas here.

[willbryant]

"joins" was an exaggeration, sorry. "joins that invoke table aliasing".

[NZKoz]

joins that invoke table aliasing and include columns of those table aliases in their where clause. You're getting much thinner there than your initial exaggeration 😃

[willbryant]

Thinner than params from JSON or XML params being put straight into a comparison with string value in the database in a where clause? :)

[jimsynz]

lets keep it civil guys.

[NZKoz]

For the record, and those who don't know us. Will and I know each other from the 'real world', this isn't a stand up shouting match ;)

[jimsynz]

I knew that, but just wanted to say something before anyone who didn't know that jumped in.

[willbryant]

Trying to figure out what options we have to fix this permanently.

I'm wondering if we could get away with always converting integer and float bind values to strings for mysql.

That works in a surprising number of places because most operations can only apply to one type or another (2 + '2' = 4, for example, not '22'). It doesn't give the same behavior in other places like COALESCE typing and so on, so I'm trying to figure out how many cases like that there are to assess the risk. It would be a problem to change that, but at least it would be "safe by default" and you could use SQL literals to bypass the stringifying where you had to, right?

I don't like it... but that's mysql eh. At least then we'd be safe by default.

Thoughts? I would love a better option.

If the problem is specific to JSON & XML params then I would like to consider tainting them and stringifying tainted values, and not touching other values. But would need some careful thought about that too.

[ncri]

@tenderlove: Updating from rails 3.2.11 to 3.2.12 also causes an issue like the one you mention. I can't migrate my database anymore, it seems to have to do with double quoting just like in your test case:

PG::Error: ERROR: relation "users" does not exist
LINE 5: WHERE a.attrelid = '"users"'::regclass

[willbryant]

I'm not sure if @tenderlove and @ncri have the same problem. @tenderlove I think yours is happening because the quoting is applied in the predicate builder and the results of that go into where_values, and that's what gets used to instantiate the new object, if I'm reading the code correctly.

    def where_values_hash
      equalities = with_default_scope.where_values.grep(Arel::Nodes::Equality).find_all { |node|
        node.left.relation.name == table_name
      }

      Hash[equalities.map { |where| [where.left.name, where.right] }]
    end

    def scope_for_create
      @scope_for_create ||= where_values_hash.merge(create_with_value)
    end

Therefore:

    scope = Minivan.where(:minivan_id => 1234)
    puts scope.where_values.inspect
    mv = scope.first_or_initialize

[#<Arel::Nodes::Equality:0x10fabd140 @left=#<struct Arel::Attributes::Attribute relation=#<Arel::Table:0x10faeeab0 @table_alias=nil, @Aliases=[], @name="minivans", @columns=nil, @primary_key=nil, @engine=Minivan(minivan_id: string, name: string, speedometer_id: string, color: string)>, name=:minivan_id>, @right="'1234'">]

/Users/will/youdo/rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:44:in quote' /Users/will/youdo/rails/activerecord/lib/active_record/relation/predicate_builder.rb:57:inbuild_from_hash'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/query_methods.rb:327:in map' /Users/will/youdo/rails/activerecord/lib/active_record/relation/predicate_builder.rb:4:ineach'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/predicate_builder.rb:4:in map' /Users/will/youdo/rails/activerecord/lib/active_record/relation/predicate_builder.rb:4:inbuild_from_hash'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/query_methods.rb:327:in build_where' /Users/will/youdo/rails/activerecord/lib/active_record/relation/query_methods.rb:136:inwhere'
/Users/will/youdo/rails/activerecord/lib/active_record/querying.rb:9:in __send__' /Users/will/youdo/rails/activerecord/lib/active_record/querying.rb:9:inwhere'
test/cases/quoting_test.rb:20:in `test_string_ids'

I guess this happens to work most of the time with ActiveRecord because generally the strings will get cast back to the appropriate type for the column.

@ncri's bit of SQL comes from WHERE a.attrelid = '#{quote_table_name(table_name)}'::regclass and quote_table_name hasn't changed, and I'm struggling to see how it could hit the predicate stuff as it goes straight to quote_column_name and PGconn.quote_ident.

@ncri, just to help eliminate some possibilities, were there any other gems (other than the v3.2.12 rails ones themselves) that changed in your Gemfile.lock when you upgraded?

[willbryant]

Erm, I'll try the last two pastes again.

[#<Arel::Nodes::Equality:0x10fabd140 @left=#<struct Arel::Attributes::Attribute relation=#<Arel::Table:0x10faeeab0 @table_alias=nil, @aliases=[], @name="minivans", @columns=nil, @primary_key=nil, @engine=Minivan(minivan_id: string, name: string, speedometer_id: string, color: string)>, name=:minivan_id>, @right="'1234'">]

/Users/will/youdo/rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:44:in `quote'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/predicate_builder.rb:57:in `build_from_hash'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/query_methods.rb:327:in `map'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/predicate_builder.rb:4:in `each'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/predicate_builder.rb:4:in `map'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/predicate_builder.rb:4:in `build_from_hash'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/query_methods.rb:327:in `build_where'
/Users/will/youdo/rails/activerecord/lib/active_record/relation/query_methods.rb:136:in `where'
/Users/will/youdo/rails/activerecord/lib/active_record/querying.rb:9:in `__send__'
/Users/will/youdo/rails/activerecord/lib/active_record/querying.rb:9:in `where'
test/cases/quoting_test.rb:20:in `test_string_ids'

[willbryant]

Also, could someone explain to me the bit about only needing to handle Integer? I'm a bit nervous about it.

          when Integer, ActiveSupport::Duration
            # Arel treats integers as literals, but they should be quoted when compared with strings
            column = engine.connection_pool.columns_hash[table.name][attribute.name.to_s]
            attribute.eq(Arel::Nodes::SqlLiteral.new(engine.connection.quote(value, column)))

But in mysql, floats compare to strings the same way as integers:

mysql> select 0 = 'test';
+------------+
| 0 = 'test' |
+------------+
|          1 |
+------------+
1 row in set, 1 warning (0.00 sec)
mysql> select 0.0 = 'test';
+--------------+
| 0.0 = 'test' |
+--------------+
|            1 |
+--------------+
1 row in set, 1 warning (0.01 sec)

I can't see what in Arel treats integers differently to floats?

[dylanahsmith]

Also, could someone explain to me the bit about only needing to handle Integer? I'm a bit nervous about it.
when Integer, ActiveSupport::Duration

From my earlier comment:

In the predicate builder it is because Fixnum and Bignum are handled as literals by Arel (https://github.com/rails/arel/blob/master/lib/arel/visitors/to_sql.rb#L557).

[willbryant]

Ta.

[dylanahsmith]

I think the correct fix will be to

• revert the changes to the predicate builder
• in Arel, fix the last_column value passed into the quote method by clearing it appropriately
• in Arel, quote integers rather than treating them as literals

[ncri]

@willbryant: Nope, I didn't change any other gems. Reverting to 3.2.11 fixes this for me.

[ncri]

My issue problem have to do with this issue: #9260

[willbryant]

Agree with @dylanahsmith re best fix. It'd be a bit of a rewrite to fix the attribute initialization stuff any other way.

Just to confirm, this will only lead to integers being quoted on mysql?

[dylanahsmith]

@willbryant According to the announcement of this exploit "MySQL, SQLServer and some configurations of DB2" are the affected databases. So no, it isn't just mysql.

This will quote integers compared to string columns regardless of the database. That way the behaviour is consistent and predictable, except that it won't be done when the column type being compared to is unknown.