This project is archived and is in readonly mode.

#6299 new
Jörg

Possible SQL injection in ActiveRecord limit

Reported by Jörg | January 17th, 2011 @ 09:56 AM

When you use limit in your query it is possible to inject sql code. Unlike in Rails 2 values for limit won't be converted to integer thus allowing to inject sql.

Testcase in Rails 2.3.10

User.find(:all, :limit => "0 UNION ALL SELECT * FROM users WHERE id = 1")
=> SELECT * FROM `users` LIMIT 0

Testcase in Rails 3.0.3

User.find(:all, :limit => "0 UNION ALL SELECT * FROM users WHERE id = 1")
=> SELECT `users`.* FROM `users` LIMIT 0 UNION ALL SELECT * FROM users WHERE id = 1

User.limit("0 UNION ALL SELECT * FROM users WHERE id = 1")
=> SELECT `users`.* FROM `users` LIMIT 0 UNION ALL SELECT * FROM users WHERE id = 1

While the documentation for ActiveRecord::FinderMethods#find states that the limit parameter must be an integer there is no such hint for ActiveRecord::QueryMethods#limit.

Greetings
Henning and Jörg

Comments and changes to this ticket

  • Torsten Bühl

    Torsten Bühl January 18th, 2011 @ 08:58 AM

    What version of Arel are you using? I have the problem with Arel 2.0.7 where the limit parameter is not converted to an integer resulting in the wrong query:

    conditions_controller.rb

    @conditions = @conditions.limit(params[:limit].to_i) if params[:limit]

    Error

    ActiveRecord::StatementInvalid (Mysql::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''100'' at line 1: SELECT  conditions.* FROM conditions WHERE (conditions.destination_id = 1) ORDER BY conditions.created_at DESC LIMIT '100'):
  app/controllers/conditions_controller.rb:37:in history'
  app/controllers/conditions_controller.rb:35:inhistory'

    With Arel 2.0.6 the limit parameter is converted to an integer.

  • Torsten Bühl

    Torsten Bühl January 18th, 2011 @ 09:05 AM

    Sorry, my conditions_controller.rb looks like (I cannot edit my comment?)

    @conditions = @conditions.limit(params[:limit]) if params[:limit]

    I had to append the .to_i method to make it work in my application.

  • Jörg

    Jörg January 18th, 2011 @ 09:17 AM

    I am using arel 2.0.6.

    We had a look at 2.0.7 and found a new test that checks the escaping of limit in mysql: https://github.com/rails/arel/blob/master/test/visitors/test_mysql....

    Testing show that 2.0.7 does escape the limit parameter and only integers result in valid mysql queries.

    It looks like activerecord should at least depend on arel 2.0.7.

  • Torsten Bühl

    Torsten Bühl January 18th, 2011 @ 10:10 AM

    Ok I've not checked if the value was escaped, I think the best option would be the conversion to integer. Otherwise it will break some apps.

  • Kevin Bullock

    Kevin Bullock February 11th, 2011 @ 03:17 AM

    Converting to integer isn't the solution; the LIMIT clause allows you to specify both a limit and an offset, like so: LIMIT x, y. To protect against injection, you either have to parse the string passed to limit, or extend the syntax to allow an optional second parameter:

    User.limit(10, 20)

    and make sure to convert both to integers.

  • Kevin Bullock

    Kevin Bullock February 11th, 2011 @ 03:20 AM

    ...or allow an optional second parameter, and quote them both. That would cause ActiveRecord::StatementInvalid to be thrown when bad (non-integer) params are passed, which might be preferable.

  • bingbing

    bingbing March 26th, 2011 @ 01:40 AM

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Pages