8 ms·

Github Is Classy

97 points by craigkerstiens 15y ago

heymishy 15y ago
I really think the actual issue here has been clouded over by reactions to the way the issue was handled. Sure it wasn't the most mature and professional way of handling the situation, it was illegal and he certainly didn't need to take it to the extent he did - but he put the issue to the forefront. Github responded well although it does seem as they were trying to spin it as though they had the situation in hand more so than I believe they did - but thats just me. Long story short - github handled it well, but the real story is hugely popular (techy) news - cant ask for better community awareness than that!
- lusis 15y ago
  The problem is people are still confusing two issues. 1 - The mass assignment rails issue was resolved as soon as could be after it was reported 2 - The public key form update vuln was NOT reported and used, NOT to attack github but to make some point to the Rails team. The second issue was the one github had been talking about in the original blog post. They handled it as soon as it was discovered. In so far as they responded as quickly as possible, yes they had it in hand.
  - mofey 15y ago
    The public key form update vulnerability was based on the same concept in a different place.
kenneth_reitz 15y ago
The amount of bad press GitHub has received from this has blown me away. If anyone should be getting flamed for this, it should be the Rails maintainers that weren't open to the discussion.
- mkramlich 15y ago
  I respect your work Ken but I disagree with you on this. Rails is just a web framework and does provide developers who use it with the ability to use it securely or insecurely. In this case, the shipped default leaned more toward convenience than security. There are tradeoffs involved. GitHub provides codebase hosting to thousands of projects, some of which are private. They have a large public attack surface. And they're business which takes money. And they're probably making millions. This should imply certain things to it's management team. It's reasonable to assume they'd take security very very seriously. Perhaps not need bank-like level security, but still, pretty well up there. Any Rails app developer has had the ability to unilaterally go into their own codebase and fix/close any security vulnerabilities. This same ability does not exist for users of the GitHub webapp. (Of course, we could choose not to use GitHub, but that would be good neither for GitHub or the user.) That said, security is hard and getting it 100% right 100% of the time is probably impossible. And I love GitHub, and think they get more things right than wrong, enough of the time, that I'll give them the benefit of the doubt.
  - raganwald 15y ago
    I respect your work Ken but I disagree with you on this. I just want to give a huge nod of recognition to this extremely civil way to express disagreement. Bravo!
  - kenneth_reitz 15y ago
    Thanks for the civil disagreement :) I do agree that this is GitHub's responsibility. I think they handled the situation appropriately, however. I'm shocked at the backlash people have against GitHub locking the user's account while it was being investigated.
- nivloc 15y ago
  Github left the most widely known gap in Rails security open to exploit. Then they did a bad job handling the fallout. I'm not saying the Rails devs shouldn't have changed it five (maybe seven now?) years ago when it first came up, but output escaping wasn't default until 3.x, and everyone in the community knew both were major problems.
shasta 15y ago
I think they handled this like adults. Very impressive.
raganwald 15y ago
Perhaps I am misunderstanding the situation. Am I correct that the so-called “vulnerability” is that if you do a mass assignment from an untrusted source—form parameters—to a model without whitelisting attributes via attr_accessible, an attacker can write values to arbitrary attributes? If so, this isn’t a security vulnerability in Rails. A new Rails application isn’t vulnerable by default. It’s a security vulnerability in apps written using a certain style, or perhaps a vulnerability by default in apps written using the generators and scaffolding baked into rails. But for guys like me who never use mass assignment, our applications are not vulnerable by default, correct? And likewise, for people who RTFM before using mass assignment, their applications are not vulnerable, correct? If I understand this, it is very different from something like the routes vulnerability a while back, where EVERY Rails app was vulnerable whether you liked it or not. Saying Rails has a security vulnerability here is like saying that ActiveRecord has a SQL Injection vulnerability because by default, find_by_sql allows you to compose queries out of strings, and if you don’t choose to use the correct form, you will be attacked by little bobby tables. (EDIT) To be clear, I like what the article says, I’m just not sure that the phrase “Rails has a security vulnerability” applies in this case, or that it is technically true that Rails is insecure by default.
- jtimberman 15y ago
  My point is that Rails does not take the high road of providing better, sane, secure defaults for people to build their applications. While the proposed solutions in the issues I linked to were denied, it doesn't mean there shouldn't be some solution added to Rails. This is definitely a case of convenience favored over security.
  - raganwald 15y ago
    I think it’s like .find_by_sql and .where and everywhere else Rails provides a way to write SQL fragments. You can compose strings and open yourself to SQL Injection, or you can have ActiveRecord insert sanitized parameters for you. There’s no “vulnerability by default” in ActiveRecord’s handling of SQL snippets. There’s a right way and a wrong way. Likewise with mass assignment. There’s a right way to use it and a wrong way to use it. FWIW, I don’t use mass assignment, ever, because the “right” way offends my sensibilities by making me load up a model with authorization logic. Rails is not vulnerable by default because Rails apps do not include mass assignment by default. It’s not like there’s a controller method called ‘update’ created if you don’t write one that uses mass assignment. You have to generate or write an update method, and if you do and you include a sql query, you have to get the sql fragments right. Likewise if you use mass assignment, you have to do attr_accessible right. All that being said, I don’t like attr_accessible, I think it might have been brilliant at the time but a few years later I think we can revisit this problem with fresh eyes and a lot of experience as a community and do even better. Which by extension means that I don’t like mass assignment. SO: We probably agree that this feature should be taken out and shot, but are quibbling over which charge should be read off the indictment before giving the order to fire ;-)
