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.
I think this is related to an issue he opened on Rails[1] which would suggest that GitHub isn't protecting against malicious mass assignment.
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.
GitHub uses Rails. Depending on who you ask, either Rails is at fault for having unsafe defaults or GitHub is at fault for not reading the documentation. It just so happens that he demonstrated his point on the Rails repo because he thinks it's a failing of Rails, but it could have been any repo.
It can all be tracked back to the well-known "Mass assignment/attr_accessible vulnerability" in Rails. Demonstrating that even Github - full of very smart engineers - is vulnerable is just an attempt to raise (even more) exposure to the problem.
He registered a commit using his own account - so he either got the password of a rails admin or he must've found a way to add his keys to the rails github account directly.
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.
If you look at the bug report, the core Rails Dev Team basically said that they like the defaults the way that they are. They have/had no intention of changing the defaults, and are trying to push responsibility on to the developers using Rails to use sane config settings.
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.
The Rails team essentially argued that it isn't on them to secure sites built with Rails. Rails provides tools to avoid this, and GitHub chose to not use them, so this is a vulnerability in GitHub. They should be using attr_accessible, and they're not.
This just reminds me of PHP years ago where making it 'easy to develop for' took precedence over making sure that the defaults were safe (security-wise). The issue with this line of thinking is that by making it easier, you attract more less-experienced devs to your platform. Devs that don't know how to read all of the documentation and configure settings in a safe way. So what you're essentially doing is giving the inexperienced more rope to hang themselves with. Just because not everyone will be burned by a lack of security doesn't mean that the 'benefits outweigh the downsides.'
The Rails team essentially argued that it isn't on them to secure sites built with Rails.
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 problem with this attitude is that such caveats are not mentioned where it would be most prudent to do so. Which means you have scaffolding and documentation that shows you can use mass assignment, but totally fails to mention what you also need to do if you're going to put that code into production. Instead, it's hidden in a totally separate section in the guides.
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.
He's being an asshole. On the other hand, if the defaults in Rails lead even strong Rails developers into making mistakes, perhaps it is time for Rails to develop an "opinion" about this.
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.
How else? By showing him how to securely disclose similar issues, for example.
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.
I agree the message is the most important part of this despite the immature way he exposed this GitHub security issue.
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.
> 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
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.
It could be far less nefarious than getting passwords or adding SSH keys. If, e.g. the `repo_id` attribute is not protected (and there are no further authorization checks), he could just send a POST and set the repo_id to the Rails project instead of his own repo and his commit could be pushed to master.
The issue seems to be a result of an entire hash of the parameters passed in the request being sent to the new method for models.
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.
It's common in tutorials and quickstart guides, usually with a note saying that you really should be protecting things and not using mass-assignment. It just makes the blog in 5 minutes videos cleaner.
Reminds me of all the PHP tutorials floating around that only treat security as a side note. Essentially, the default configuration with Rails seems to be that users can set the data for any database column. It's almost as bad as register_globals...
Yes, mass assignment with Model.new(params) and model.attributes=params is a best practice for professional production Rails websites. Business and security rules for field updates are coded in the model (attr_accessible/attr_protected/validates).
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.
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.
Looks like this guy got really frustrated with the Rails devs basically saying that he didn't know what he was talking about. This reminds me of all of the unsafe defaults that PHP used to have. Same justification too, "it's a config setting, so it's up to the developer/sysadmin to read the docs and set them right."
This justification seems especially odd to me since Rails did so much in the first place to popularize the idea that the default behavior should be the one most likely to be "right". Don't they (or didn't they at one time) have a mantra "convention over configuration"?
This also makes it all the more serious. As the PHP developers found out the hard way:
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..
This guy brought up the vulnerability and the maintainers didn't seem to take it seriously since he wasn't articulate enough or was not approaching them with enough respect maybe for their liking? I wish they would have kissed his ass a little to get the low-down on the vulnerability so I didn't have to worry about my company's private github repos. He deserves props for bring it up for discussion.
This has been discussed since the early days of Rails, and they have chosen to leave their defaults as such and encourage developers to implement model security as needed. Github (seemingly) did not implement model security. This is a vulnerability that is different from application to application, and if the team was following best practices, is not there.
If it's a simple mass-assignment vulnerability, the Rails team has nothing to do with it given that mass-assignment is a feature and the vulnerability is well documented:
The fact that this is even a discussion is sufficient for me to consider it a bug. It's irresponsible of the Rails team to leave this default the way it is given that it's long been a known risk.
That they like to consider it a "feature" doesn't make it any better - it just makes them look like idiots
Er.. that's because there's nothing malicious an attacker can do with the mass-assignment vulnerability in the "Hello Rails" app?
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.
