Rails is this query open to sql injection?

Question

I'm still learning how to write good queries using ActiveRecord. I'm curious if this query is subject to sql injection because of the way i'm using the date field in the query.

Can someone please point out any obvious mistakes or any better ways to write this query?

@arrangements_for_month =
  Arrangement.joins(:timeslot).
              where("timeslots.timeslot BETWEEN '#{month}' AND '#{month.end_of_month}'", params[:id]).
              order('location_id')

Just a general note, I've found a few cases where doing params[:id].to_i has saved us from security vulnerabilities in rails. So even rails claims to escape the parameters in a given method, trust nobody :) — Abe Petrillo
– Abe Petrillo, Commented Mar 14, 2013 at 1:19
Can you elaborate on how params[:id].to_i has prevented security vulnerabilities? — Catfish
– Catfish, Commented Mar 14, 2013 at 2:02
"1".to_i returns the integer 1. "some_string".to_i returns 0 — Abe Petrillo
– Abe Petrillo, Commented Mar 14, 2013 at 2:33

Jorge Israel Peña · Accepted Answer · 2013-03-14 01:25:23Z

8

You should just use the preferred way of including parameters to be safe. Check out this guide:

Building your own conditions as pure strings can leave you vulnerable to SQL injection exploits. For example, Client.where("first_name LIKE '%#{params[:first_name]}%'") is not safe. See the next section for the preferred way to handle conditions using an array.

Try:

@arrangements_for_month = Arrangement.joins(:timeslot)
  .where("timeslots.timeslot BETWEEN ? AND ?", month, month.end_of_month)
  .order('location_id')

And just a heads up, if you like, there is an alternative way to define a range condition like that using ruby ranges, as described in that section of the linked guide:

Client.where(:created_at => (Time.now.midnight - 1.day)..Time.now.midnight)

So, without knowing anything else about your code, you can probably do something like this:

@arrangements_for_month = Arrangement.joins(:timeslot)
  .where("timeslots.timeslot" => month .. month.end_of_month)
  .order('location_id')

edited Mar 14, 2013 at 1:25

answered Mar 14, 2013 at 1:19

Jorge Israel Peña

39k16 gold badges96 silver badges126 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Zippie Over a year ago

Why is the code that you gave right after Try: safer than his?

mkk · Accepted Answer · 2013-03-14 01:32:34Z

yes it is. Everytime you insert user's input into the query string, it is vulnerable. If month will be :

5' AND '8'; DROP TABLE timeslots;--

you might be in serious troubles. Not to mention drop database etc.

I haven't reproduced exactly this query, but something similar [ I had to add ) in my query due to using acts_as_paranoid plugin ]:

SomeModel.pluck(:id)
 => [1, 2, 4, 3, 5, 6]

abc = 'a\');delete from some_models where id=6;--'
User.where("name = '#{abc}'")
 => []

SomeModel.pluck(:id)
 => [1, 2, 4, 3, 5] # please note that record with id 6 was deleted!

The reason why the attack was possible, is that I could provide ' and -- ( which starts comment ). When yo use the suggested way, i.e. using .where("name = ?", "my_name"), then the attack would not be possible. Check this out:

abc = 'a\');delete from some_models where id=5;--'

User.where("name = ?", abc)
 => []

SomeModel.pluck(:id)
 => [1, 2, 4, 3, 5] # this time record with id 5 was not deleted

This is first query:

 User Load (1.5ms)  SELECT "users".* FROM "users" WHERE ("users"."deleted_at" IS NULL) AND (name = 'a');delete from some_models where id=6;--')

This is the second

  User Load (1.0ms)  SELECT "users".* FROM "users" WHERE ("users"."deleted_at" IS NULL) AND (name = 'a'');delete from some_models where id=5;--')

Note the additional ' in the second - query(name = 'a'')

Collectives™ on Stack Overflow

Rails is this query open to sql injection?

2 Answers 2

1 Comment

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related