6 ms·

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

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.

martingordon 15y ago
It's a vulnerability in Rails, which is what GitHub is built on. 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 https://github.com/rails/rails/issues/5228
- phillmv 15y ago
  Well… strictly speaking it's not a vulnerability in that you aren't inherently unsafe just because you use rails. 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.
- Scriptor 15y ago
  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.
  - cdcarter 15y ago
    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.
    - Scriptor 15y ago
      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...
  - winstonm 15y ago
    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.
runemadsen 15y ago
As Github is a Rails 2 app, I'm pretty sure it's a Rails (and therefore also Github) vulnerability.
swanson 15y ago
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.
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.
atambo 15y ago
I think it's a mass assignment vulnerability: https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228
- bobx11 15y ago
  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.
  - cdcarter 15y ago
    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.
    - vidarh 15y ago
      And he's just demonstrated why this is not a responsible approach from the Rails team.
- deleted 15y ago
  [deleted]
  - atambo 15y ago
    He did: http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.html http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.htm...
- pyre 15y ago
  Relevant: 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."
nirvdrum 15y ago
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.
tmcdonald 15y ago
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. [1]: https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228

martingordon 15y ago

It's a vulnerability in Rails, which is what GitHub is built on. 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 https://github.com/rails/rails/issues/5228

phillmv 15y ago
Well… strictly speaking it's not a vulnerability in that you aren't inherently unsafe just because you use rails. 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.
Scriptor 15y ago
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.
- cdcarter 15y ago
  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.
  - Scriptor 15y ago
    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...
- winstonm 15y ago
  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.

runemadsen 15y ago

As Github is a Rails 2 app, I'm pretty sure it's a Rails (and therefore also Github) vulnerability.

swanson 15y ago

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.

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.

atambo 15y ago

I think it's a mass assignment vulnerability: https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228

bobx11 15y ago
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.
- cdcarter 15y ago
  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.
  - vidarh 15y ago
    And he's just demonstrated why this is not a responsible approach from the Rails team.
deleted 15y ago
[deleted]
- atambo 15y ago
  He did: http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.html http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.htm...
pyre 15y ago
Relevant: 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."

nirvdrum 15y ago

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.

tmcdonald 15y ago

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. [1]: https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228