rails brakeman order sql injection

Question

How can I avoid a brakeman warning in Rails when constructing an order method from parameters?

def index
  @methods = [:name, :manager, :deadline]
  assignments = Assignment.order(sort_column(@methods) + " " + sort_direction).received(current_user).root
end

def sort_column(column_names)
  column_names.each do |column|
    return column if column == params[:sort]
  end
  return 'updated_at'
end

def sort_direction
  params[:direction] == 'asc' ? 'asc' : 'desc'
end

I'm working hard to avoid ever putting user-generated code directly into the query, but brakeman still alerts (medium confidence) that this is a SQL injection vulnerability.

Is this a false positive? If not, how do I correct the vulnerability?

If so, is there an easy way to avoid the false positive?

Looks like a false positive to me, it is probably freaking out about the string operations in the `order` call. What happens if you replace `sort_column(@methods) + " " + sort_direction` with a single method call? — mu is too short, Nov 29 '12 at 06:38
What mu said. It's a false positive and it's because you are manually building the string. Actually, even removing the `" "` will make the warning go away. — Justin, Nov 29 '12 at 17:28
Unfortunately `"#{sort_column(@methods)} #{sort_direction}"` doesn't prevent the false positive. — dsilver829, Nov 30 '12 at 16:00
Neither does creating a private method in the controller and calling the private method to construct the string. Brakeman seems to be too smart to be fooled by shifting things around, but not smart enough to explain how I can deal with this. — dsilver829, Nov 30 '12 at 16:01

score 8 · Accepted Answer · answered Dec 01 '12 at 16:54

Okay, this is too long for a comment.

From my testing, moving the string building into a method like this does make the warning go away:

def index
  @methods = [:name, :manager, :deadline]
  assignments = Assignment.order(sort_order).received(current_user).root
end

def sort_order
  sort_column(@methods) + " " + sort_direction
end

However, that's just hiding the problem. I would suggest adding something like this to the Assignment model instead:

class Assignment < ActiveRecord::Base

  def self.sorted_by(column, direction)
    direction = direction.downcase == 'asc' ? 'asc' : 'desc'
    column = sanitize_sql(column)
    order("#{column} #{direction}")
  end

end

Just keep in mind that sometimes you have to choose between keeping a tool happy and keeping your code reasonable. As for the false positive, I don't see this particular issue being resolved, since it is not simple to inspect sort_column and know it is safe.

score 3 · Answer 2 · answered Nov 29 '12 at 07:28

3

You could add a sanitize method on the order by clause

 assignments = Assignment.order(ActiveRecord::Base::sanitize(sort_column(@methods) + " " + sort_direction)).received(current_user).root

answered Nov 29 '12 at 07:28

Sebi

669
5
6

1

I thought this worked, but it turns out to simply negate the order method. Brakeman likes it and stops issuing alerts, but the sanitization appears to break the ordering. Weirdly, it doesn't error. It just stops ordering silently. – dsilver829 Nov 30 '12 at 05:22
2

Well... that's interesting. Before sanitizing the order by clause looks like `"name asc"` and after sanitizing it becomes `"'name asc'"`. The quotes being added are apparently causing the query not to work. – Sebi Nov 30 '12 at 07:16

rails brakeman order sql injection

2 Answers2

Linked