Re:GitHub Enterprise SQL Injection
by qz_ on Saturday January 07, 2017 @06:09PM
(#13347224)
Props to OP for finding the vulnerability without knowing Ruby on Rails.
[ Parent
| on HN ]
Interesting that this is a standard use-case for ActiveRecord [0] and does not include any built-in protection against SQL injection. You can pass a hash of symbols into the method, but there is no safe way to pass string arguments with sanitizing them yourself.
[0] http://guides.rubyonrails.org/active_record_querying.html#or...
God, IMO this should be reported as a bug in rails; in my opinion it ActiveRecord should never accept strings for this without parsing them.
This is probably incredibly common for things like order params in many web apps and also relies on silly conversion rules in MySQL. Any time you see order=name in params you should be suspicious. Better to use numeric enums. There are similar problems with column names, this site has some great examples which apply to many ohms, not just rails:
The lesson is you should always strongly assert the type of user input (where you can, convert to known good values which is even better), and never magically convert types as rails/ MySQL does here. It's less convenient but safer.
Re:GitHub Enterprise SQL Injection
by true_religion on Sunday January 08, 2017 @06:38AM
(#13349303)
Django for example will throw an error if an ordering parameter does not correspond to a database column.
If you have to specify all the columns of a table in Rails, then ActiveRecord ought to be able to restrict the order params to only column names.
I imagine some ORMs don't have this so people can do order by computed columns like "order by assets - liabilities"
To your point it probably makes sense to have an order method that verifies column names and an order_raw method that takes a string so the caller understands the risk, while protecting the usual case.