Github Is Classy | Hacker News Reader

Github Is Classy

jtimberman.housepub.org

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.

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.

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 ;-)

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

Or install a plugin and inherit one:

https://github.com/josevalim/inherited_resources/blob/master...

Yes, construction of raw SQL queries outside library code is a security hole. The same with writing browsers in C or C++. These bad design choices can be traced to countless security issues. The same for generating HTML using string manipulation. These are all stupid ideas, and until people realise that they're holes we will be stuck with endless security problems. And it's the coward's way out to hide behind the "sharper tools" nonsense. You don't hear chainsaw vendors wearing safety flaws as a badge of pride.

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?

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.

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 :)

Pretty much every tutorial I can find on using controllers in Rails uses mass assignment, and the majority of them (including the beginners guide on guides.rubyonrails.org) don't mention attr_accessible or the security risk in allowing unrestricted mass assignment.

We're telling people to RTFM, but the FM doesn't say a thing about how this is dangerous.

If it was considered standard to avoid use of your brakes at all costs, and accelerate at maximum speed at all times, and this was reflected in the owner's manual - sure, I would think cars were unsafe.

It may not be mentioned in the beginner docs... I feel ok insisting that professional software developers read more than an intro to the framework they are using.

Also not mentioning it isn't the same as avoiding it 'at all costs'.

To continue the analogy: Sales person says "Look how fast this Ferrari can go!", then later, in the manual, "Drive at a responsible speed given your local laws, etc."

All that said I do actually think that changing the default to be more annoying and more safe is a good thing (and it's been done https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac...). Just saying that I agree with raganwald -- this isn't even close to a 'security vulnerability'.

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

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.

Scott Meyers (author of Effective C++ et al.) has talked about this subject many times (including in the aformentioned book) and it puts the ball squarely in the Rails team's court.

Let's make the reasonable assumption that your clients—the people using your interfaces— are trying to do a good job. They're smart, they're motivated, they're conscientious. They're willing to read some documentation to help them understand the system they're using. They want things to behave correctly.

That being the case, if they make a mistake when using your interface, it's your fault. We're assuming they're doing their best—they want to succeed. If they fail, it's because you let them. So, if somebody uses your interface incorrectly, either they're working hard at it (less likely) or your interface allowed them to do something easy that was not correct (more likely). This puts the shoe on the foot not used to wearing it: it means that responsibility for interface usage errors belongs to the interface designer, not the interface user.

Source: Scott Meyers: The Most Important Design Guideline? http://www.aristeia.com/Papers/IEEE_Software_JulAug_2004_rev...

I think we’re in agreement. I don’t like the design, I don’t use it myself. I just think that it’s not correct to say that Rails has a security vulnerability and especially that Rails is vulnerable by default. Both of these expressions carry the false connotation that all rails apps are vulnerable and that the fix for the vulnerability lies in patching Rails, when in actuality Rails has a questionable design problem, and every developer has the power in their own hands to secure their application.

If I make something and it has a property that can be directly traced to recurring security problems, then that is a vulnerability. That it might not be cut out of whatever neat template you've built in your mind doesn't change that fact.

Scaffolding is part of rails, so fixing the bug does involve patching rails.

"Windows has no security vulnerabilities itself, since you can edit the exe of any malfunctioning app. A security conscious app developer is responsible for auditing Windows and making necessary changes. Heck, most vulnerabilities have already been documented, and sometimes the workaround doesn't even involve coding. " See how silly that is?

So in other words: bad design is justified by the notion that people shouldn't make mistakes. Praise the lord you don't design anything that can burn, irradiate or cut people.

I don’t think you should try to rephrase my words in your words, because in doing so you are completely misrepresenting what I said. So please, stick to just reading my words, and if you want to disagree with what I actually write, do that. What I said was:

If you want to say that you disagree with the choice as it was originally made, I agree with that too. Meaning, I disagree with the design. At no time did I say that this or any design was “justified.” That’s your word, not mine. So I think you should stick to espousing your opinions, not disagreeing with things I didn’t say.

I’m very clear that I don’t like the design and don’t use the feature myself. But as I posted elsewhere, I just think that it’s not correct to say that Rails has a security vulnerability and especially that Rails is vulnerable by default. Both of these expressions carry the false connotation that all rails apps are vulnerable and that the fix for the vulnerability lies in patching Rails, when in actuality Rails has a questionable design problem, and every developer has the power in their own hands to secure their application.

My issue is entirely with the nomenclature, not with whether I like or dislike mass assignment.

I'm not interested in your sophistry. You're saying it is not a vulnerability in rails, on the basis that it can be fixed by users. That's tantamount to justifying it, regardless of the degree. Rails is dead wrong here and I'm not interested in playing the "try to be right on the Internet" game with you.

Downvoted once again by people who have no reply.

It's not sophistry, the distinction raganwald is making is relevant.

However, even if it is not technically accurate, in the interest of getting the topic in front of as many Rails developers as possible, it's probably better to sweep that distinction under the rug and let them figure out for themselves whether it applies to them.

So your contention is that because something is "relevant" that prohibits the possibility of it being sophistic? The distinction is completely artificial. This bug in rails can be directly traced to recurring security problems. If that's not a vulnerability, then we speak a different dialect of English.

Ok, look, I actually think it is a vulnerability to most approximations which isn't what comes across in what I wrote.

That said, I don't think what raganwald was saying was sophistry at all. Sophistry implies an attempt to deceive. He was just being pedantic and a little narrow with his definition of vulnerability.

So when you say his argument sophistry, and then follow up with "... and I'm not interested in playing the "try to be right on the Internet" game with you." you're just lashing out. So that's probably why people (not me) where downvoting without replying.

  I'm not interested in playing the "try to be right on the Internet" game with you.

Just as well, it seems that we agree on so much that focusing on where we are saying different things devolves into pedantry precisely because we agree on the important matters.

As I said elsewhere:

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 ;-)

