This project is archived and is in readonly mode.

Possible SQL injection in ActiveRecord limit

#6299

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

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

Activity

Torsten Bühl

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.

January 18th, 2011 @ 08:58 AM

Torsten Bühl
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.
January 18th, 2011 @ 09:05 AM
Jörg
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.

January 18th, 2011 @ 09:17 AM
Torsten Bühl
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.

January 18th, 2011 @ 10:10 AM
Kevin Bullock
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.
February 11th, 2011 @ 03:17 AM
Kevin Bullock
...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.

February 11th, 2011 @ 03:20 AM
bingbing
omega watch

March 26th, 2011 @ 01:40 AM