JumpCrisscross 15y ago
I think Google's full disclosure policy, which "involves making full details of a vulnerability available to everybody simultaneously, giving no preferential treatment to any single party" is far classier. http://googleonlinesecurity.blogspot.com/2010/07/rebooting-responsible-disclosure-focus.html http://googleonlinesecurity.blogspot.com/2010/07/rebooting-r...
jtimberman 15y ago
I never said this is a security vulnerability in Rails. I said it is a bug (and a serious one). A bug, as you may be aware, is a mistake, fault, failure, problem or other unintended or surprising behavior in software. See updates to my post for some additional clarification. I hope that helps. Thanks for reading!
- mofey 15y ago
  "The same user exploited another vulnerability". It wasn't exactly "another vulnerability". It still had to do with the same mass attribute assignment feature just in a different place.
pwim 15y ago
The irony of this all is that github isn't using attr_accessible at all in their application, but rather their own custom solution: http://news.ycombinator.com/item?id=3664999 http://news.ycombinator.com/item?id=3664999
ColdAsIce 15y ago
Egor Homakov is classy. Damn classy.

I really think the actual issue here has been clouded over by reactions to the way the issue was handled. Sure it wasn't the most mature and professional way of handling the situation, it was illegal and he certainly didn't need to take it to the extent he did - but he put the issue to the forefront. Github responded well although it does seem as they were trying to spin it as though they had the situation in hand more so than I believe they did - but thats just me. Long story short - github handled it well, but the real story is hugely popular (techy) news - cant ask for better community awareness than that!

lusis 15y ago
The problem is people are still confusing two issues. 1 - The mass assignment rails issue was resolved as soon as could be after it was reported 2 - The public key form update vuln was NOT reported and used, NOT to attack github but to make some point to the Rails team. The second issue was the one github had been talking about in the original blog post. They handled it as soon as it was discovered. In so far as they responded as quickly as possible, yes they had it in hand.
- mofey 15y ago
  The public key form update vulnerability was based on the same concept in a different place.

kenneth_reitz 15y ago

The amount of bad press GitHub has received from this has blown me away. If anyone should be getting flamed for this, it should be the Rails maintainers that weren't open to the discussion.

mkramlich 15y ago
I respect your work Ken but I disagree with you on this. Rails is just a web framework and does provide developers who use it with the ability to use it securely or insecurely. In this case, the shipped default leaned more toward convenience than security. There are tradeoffs involved. GitHub provides codebase hosting to thousands of projects, some of which are private. They have a large public attack surface. And they're business which takes money. And they're probably making millions. This should imply certain things to it's management team. It's reasonable to assume they'd take security very very seriously. Perhaps not need bank-like level security, but still, pretty well up there. Any Rails app developer has had the ability to unilaterally go into their own codebase and fix/close any security vulnerabilities. This same ability does not exist for users of the GitHub webapp. (Of course, we could choose not to use GitHub, but that would be good neither for GitHub or the user.) That said, security is hard and getting it 100% right 100% of the time is probably impossible. And I love GitHub, and think they get more things right than wrong, enough of the time, that I'll give them the benefit of the doubt.
- raganwald 15y ago
  I respect your work Ken but I disagree with you on this. I just want to give a huge nod of recognition to this extremely civil way to express disagreement. Bravo!