By the way, this is the perfect example of modernistic trash-thought that focuses on details and not behaving usefully. You navel gaze over the "correct" usage and nitpick my argument instead of getting behind the position that is going to prevent harmful behaviour.

raganwald likes to put all the blame on "novices" and tell people to "RTFM" then take umbrage to the suggestion that he's defending the existence of bad design based on an idiotic, legalistic approach to conversation.

Especially ironic considering the thing that sparked this whole discussion was a vunerability on GitHub, of all places. If the developers at GitHub are lumped in with the uneducated "novices", who exactly are the experts?

"Young man, do not take that flippant tone with me (raps ruler on desk). Those novice developers failed to RTFM."

I suppose that the github developers all failed to RTFM, too, right? So either they're all n00bs, or we can safely assume that everyone makes mistakes when the framework makes mistakes easy to make.

(Also, I suspect that you're joking about the "young man" thing, but it's probably worth pointing out that I've been coding for a long time. I may even be older than you. But I still make mistakes, and I definitely appreciate it when my frameworks make the Right Way the Only Way. It's not just about novices.)

Of course I was joking about the young man thing. As I suspected you were with the “Gee” thing. It’s not a formal debate, a little “local” colour makes comments like this more enjoyable to read.

I used to see this argument a lot from PHP people. You know, it's not PHP that's insecure, it's all those novices writing insecure PHP code.

Except that PHP makes it so easy to write insecure code that novices are pretty much guaranteed to produce vulnerabilities and even experts have to be vigilant to avoid accidentally stepping on one of the many landmines in the language.

If the designers of the language (or framework) put a landmine in it, and a developer steps on it, the designers of the language absolutely bear some responsibility for the fact that the landmine was there to be stepped on.

sounds like pointers in C vs C++

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.

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.

> 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 policy of the optional tool is broken. That's a bad thing. The entire point of a security baseline is to provide an attractor towards which design and code approach without resistance. If you want insecure, fine, but you'll have to go out of your way to get it.

Rails is not doing that.

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

And I would agree. The allure of Rails is how easy it is to get something going. The problem with Rails is how easy it is to get any old thing going.

See also: every major PHP application ever written.

The problem is, the fact that you don't use mass assignment is by far the exception rather than the rule. Every notable Rails tutorial uses mass assignment. Every person using InheritedResources is using mass assignment. GitHub is using mass assignment. Passing in params more or less raw from the controller, and leaving cleansing and validation up to the model is as strong an idiom as there is in Rails.

Regarding your ActiveRecord example, I would totally agree that there was a hole if the standard approach to running SQL was to use find_by_sql with string interpolation. Fortunately, that's appropriately viewed as an option of last resort, with appropriate amounts of "here be dragons" around the dangers of calling that method. Meanwhile, tutorials gleefully do mass-assignment, with perhaps a footnote about attr_accessible, which was of course not included for the sake of brevity.

It might be worth saying that Rails strictly as a framework does not have a security problem with mass assignment, only a very dangerous tool, but there is certainly a problem with the culture regarding it.

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

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.

>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?

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.

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.

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

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.

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.

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.

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

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

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.

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.

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!

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.

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.

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.

You are told that your back door was open 3 months ago. You "investigate" then tell the witness they are mistaken, the back door is closed. 3 days later the witness walks through the back door which has been open for 3+ months and shits in your fridge. You suspend his account and write an inflammatory post about the guy for has been telling you for 3 months that your back door was open. He could have walked in and burned your house down, shredded your reputation, and caused every paying developer you have to jump ship. He didn't.

GitHub has handled this situation in the worst possible way, from start to finish. Thank god an ethical hacker shit in their fridge. The alternative is frankly unimaginably bad for the whole community.

Did he actually contact GitHub directly?

Didn't he just post issues/comment on the issue in rails/rails?

That was my understanding as well. @chives, can you elaborate?

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

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

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.

> A bad workman always blames his tools.

So do expert workmen when given crappy tools.

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

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!

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.

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

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

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!

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

I think they handled this like adults. Very impressive.

Reply on news.ycombinator.com