13

I am in the process of upgrading my Rails app to 5.2.3

I am using the following code in my app.

MyModel.order('LOWER(name) ASC')

It raises the following deprecation warning:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "LOWER(name)". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql()

I have changed the above as the deprecation warning recommends and the warning gone away:

MyModel.order(Arel.sql('LOWER(name) ASC'))

I have surfed about related discussion here. It seems this change is introduced to disallow the SQL injections.

But the order clause LOWER(name) ASC doesn't contains any user input. Why this ordering is considered as insecure? Is this the intended behavior or Am I missing anything here?

user11350468
  • 1,357
  • 1
  • 6
  • 22
  • 3
    `MyModel.order` (which is raising the deprecation warning) only knows that you're passing it a string, it doesn't know where the string came from or what it contains. – mu is too short Jun 22 '19 at 18:40
  • 3
    @muistooshort But it doesn't raise the deprecation warning for `MyModel.order('name')` – user11350468 Jun 23 '19 at 05:14
  • True, well spotted. – mu is too short Jun 23 '19 at 18:24
  • I found the PR had whitelisted expressions, and this helped me understand much better why some arguments triggered this, while others did not: https://github.com/rails/rails/pull/27947/files#diff-05a863a92808f1e882d6b0d3b00fe1000ccf26bb7f9c4a2ba3c6151d3718abe8R170-R180. For example: `order('count(id)')` is ok, but `order('count(*)')` is not. It's because the simple function call `count()` doesn't trigger. But `*` does. – Mr. Tim Aug 19 '21 at 21:31

1 Answers1

15

This is intended behavior, you linked to right discussion and that is exactly what it is. I can re-elaborate it a bit more though so it is easy to understand.

First, re-explaining sql injection, just for reference, doing this:

MyModel.order('LOWER(name) ASC')

Means people can pass any arbitrary string in the order function, this string might contain column names and/or order type input from user.

Now lets say, there is a dropdown in your web app, where user selects column and another one where user selects desc or asc and it submits. the data.

On the controller action what one might be doing is:

order_sql = "#{params[:column_name]} #{params[:column_order]}"

This is exactly where sql injection can take place, a malicious user can edit the form submission data and instead of sending asc or desc in column_order param, he can send some sql script something like: asc; delete from table_name_user_guessed_or_knows causing SQL injections, this is why rails want users to be cautious when using sql in order functions. And allow specifically the safe sql with user of Arel.

Now the second part, why name asc is allowed as input and LOWER(name) asc is not?

Deprecation warning reads:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "LOWER(name) asc". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql()

Focus on the words: non-attribute argument(s), non attribute arguments is anything which is not attribute, be it any extra sql appended at end for an SQL injection or be it some method call on the attribute, because methods calls can also be used to alter intended behavior of the SQL.

Next, you asked:

the order clause LOWER(name) ASC doesn't contains any user input

Rails simply has no way to know how a string got formed, it only knows it's a string being passed. Thats why it complains and want developers to be cautious.

This is why name asc is allowed, because it is simple attribute argument. While LOWER(name) asc is throwing warning because its not simple attribute argument, there is a method call on this argument which can potentially be used for SQL injection.
(Obviously an attacker wont probably use simple LOWER function for attacks, but rather he will use some special functions, maybe one he defined with similar injection approach in some previous or even same call himself).

Zia Ul Rehman Mughal
  • 2,119
  • 24
  • 44
  • What does the first part of this answer have to do with SQL injection? I'd understand if `MyModel.order('LOWER(name) ASC')` is actually `MyModel.order(order_sql)`. Are you suggesting that injection can happen through `name`? – 53c Aug 31 '23 at 08:14
  • 1
    @53c Rails has no way of knowing where a custom string is coming from, it doesn't know if this string is programmed by trusted programmer, or it is a user input, that is why it treats them both same. – Zia Ul Rehman Mughal Sep 01 '23 at 07:09