- kenneth_reitz 15y ago
  Thanks for the civil disagreement :) I do agree that this is GitHub's responsibility. I think they handled the situation appropriately, however. I'm shocked at the backlash people have against GitHub locking the user's account while it was being investigated.
nivloc 15y ago
Github left the most widely known gap in Rails security open to exploit. Then they did a bad job handling the fallout. I'm not saying the Rails devs shouldn't have changed it five (maybe seven now?) years ago when it first came up, but output escaping wasn't default until 3.x, and everyone in the community knew both were major problems.

shasta 15y ago

I think they handled this like adults. Very impressive.

raganwald 15y ago

Perhaps I am misunderstanding the situation. Am I correct that the so-called “vulnerability” is that if you do a mass assignment from an untrusted source—form parameters—to a model without whitelisting attributes via attr_accessible, an attacker can write values to arbitrary attributes? If so, this isn’t a security vulnerability in Rails. A new Rails application isn’t vulnerable by default. It’s a security vulnerability in apps written using a certain style, or perhaps a vulnerability by default in apps written using the generators and scaffolding baked into rails. But for guys like me who never use mass assignment, our applications are not vulnerable by default, correct? And likewise, for people who RTFM before using mass assignment, their applications are not vulnerable, correct? If I understand this, it is very different from something like the routes vulnerability a while back, where EVERY Rails app was vulnerable whether you liked it or not. Saying Rails has a security vulnerability here is like saying that ActiveRecord has a SQL Injection vulnerability because by default, find_by_sql allows you to compose queries out of strings, and if you don’t choose to use the correct form, you will be attacked by little bobby tables. (EDIT) To be clear, I like what the article says, I’m just not sure that the phrase “Rails has a security vulnerability” applies in this case, or that it is technically true that Rails is insecure by default.

jtimberman 15y ago
My point is that Rails does not take the high road of providing better, sane, secure defaults for people to build their applications. While the proposed solutions in the issues I linked to were denied, it doesn't mean there shouldn't be some solution added to Rails. This is definitely a case of convenience favored over security.
- raganwald 15y ago
  I think it’s like .find_by_sql and .where and everywhere else Rails provides a way to write SQL fragments. You can compose strings and open yourself to SQL Injection, or you can have ActiveRecord insert sanitized parameters for you. There’s no “vulnerability by default” in ActiveRecord’s handling of SQL snippets. There’s a right way and a wrong way. Likewise with mass assignment. There’s a right way to use it and a wrong way to use it. FWIW, I don’t use mass assignment, ever, because the “right” way offends my sensibilities by making me load up a model with authorization logic. Rails is not vulnerable by default because Rails apps do not include mass assignment by default. It’s not like there’s a controller method called ‘update’ created if you don’t write one that uses mass assignment. You have to generate or write an update method, and if you do and you include a sql query, you have to get the sql fragments right. Likewise if you use mass assignment, you have to do attr_accessible right. All that being said, I don’t like attr_accessible, I think it might have been brilliant at the time but a few years later I think we can revisit this problem with fresh eyes and a lot of experience as a community and do even better. Which by extension means that I don’t like mass assignment. SO: We probably agree that this feature should be taken out and shot, but are quibbling over which charge should be read off the indictment before giving the order to fire ;-)

JumpCrisscross 15y ago

I think Google's full disclosure policy, which "involves making full details of a vulnerability available to everybody simultaneously, giving no preferential treatment to any single party" is far classier. http://googleonlinesecurity.blogspot.com/2010/07/rebooting-responsible-disclosure-focus.html http://googleonlinesecurity.blogspot.com/2010/07/rebooting-r...

jtimberman 15y ago

I never said this is a security vulnerability in Rails. I said it is a bug (and a serious one). A bug, as you may be aware, is a mistake, fault, failure, problem or other unintended or surprising behavior in software. See updates to my post for some additional clarification. I hope that helps. Thanks for reading!

mofey 15y ago
"The same user exploited another vulnerability". It wasn't exactly "another vulnerability". It still had to do with the same mass attribute assignment feature just in a different place.

pwim 15y ago

The irony of this all is that github isn't using attr_accessible at all in their application, but rather their own custom solution: http://news.ycombinator.com/item?id=3664999 http://news.ycombinator.com/item?id=3664999

ColdAsIce 15y ago

Egor Homakov is classy. Damn classy.