Rails/ActiveRecord SQL Injection

If being cool were all it took to be immune from attacks then Rails would be the most secure system in the world. Sadly, it's not coolness that counts and Rails (and ActiveRecord) are just as susceptible as any system that allows direct submission of SQL.

Here we have a little code that connects to the database and does a reasonable ActiveRecord request to the database:

require 'rubygems'
require 'active_record'
 
ActiveRecord::Base.establish_connection(
	  :adapter  => 'mysql', :database => 'chat',
	  :username => '****', :password => '****',
	  :host     => 'localhost' )
ActiveRecord::Base.logger = Logger.new(STDOUT)

class ChatItem < ActiveRecord::Base
  set_table_name "chatitems"
end

# Good
ChatItem.find(:all).each { |ci| }
  # SQL - ChatItem Load (0.2ms)   SELECT * FROM `chatitems` 

The code opens up a connection to the database, defines a wrapper class for the table, in this case 'chatitem', and requests all of the records.

So far so good, but we haven't actually taken any user input yet.

term = 'Megan'
# Good
ChatItem.find(:all,:conditions=>["user=?",term]).each { |ci| }
  # SQL - ChatItem Load (0.1ms)   SELECT * FROM `chatitems` WHERE (user='Megan')  

This code now takes user input with the 'term' variable, which is hardcoded here but could easily have come from a request. The code is still good however because it uses a replacement operator with the '?' and allows ActiveRecord to do the right thing and escape the contents of the supplied variable.

Other equally good methods here are to use the find_by style or the where style. All of which is documented on the Rails site.

This code is a prime example of how it should be done. Now let's look at a bad example:

# Bad: open to attack
ChatItem.find(:all,:conditions=>"user='#{term}'").each { |ci| }
  # SQL - ChatItem Load (0.1ms)   SELECT * FROM `chatitems` WHERE (user='Megan')

In this case I use the Ruby string replacement operator to just hand a piece of SQL code to the ActiveRecord engine. This code is susceptible because I don't do any escaping. You can see from the result that it's ok in this case, but the 'term' variable could easily hold a SQL injection attack string.

Here it's even worse. The code just gives the whole SQL string.

# Bad: Too complex and open to attack 
ChatItem.find_by_sql("SELECT * FROM chatitems WHERE user='#{term}'").each { |ci| }
  # SQL - ChatItem Load (0.1ms)   SELECT * FROM chatitems WHERE user='Megan'

Just like the other this is also highly susceptible to attack. As is the final one that doesn't use the object model at all:

# Bad: Too complex, doesn't map to objects, and open to attack
ChatItem.connection.select_all("SELECT * FROM chatitems WHERE user='#{term}'").each { |ci| }
  # SQL - ChatItem Load (0.1ms)   SELECT * FROM chatitems WHERE user='Megan'

This code I wouldn't even categorize as ActiveRecord since it doesn't use any of the object model, it just uses ActiveRecord to execute SQL directly.

The lesson here is pretty simple and pleasant. ActiveRecord makes it easy and secure to access the database if you do things the Rails way. The more that you stray from that path the more things will get both difficult and insecure.

For security folks on Rails applications the message is: 'Use ActiveRecord the easy way'. The more SQL your developers are writing the worse off they are. The code will be slower, harder to read, and more open to attack.

You make get some carping about SQL inefficiencies but the SQL code that ActiveRecord creates is sent to the longs and you can contrast and compare to see if the code it writes is any less efficient than what your developers will write to replace it.