SQL injection with :order, :limit, :group

I know how to avoid SQL injection attacks when you use :conditions
User.find :first, :conditions => ["login=?", params[:username]]

but how about with :order, :limit or :group?

# uh-oh...spaghetti-oh
User.find :first, :order => "login; delete from users; select * from users"

Pat

I assume you're permitting, for example, user-specified ordering? Unless you're permitting users to modify the DDL, I think your best bet is to interpret their request though some parameter and build the query rather than interpolate their input directly.

You might also look to see if the ActiveRecord adapter for the database you're using has an equivalent to PostgreSQL's quote_ident function. I don't know if the PostgreSQL adapter exposes that functionality directly though.

Michael Glaesemann
grzm seespotcode net

I have run in to this a few times as well. If you want to simply allow
them to sort on the columns in a table, ascending / descending, then
this code makes it safe:

sort_col = User.column_names.include? params[:sort_col] ?
params[:sort_col] : 'created_at' #or whatever default you want
sort_dir = params[:sort_dir].downcase == 'asc' ? 'asc' : 'desc'

User.find :first, :order => "#{sort_col} #{sort_dir}"

You can easily create a library function to use this elsewhere. Hope
this helps.

-Bill

Pat Maddox wrote:

Since you probably have a view that allows only specific ordering
columns and directions, I would suggest something like:

  allowed_directions = [ "asc", "desc" ]
  allowed_columns = [ "posting_date", "sender", "recipient" ]

  sort_column = "default-column-name"
  if allowed_columns.include?(params[:sortcolumn])
    sort_column = params[:sortcolumn]
  end
  sort_direction = "asc"
  if allowed_directions.include?(params[:sortdirection])
    sort_direction = params[:sortcolumn]
  end

and then use the sort column itself. This sanitizes user input by
following the rule of allowing specific actions and disallowing all
others.

I _think_ you can also do:

  :order => [ ":column :direction", { :column => params[:sortcolumn],
:direction => params[:sortdirection] }]

but I'm not certain. I like my way better -- it's less trusting of magic.

--Michael

Yeah, my initial concern was that there are a lot of sort / group
variations and they will change over time. I think what I'll end up
doing is writing a little helper object to offload that. So then I
have something like

MyModel.find :all, SanitizedOptions.new(:order => params[:order],
:limit => params[:limit], :group => params[:group])

Implementation will be simple - just check to see if the passed in
option is in a particular array, like you said. Then the finder
doesn't have to deal with that stuff, I express that this is a
potentially dangerous method, and it's obvious where changes to the
allowed options should go.

Thanks for the input everybody.

Pat

You might find this of use:

User.column_names

=> ["id", "login", "email", "crypted_password", "salt",
"password_reset_code", "activation_code", "activated_at",
"site_admin", "created_at", "updated_at", "lock_version", "time_zone"]

This should let you make a generic function as a module, and then just
mix it into each of your models. I think. :slight_smile:

--Michael

Take a look at my post above, that is exactly what I am doing :slight_smile:

Michael Graff wrote: