Github Is Classy

http://jtimberman.housepub.org/blog/2012/03/04/github-is-classy/

by craigkerstiens • 14 years ago

97 67 14 years ago

jtimberman.housepub.org

heymishy2• 14 years 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• 14 years 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.

mofey2• 14 years ago

The public key form update vulnerability was based on the same concept in a different place.

kenneth_reitz3• 14 years 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• 14 years 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.

raganwald9• 14 years 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_reitz3• 14 years 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• 14 years 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.

envex2• 14 years ago

How did they do a bad job?

They (temporarily) disabled the account, fixed the problem and re-enabled the account. All while keeping people in the loop about what happened......at 9am............on a sunday.

I may be wrong, but that seems like a pretty reasonable way of handling the situation.

gurkendoktor• 14 years ago

The only thing that GitHub sells on top of its free services are private repositories! I am surprised how little flak they get.

xentronium• 14 years ago

> If anyone should be getting flamed for this, it should be the Rails maintainers that weren't open to the discussion.

A bad workman always blames his tools.

ryanbrunner5• 14 years ago

If someone made a mitre saw that didn't have a guard over the blade, and people were getting their fingers cut off, I'd absolutely blame the tool.

SideburnsOfDoom• 14 years ago

> A bad workman always blames his tools.

So do expert workmen when given crappy tools.

shasta• 14 years ago

I think they handled this like adults. Very impressive.

raganwald9• 14 years 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.

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.

jtimberman2• 14 years 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.

raganwald9• 14 years 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 ;-)

skeletonjelly• 14 years ago

Sure. But I feel like it's the job of a framework to hide away complexities to help the developer write code faster. I don't use Ruby/Rails but from my experience with ASP.NET MVC it seems that these helper functions exist for a reason. So why provide a tool that's inherently insecure? Caveat emptor?

heymishy2• 14 years ago

Sure abstraction is good and speeds up workflow, but in this case, it was rolled into main without a lot of noise and essentially opened up developers to the situation without giving fair warning that this type of thing COULD happen. I'm not saying the blame lies solely with the Rails team - but the issue here is more with full disclosure and due diligence in communicating to the community.

thenduks2• 14 years ago

On one hand you have `update_attributes` - IMO you should just never use this (seems raganwald agrees), but hey, it exists and it can be useful for simple stuff. So on the other hand is `attr_accessible` - You need to specify what is allowed to be mass-assigned if you want to use mass-assignment. This stuff is covered in the basic Rails Guides (http://guides.rubyonrails.org/security.html#mass-assignment).

Saying that `update_attributes` is a security vulnerability doesn't really make sense... it's like if I said cars can accelerate to very unsafe speeds by default and this is a huge safely risk, ignoring the fact that cars also come with brakes :)

timr2• 14 years ago

"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."

Well, gee...so maybe some novice developers write an app using the generators and scaffolding baked into Rails, and as a result they get a security vulnerability that a more experienced developer might have avoided by doing extra work, and you say that's not a security problem in Rails?

Maybe it isn't a "vulnerability", per se, but it sure seems like a poor design choice. A "hardened" framework is going to make parameter whitelisting the default behavior, even if it's a little less convenient. If the github developers (who are presumably experienced) can make this mistake, then anyone can make this mistake.

raganwald9• 14 years ago

so maybe some novice developers write an app using the generators and scaffolding baked into Rails, and as a result they get a security vulnerability that a more experienced developer might have avoided by doing extra work, and you say that's not a security problem in Rails?

Young man, do not take that flippant tone with me (raps ruler on desk). Those novice developers failed to RTFM. If you want a framework that produces secure applications for inexperienced developers who do not read the fine manual, you are setting a laudable goal for Rails, but I hardly think that “Secure even for people who are 1. novices and 2. compound their ignorance by refusing to RTFM” is synonymous with “security problem.”

If you open your history book to when Rails was first becoming popular, there were many people grumbling that because it was built on top of a dynamic language that it was vulnerable to all sorts of bugs caused by novice developers getting their types wrong. The argument at the time was that Rails provided a certain type of freedom and power at the expense of certain safeguards. The same argument was played out when people started noticing that monkey-patching run amuck caused certain problems.

I agree with you that this is a design choice. If you want to say that you disagree with the choice as it was originally made, I agree with that too. I won’t say that I would have made a different choice back when that feature was first baked in, I did not write the framework. But here we are today, and it seems like a very good time to make a different choice.

ShiningRay• 14 years ago

sounds like pointers in C vs C++

jacques_chester2• 14 years ago

You confusing mechanism and policy. Sure, Rails includes a mechanism that closes this security hole. But it requires additional effort to apply a more secure policy. The security baseline is not elevated by the mechanism.

Historical experience has shown that no matter how excellent the mechanism, the practical security baseline is determined by the default policy.

Windows NT has had, since inception, a far more sophisticated security mechanism than the default unix model. But that doesn't matter in the real world, because to retain backwards compatibility with Windows 95 (and through to DOS), almost no security policy was shipped by default until Vista.

Was Windows insecure? According to your criteria, no. According to the actual real-world consequences, yes.

The bottom line is that policy matters and that in this case, the rails crew have chosen a demonstrably bad policy.

raganwald9• 14 years ago

The way you describe it, it sounds like there is a hole in every app and developers must close the hole to be secure. My claim is that Rails provides a completely optional tool, mass assignment, that opens a hole, and another completely optional tool that closes the same hole. And they document this well.

The one tool can be said to have a poor policy, I agree with that. But Rails does not have a hole in it. I did not have to rush to “close” holes in the projects I’m responsible for when I read these posts because I hadn’t used the tool in the first place.

What Rails has is a poorly designed tool. But if you’re going to say that if a developer can misuse the tool then Rails has a security problem, then I’m going to say that the exact same thing is true of ActiveRecord.

yuhong• 14 years ago

The funny thing is that Windows 95 was released AFTER Windows NT.

Drbble2• 14 years ago

Thank you! "policy vs mechanism" popped into my head when this debate for going, but I didn't write it up well like the above post did.

cbs• 14 years ago

>And likewise, for people who RTFM before using mass assignment, their applications are not vulnerable, correct?

Brilliant point. While we're all talking about this issue, lets all team up, all of us here on HN, right now, to email php-internals and ask them to bring back register_globals and turn it on by default. It was a mistake on their part to disable it. After all, who doesn't read and follow all the documentation?

iamgilesbowkett• 14 years ago

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.

I believe this is absolutely correct.

Peaker• 14 years ago

Convenience over safety is one of the repeated, expensive, wrong choices so endemic in the software world.

Convenience is truly overrated. Inconvenience is cheap, lack of safety (which translates to security issues and bugs) is far far more expensive.

tylermenezes• 14 years ago

> or that it is technically true that Rails is insecure by default

PHP learned their lesson a while ago with register_globals: if you need to specifically disable people from attacking your site in many places, it's inevitable that you'll miss one or two.

jamesgeck0• 14 years ago

I'm a just a fool with a simple Rails app and copy of _The Design of Everyday Things_, but this seems like a usability issue.

The default scaffolding doesn't contain an attr_accessible property, so it's not visible to the user that there's something missing. There's also no feedback from Rails that your attributes are insecure; only feedback if one tries to change an attribute explicitly made non-accessible. Figuring out which attributes must be private is a problem possibly beyond the scope of core Rails, though.

Perhaps a command like `rake routes` for publicly accessible model attributes might be of benefit.

ryanbrunner5• 14 years ago

On top of that, Rails sometimes seems like it goes out of it's way to make attr_accessible a pain to use. I know more than once on my team someone has spent more than a few minutes trying to figure out why a model wasn't updating before they remembered to add a property to attr_accessible. In one case, we had a (rarely-used) field uneditable for a few weeks due to it not being included on the list.

I really liked wycats proposal of moving this sort of thing to the controller - it's a much more logical place to look, and I do think there's a thing as a too-skinny controller - mapping inputs to model attributes is a perfectly sane thing for the controller to do IMO.

aneth• 14 years ago

Totally agree. The problem is the idiom of using mass assignment, which IMO should almost never be used.

This is not a bug or a security hole in Rails, but an issue with programmers not paying attention.

If Rails had no attr_accessible feature, it would be standard practice to always filter attributes in the controller, and no one would call this a rails issue - they would put the blame where it belongs: Github.

Instead, because attr_accessible exists, people are flaming that it should be enabled by default.

smackay• 14 years ago

This short excerpt from the Hitchhikers Guide to the Galaxy seems appropriate:

“`...You hadn't exactly gone out of your way to call attention to them had you? I mean like actually telling anyone or anything.' `But the plans were on display...' `On display? I eventually had to go down to the cellar to find them.' `That's the display department.' `With a torch.' `Ah, well the lights had probably gone.' `So had the stairs.' `But look you found the notice didn't you?' `Yes,' said Arthur, `yes I did. It was on display in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying ”Beware of The Leopard".'"

generalk• 14 years ago

  > But for guys like me who never use mass assignment, our 
  > applications are not vulnerable by default, correct?

Yes, if you never ever call `update_attributes` you're not vulnerable. This is, however, the most common way to perform model updates.

  > And likewise, for people who RTFM before using mass 
  > assignment, their applications are not vulnerable, 
  > correct?

No! Absolutely not! The Github exploit is a prime example of this. Solid developers, who have obviously ReadTFM, can easily fall victim to this because the default is to be insecure!

The fact is that Rails ships code generators that give you insecure code and put the onus on you to do something about it.

I'm not absolving any developer of their responsibility to not shoot themselves in the foot. Ultimately, if this Github exploit had been used to do serious damage, Github, not the Rails core team, would have been responsible. It is the developer's responsibility to ensure that their code is as free of vulnerabilities as they can make it.

JumpCrisscross• 14 years 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-r...

jtimberman2• 14 years 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!

mofey2• 14 years 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• 14 years 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

Github Is Classy

Related Stories