Hello everyone! I am MEGA EXCITED for this release, as it's the first version in which I'm a committer. Wooo! In addition, 954 other intrepid Rubyists in total contributed to this release: http://contributors.rubyonrails.org/edge/contributors
Please note that this is a beta, not an rc, so some things may be a bit wonky. Please file an issue on GitHub and I will do everything I can to help you help us iron out all the kinks.
❤❤❤
(Also, I am on a plane and my battery is almost dead, so I may drop out of this thread soon. Ill read it when I'm home.)
I'm curious to know if these examples of potential SQL injection tested against 3.2.11 have been dealt with in 4.0? Would be great to see these possible problems tidied up.
OK. I've checked against 3.2.12, and at least one isn't fixed, so it might be worth passing the link on to the security team. They'll all be easy fixes, and it'd be great to see them fixed (if not already) in 4.0.
Thanks for contributing Rails 4.0 - I'm excited to try it out, in particular the new caching looks great.
The examples here do not include SQL injection from known CVEs and are not vulnerabilites themselves, only potential misuses of the methods.
What would you like to have changed here? The methods work fine as long as you do not explicitly embed a user-supplied string in a larger SQL fragment.
I don't think all of these examples involve embedding strings. Many people assume that they can use any ActiveRecord method, and as long as they don't embed strings, they'll be safe from sqli. Take this first example:
Order.calculate(:sum, params[:column])
I've just checked in Rails 3.2.12 and this is still possible, haven't checked against Rails 4.0. If I pass in: http://localhost:3000/adverts/1?column=id) FROM users WHERE name = 'Bob' SUM(id;
And do a simple sum in the controller:
sum = Advert.calculate(:sum, params[:column])
# or
sum = Advert.sum(params[:column])
I get:
SELECT SUM(id) FROM users WHERE name = 'Bob' SUM(id) FROM "adverts"
Which is not good as it is injecting sql - with judicious use of sql comments etc it could probably be made to execute any sql statement (apparently it does on sqlite with this particular test, but it fails in postgresql which I tested with). Other queries work with psql though [deleted example and reported]. I've reported this with a working example of sqli just in case it is an unreported problem.
Now the intended use of sum, count etc is to deal with symbols chosen by the programmer, but a naive implementation of say an admin form for summing some record attributes might use a param from a form, and pass that in to the sum method for column name, just as they might use params[:id] with find. I think Rails should deal with that.
Also the example given of User.exists? params[:id] is probably commonly used, but vulnerable to manipulation of results at the very least.
I'm not the original author of this page (which is already public and probably has been for some time), and am aware this is not an appropriate place to report bugs, but it would be reassuring if the Rails team looked at this link and fixed anything which is dangerous like the usage above.
IMHO all rails ActiveRecord methods should guard against sqli as much as possible, not just the commonly used ones, and if they don't currently it needs to be fixed.
Ok, this one is a genuine problem in that the method is expected to only deal with column names and is not supposed to accept full SQL, but you made it sound like the page was a list of security issues that Rails should deal with but isn't, which on top of the recent discussions about Rails security sounds a lot like FUD (maybe unintentionally); this is actually a page listing Rails anti-patterns.
If you actually learn Rails and aren't just piecing things together until it works, it isn't a secret that most methods accept raw SQL as the first argument, with optional placeholders for expected values, so if you want escaping you either pass an array ["col1 = ?", params[:val1]] and the values will be quoted or you have to quote explicitly. Methods like .where(), .joins(), .having(), .order() etc. would be crippled if they escaped this first string argument so I am not sure how a "fix" should like here, you have to demarcate somehow which input is trusted and which is untrusted and those methods are 95% of the time used to deal with trusted input.
If you actually learn Rails and aren't just piecing things together until it works
This kind of defensive attitude isn't helpful, I'm quite familiar with Rails since 1.x thanks and want to see it improve, particularly on security issues. I use it every day. I've only quickly looked at this page but as it contains many common antipatterns, and some straight up vulnerabilities, I think it'd be worth fixing everything on it if possible. Having read the above list I'm starting to think I'd be best to convert all params explicitly to the expected type before use, as Rails accepts many varying types for params and this has increased the attack surface. I'm actually pleased there is a spotlight on Rails security right now though as things will improve - much better than no-one but blackhats being interested.
I highlighted two vulns from the list, one in Model.exists? which is expected to take an id param straight from user input, and one in Model.sum,count etc which is more serious but less likely to be encountered as it requires using a column name straight from user input (not advisable). Neither of them take sql as the first argument, one is intended to be an id, and the second a column name. While the above example is a misuse of sum in my opinion, I think Rails should still be a good citizen and deal with it, given the common pattern of Model.query(params[:xxx]) which can be very useful and which many people will pattern their other usage on - any method which takes params directly and not raw sql (i.e. not the ones you list) should I feel be protected against sqli, even or especially if it is not a commonly used one.
Taking security seriously (even if it means inconvenience or working around common user errors) is not FUD, it's a worthwhile process and one which Rails should continue with.
I just think that with your first brief comment you unintentionally scared a lot of people away from Rails, possibly indicating a new release was done but some serious security issues were simply ignored. I had trouble understanding what you meant until you supplied those additional explanations, many thanks for which. I agree the examples you pointed out are problematic, I am just happy we made clear that this is just about 2 or 3 examples from the long list in the link you posted. There is certainly room for improvement in Rails security-wise and I am also happy this is being discussed, as long as the discussion is balanced.
I suppose it's a lot less of an issue now anyways, but in 3.2, using fetch with memcache with raw objects is super gross since you get totally incompatible responses depending on whether an item existed in cache or not.
I personally read every single issue, comment, and commit that goes into Rails. But often tickets come up that I don't know anything about, so I don't say anything. This suggestion is one of them; I'm not mega familiar with that part of the codebase.
In my opinion the easy way is to look through the github issues and pull requests and understand what's going on. From there, try reading docs and seeing what does / doesn't make sense and editing it. You pretty much always get feedback if you send a decently put together pull request.
Look for places the code might not currently be clear, and try to improve clarity without changing behavior.
Once you've done that kind of thing, sky's the limit as far as I am concerned. I'm working on understanding and improving the parameter parsing behavior at the moment.
Is there any sort of prioritized TODO list, either by difficultly or importance? And that's not so I can knock out some big feature and be the hero, but so I can whittle away at some of the more nagging edge cases and make some random developers day when things just work.
Even just making a little Rails app or script that re-produces the bug would be helpful. Writing a test case that's failing would be mega helpful. Fixing the bug would be incredibly helpful. :)
The issues page is the best you have for that, sort by popularity or something like that.
That's one of the only things I'm frustrated about - I wish there were a clearer list so that I didn't have to troll for tasks that need doing, or ask a core member on IRC/campfire
Definitely check out Rails Dev Box(https://github.com/rails/rails-dev-box) too. It helps to create a virtual environment for testing and running the Rails test suite.
Once you have that installed, you basically don't have to worry about dependencies that you need to bootstrap the Rails test suite.
Please note that this is a beta, not an rc, so some things may be a bit wonky. Please file an issue on GitHub and I will do everything I can to help you help us iron out all the kinks.
❤❤❤
(Also, I am on a plane and my battery is almost dead, so I may drop out of this thread soon. Ill read it when I'm home.)