ActiveRecord find and MySql ‘IN’ gotcha

2 Comments

1. Peter Bex
   Posted August 23, 2009 at 12:27 | Permalink

   Hi Jeroen,

   Good to see you’re back in the blogging business

   Unfortunately, this post is giving bad advice.

   Tasks.find(:all, :conditions => ["categorie_id IN (#{@categories.join(',')})"])

   is unsafe unless you are absolutely sure that categories are all
   integers. If they are strings (and that includes “integers”
   passed from HTML forms, which are actually strings and can be
   manipulated by attackers) you are vulnerable to SQL injection. That’s why you use the question mark syntax in the first place. Otherwise, you wouldn’t need that.

   A simple example:

   
   # The hash here is to make the rest of the statement a comment
   @categories = ['1)) UNION SELECT * FROM users #']
   Tasks.find(:all, :conditions => ["categorie_id IN (#{@categories.join(',')})"])
   
   

   Of course, this will give an error if the number of columns do not match but it is not hard to pad in extra columns. There are also ways to make it match if users has more columns; you would start by reading out id and name, or login, and password and pad those where needed.

   There are a couple of proper ways to find tasks by category, even if the categories are uncontrolled strings:

   Tasks.find(:all, :conditions => ["categorie_id IN (?)", @categories])
   In this case, Rails will join with a comma and escape accordingly.

   A simpler syntax is:

   Tasks.find(:all, :conditions => {:categorie_id => @categories})

   You could also first fetch the categories and use the ‘task’ method on those objects:

   Categorie.find(@categories).map(&:tasks).flatten

   I hope you find this comment useful!

2. Jeroen van Doorn
   Posted August 24, 2009 at 08:23 | Permalink

   Hi Peter!

   Long time no speak!

   Very useful indeed! Will have a look at my code to see why I did this in the first place … But I stand corrected

   Hopefully I’m able to post some more stuff in the near future

   Speak soon!
   Jeroen