Rails Brakeman Warning of SQL Injection

Rails brakeman warning of sql injection

After some kind of research here is what I would use.
There is a method called sanitize_sql_array (ref), you can use it to escape statements by passing an sql string and replacement values to it like:

sanitize_sql_array(['user_id = :user_id', user_id: 5])
# => "user_id = 5"

If we'd pass a table name to this method it will also escape it, but will apply a quote method of ActiveRecord::Base.connection object on a value, which is used to escape variables, but not table names. Maybe sometimes it will work, but it failed for me when I was using PostrgreSQL, because quote method uses single quotes, but PostgreSQL requires double-quotation for table names.

sanitize_sql_array([
  'INNER JOIN :table_name ON :table_name.user_id = :user_id',
  { table_name: 'users', user_id: 5 }
])
# => "INNER JOIN 'users' ON 'users'.user_id = 5"

connection object also has a method quote_table_name, which could be separately applied on table names, to make sure that they are escaped + use sanitize_sql_array for user id.

scope :assigned_to_user, -> (user) {
  task_table = connection.quote_table_name(UserTask.table_name)
  current_table = connection.quote_table_name(table_name)
  sanitized_sql = sanitize_sql_array([
    "INNER JOIN #{task_table}
    ON  #{task_table}.user_id = :user_id
    AND (#{task_table}.type_id = #{current_table}.type_id)
    AND (#{task_table}.manager_id = #{current_table}.manager_id)",
    { user_id: user.id }
  ])
  joins(sanitized_sql)
}

Or you could actually just use sanitize on user.id instead of wrapping everything in sanitize_sql_array method call (#{sanitize(user.id)}).

By the way, Brakeman won't already show any warnings, because query has been moved to a variable. Brakeman literally parses your code as is and it does not know about a variable and it's content. So all this thing is just to make yourself sure that everything is being escaped.

Just to shut up Brakeman you could just move a query to a variable:

scope :assigned_to_user, -> (user) {
  task_table = UserTask.table_name
  query = "INNER JOIN #{task_table}
          ON  #{task_table}.user_id = #{user.id}
          AND (#{task_table}.type_id = #{table_name}.type_id)
          AND (#{task_table}.manager_id = #{table_name}.manager_id)"
  joins(query)
}

How to fix 'Possible SQL injection' in raw SQL when scanning with Brakeman

The problem is the interpolation you're doing to create the statement.

".. (1, #{Student.get_level_name(1)});"

Despite Brakeman doesn't know where that value is coming from if you pass any value there, you're vulnerable to SQLi.

That's why you should be using ActiveRecord to handle database inserts. It allows you to pass the values for the records and it deals with the bindings and sanitization:

INSERT INTO "students" ("student_id", "created_at", "updated_at")
VALUES ($1, $2, $3)
RETURNING "id"
[["student_id", "1"], ["created_at", "2019-09-27 07:06:57.198752"], ["updated_at", "2019-09-27 07:06:57.198752"]]

There you can see ($1, $2, $3) correspondingly as the "student_id", "created_at", "updated_at" values, which aren't passed in RAW form to your query (timestamps are generated automatically if you added them).

So, for the insert:

Student.create(student_id: 1, level: Student.get_level_name(1))

Rails Brakeman SQL injection warning while accessing an oracle view/function

Yes, it is real. Almost every time, you build any SQL query from simply concatenating variables, you are vulnerable to SQL injection. Generally, an SQL injection happens each time when data inserted into the query can look like valid SQL and can result in additional queries executed.

The only solution is to manually enforce appropriate escaping or to use prepared statements, with the latter being the preferred solution.

With ActiveRecord / Rails, you can use exec_query with binds directly

sql = 'SELECT * FROM TABLE(FN_REQ(?,?))'
connection.exec_query(sql, 'my query', [demo_type_param, demo_tid_param])

Here, Rails will prepare the statement on the database and add the parameters to it on execution, ensuring that everything is correctly escaped and save from SQL injection.

rails brakeman order sql injection

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.

Rails Brakeman SQL injection warning when using Arel syntax

It's a false positive.

In this situation, Brakeman knows Relationship is a model, and that select and where are query methods. So it assumes Relationship.select(...).where(...).to_sql is a record attribute (and potentially dangerous). It shouldn't, though, since to_sql just generates the SQL code for the query as you mentioned. I'll fix this.

The second version of course does not warn because you are interpolating a string literal.

---

Related Topics