5 ms·

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 acc

by phillmv 15y ago

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.

pyre 15y ago
> Responsible disclosure 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.
- phillmv 15y ago
  Rails bugtracker != notifying the people at github that they forgot to add a mass-assign protection.
  - deleted 15y ago
    [deleted]
  - ramblerman 15y ago
    you're missing the point. If the good engineers at github are getting this wrong then chances are this is a common security flaw. Hence a plea to the framework to make this a default.
- cmelbye 15y ago
  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.
  - pyre 15y ago
    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.'
  - FuzzyDunlop 15y ago
    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.
  - cbs 15y ago
    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.
ericb 15y ago
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.
- skMed 15y ago
  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.
  - ericb 15y ago
    > 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.
- thibaut_barrere 15y ago
  My thoughts exactly: the "asshole" is 18. People should give him a break already, and consider we're lucky he's getting the word out.
swanson 15y ago
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.

by phillmv 15y ago

pyre 15y ago

> Responsible disclosure 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.

phillmv 15y ago
Rails bugtracker != notifying the people at github that they forgot to add a mass-assign protection.
- deleted 15y ago
  [deleted]
- ramblerman 15y ago
  you're missing the point. If the good engineers at github are getting this wrong then chances are this is a common security flaw. Hence a plea to the framework to make this a default.
cmelbye 15y ago
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.
- pyre 15y ago
  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.'
- FuzzyDunlop 15y ago
  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.
- cbs 15y ago
  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.

ericb 15y ago

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.

skMed 15y ago
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.
- ericb 15y ago
  > 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.
thibaut_barrere 15y ago
My thoughts exactly: the "asshole" is 18. People should give him a break already, and consider we're lucky he's getting the word out.

swanson 15y ago

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.