I'm confused. Is this a generic Github vulnerability or is this a vulnerability in tools outside of Github used by Rails? The 'hacker' seems to suggest it's the former ("Github pwned"), which would be pretty serious stuff.
By default, if you have an new, create or update_attributes (and more, I imagine) call which changes various attributes based on a hash from parameters (eg params[:post], where you have params[:post][:title], params[:post][:body] etc) Rails allows mass assignment of every attribute on that model, since attr_accessible is not called.
There is a method you can call in the model called attr_accessible that restricts the columns that can be updated through mass assignment, while still allowing for manual assignment of other columns.
An example of this might be a post's user_id, which you would usually want to set to the current user while not allowing mass assignment. Without specifying attr_accessible it would mean that if a malicious user added params[:post][:user_id] to their POST/PUT, the Rails application would update the user_id as per the params value. If attr_accessible had been called, defining the columns that the developer wanted to be mass assigned (say post and title), it would mean that the user_id would not be mass assigned and Rails would log that this was the case.
attr_accessible therefore acts as a whitelist for columns that can be mass assigned. It just so happens that the Rails default is to have no whitelist and allow all columns to be mass assigned, despite the fact that the sensible option is to always have a call to attr_accessible in your models.
The comments on the commit mention he just raised an issue that few people protect the attributes on their models from mass assignment, which… is one way this could happen.
Kind of a dick move, though. Responsible disclosure, doing it on a Sunday morning, etc, etc.
> Responsible disclosure
Looks like the guy did report it and the response was: "Not our problem" / "Not an issue." He got frustrated and decided to make a very public example of how this is bad.
I'm getting a good laugh seeing this card played to defend terrible practice as the norm for what sells itself as an opinionated framework that champions convention over configuration.
The approach this encourages is to code insecure (because you don't know any better), without any awareness of it being insecure, then remember to go back and secure it when you've found out what all the vulnerabilities are. Inevitably, you will waste time, and probably miss things out.
Hence a plea to the framework to make this a default.
I hope the asshole messenger doesn't obscure the importance of the message because people often push back harder against a message when it is delivered in an obnoxious manner by an obnoxious person.
Having the whole internet bashing him isn't good for anyone.
I'm for one very glad he kept pushing it out (but didn't do any real harm). This kind of vulnerability is just so common in Rails apps: I'd like to see safer defaults.
Rails can certainly adopt an 'opinion' regarding this issue, but if I think if we were to take a look around at heavy web frameworks today, we would see a very similar approach of "let the developer decide" when dealing with Model security and serialization of fields.
These framework devs have no idea how people are going to use their models, so forcing them to whitelist everything by default may cause unnecessary headaches. Instead, they provide tools to prevent this exploit from happening should devs expose the model.
What everyone else is doing isn't a great justification--if decisions in Rails were based on what everyone else was doing, it would have been written in Java. In terms of Rails opinions, sensible defaults would be one that would suggest this should be rethought. In terms of rewrite-work, the Rails team didn't shy away from that with Rails 3, but I think the cross site scripting protection was worth the work. And even if the default is changed, nothing stops it from being a single line of code to turn it off.
The vulnerability was demonstrated by adding a commit to the Rails project on GitHub, indicating that GitHub suffers from the vulnerability.
Here's the relevant issue. It might clarify things a bit better: https://github.com/rails/rails/issues/5228
Is this really common practice in Rails code? Sure you can specify in the model that certain attributes can't be changed. But shouldn't this stuff be checked when validating form input? Normally I'd have a hash of filters, with the field/column name mapped to the appropriate set of rules for that field. Anything that's not specified in the filters doesn't go to the model's new method.
In this case, it's like the equivalent of PHP code where you santizie the data in $_POST and just send that whole variable to the database.
Choosing which fields are accepted shouldn't just be a model security issue, it's a form validation issue. This makes choosing whether an admin can change a field or not trivial as well. If a request is made as an admin, (maybe through a form that's only accessible by someone with an admin role) then you just apply the validation rules for an admin. Otherwise you apply the rules for a user.
That's how it's been since Rails 1. Which is cool. But it's error-prone for newbies, especially when Rails's model and controller generators make all attrs writeable by default, with nary a generated comment about how or why to lock things down. In a culture of convention over configuration, attrs should be locked down by default: "config.active_record.whitelist_attributes=true" for new apps, and throw a helpful message when I mass assign to a model that has no accessible attrs configured yet.
It is a bug in that it's a usability issue; maybe it should be turned on by default, much like the auto escaping to prevent XSS that came in rails 3.
What I want you to see in that thread I mentioned is the
way the core team perceives this. You are not discovering
anything unknown, we already know this stuff and we like
attr protection to work the way it is.
I guess that doesn't apply to security.
When you make it really easy to get started, a lot of people won't learn the system in depth enough to understand all the issues because they don't need to in order to make it work "well enough" for most cases.
By making Rails so easy to get started, they pretty much guarantee that there's going to be a ton of developers that don't pick up on, or forget, that they need to deal with issues like this.
That even a site like Github was vulnerable to this demonstrates just how seriously wrong it is to pick a default like this..
http://edgeguides.rubyonrails.org/security.html#mass-assignm...
Heck, I even learned this way back when I was learning Rails:
http://railscasts.com/episodes/26-hackers-love-mass-assignme...
That they like to consider it a "feature" doesn't make it any better - it just makes them look like idiots
The Security guide does describe the issue, and even describes attr_accessible as a "Countermeasure". http://guides.rubyonrails.org/security.html#mass-assignment
Without any precautions Model.new(params[:model]) allows attackers to set any database column’s value.
Unbelieveable! PHP showed long ago that allowing a web request to auto-populate arbitrary members in app objects is just a spectacularly bad idea.
Even the canonical "Hello Rails" example code neglects to specify attr_accessible. http://guides.rubyonrails.org/getting_started.html#hello-rai...
The general attitude of Rails developers towards security here is really shocking. I don't think I could recommend anyone use Rails.
Being able to change the :id or timestamps of the post isn't anywhere near the SQL injection vulnerabilities I've seen in many tutorials in other languages/frameworks.
I agree, though, I wouldn't recommend Rails to people who can't bother to read documentation.
Deleted comment