Last June we talked about a similiar vulnerability affecting ActiveRecord.
The vulnerability is in ORM resource dynamic finder methods.
Carefully crafted requests can be used to turn a dynamic finder method parameter into a query scope.
Let us suppose you have a User model with two fields: an email and a password. We also suppose that our code will take an id parameter from the outside and using it to retrieve a user from the backend.
You can retrieve a user with a given id with a call like this one:
The find_by_id method is not explicity declared by the User model, it’s dynamically created by ActiveRecord as well the methods:
If the parameter id passed to the dynamic find_by_id method, contains an Hash this is used as options to the SELECT statement instead of being part of the WHERE clause.
This very trivial ruby script shows an in memory SQLite3 database managed with ActiveRecord as ORM.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34
The old fashioned defensive programming practice
ActiveRecord dynamic finders don’t care about the input you pass them. Of course it could be up to your application to properly sanitize the input instead of passing params directly to find_by_id method.
I rather prefer this approach since your application knows exactly the meaning of that line of code.
If you want your code to be robust against SQL Injection you must sanitize your input before moving forth.
If you need to call find_by_id, the parameter must be an integer identifier, so your approach would be explicitly casting to an Integer object.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
Of course this is just an example showing a security aware approach when reading a parameter that you want to manage like an Integer before using it in your ORM of choice.
Do you think adding some security checks break your agile workout? Do you think it’s just an unnecessary step adding latency to your fast and furious website?
You may want to think it twice before answering. My point of view is that for sentive parameters read from HTTP request you have to carefully handle it before passing to backend.
Off by one
On Phusion blog a more detailed and security aware dissertion was made in order to explain that there is of course a problem in the dynamic finder methods for ActiveRecord but exploiting it it’s a very hard task. And I agree with it.
The main problem looking at the patch is that in my opinion, it solves this problem but it doesn’t make a solid framework for input validation before building the query.
Adding your web application code to some positive filtering for the parameters you read helps you in:
- be transparent about the security model of your underlying framework
- be confident that input will fit what you expect from the user with ad hoc controls
The overhead in execution time can be, of course, an issue for impatient developers but the tradeoff between being compromised and having a couple of if statements to be executed can be a well tolerated solution.
What do you think about it? Is defensive programming a solution or just a redundant piece of code? Tell me your thoughts.