Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Github Is Classy

jtimberman.housepub.org

Post reply on HN
Print view View original

11–20 of 70 posts

Re: Github Is Classy

#11

Post by raganwald »

jtimberman wrote: post #7
raganwald wrote: post #5

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 vulnerabil…

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

Re: Github Is Classy

#12

Post by lusis »

heymishy wrote: post #2

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 mo…

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.

Re: Github Is Classy

#13

Post by thenduks »

skeletonjelly wrote: post #8
raganwald wrote: post #5

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 vulnerabil…

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?

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

Re: Github Is Classy

#14

Post by [deleted] »

raganwald wrote: post #11
jtimberman wrote: post #7

Earlier quoted context omitted.

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 w…

[deleted]

Re: Github Is Classy

#15

Post by benatkin »

raganwald wrote: post #11
jtimberman wrote: post #7

Earlier quoted context omitted.

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 w…

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

Re: Github Is Classy

#16

Post by jacques_chester »

raganwald wrote: post #5

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 vulnerabil…

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.

Re: Github Is Classy

#17

Post by raganwald »

timr wrote: post #10
raganwald wrote: post #5

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 vulnerabil…

"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 s…

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.

Re: Github Is Classy

#18

Post by mkramlich »

kenneth_reitz wrote: post #3

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.

Re: Github Is Classy

#19

Post by raganwald »

jacques_chester wrote: post #16
raganwald wrote: post #5

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 vulnerabil…

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 sop…

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.

Re: Github Is Classy

#20

Post by jacques_chester »

raganwald wrote: post #19
jacques_chester wrote: post #16

Earlier quoted context omitted.

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 sop…

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 no…

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

Post reply on HN