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.
2 Comments
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:
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).flattenI hope you find this comment useful!
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