Hacker News Reader

Comment by ceejayoz | Hacker News Reader

The point, though, is that this is more of a Rails issue than a Github issue. If Github gets it wrong, others are likely to.

View on HN

The users who trust Github don't care if it was a "rails issue" or a "github issue"

It's a Github issue no matter what the cause or responsible party.

If Github was run on unpatched windows XP boxes with all IPs public, you wouldn't argue that it was Microsoft's fault because "everyone knows" what a bad idea that would be. The base assumption should not to be completely reliant on your environment and frameworks to be secure, because they very likely have un-patched bugs and exploits. You do what you can to harden yourself up, but when something goes wrong - it's your problem.

But (to continue your analogy), github was run on fully patched XP boxes, in which case: yes, it would have been Microsoft's fault.

And it would have ultimately been Github's fault (in the eyes of their users) in that case too -- because they are providing the service, and their choices (XP / Rails, Firewall / Application Firewall / Audit) is their fault.

Contrary to how a lot of things are handled today, fault can be shared by multiple parties. what Egor did was get exposure and reaction from a lot of involved parties, where he was unable to get any reaction from some of them (Rails team) in the past.

Well, it's a beginner-level Rails mistake; it is not precisely an obscure issue. Googling 'attr_accessible' will show you discussion going back years, and it has been actively exploited before. I'm shocked that the mistake was made in Github, though.

It's a beginner-level Rails mistake but it's very widely seen in codebases: it's a real, common threat.

Obviously not beginner-level if more famous sites suffer from the same problem -- Egor mentions some in his posts and he is only one person -- imagine what all the black hats can do working in parallel.

By your reasoning, since tons of C programs can't use strcpy() correctly and thus contain buffer overflows, it takes a C master to use strcpy() correctly. I'd say instead that the kind of reasoning is wrong and that even experts can make beginner-level mistakes (not so often). But these are the kind of mistakes a good framework/language/etc. should try to prevent - especially if security-related. Tons of research tries to reduce error-prone programming activities.

Allow me to rephrase that:

"Well, it's a beginner-level PHP mistake; it is not precisely an obscure issue. Googling 'SQL Injection/register_globals/get_magic_quotes/etc' will show you discussion going back years, and it has been actively exploited before. I'm shocked that the mistake was made in ..., though."

Ten+ years of register_globals, get_magic_quotes and SQL injection attacks (etc etc etc) in PHP show that even well-known issues still bite people everyday. IMO, it's up to the framework developers to make it easy to do the right thing and damn hard to do the wrong thing.

I don't think it's on the same scale as register_globals or magic_quotes; both of those were much stupider. It's still very stupid, granted, but not on the same scale as register_globals.

My understanding is that it's an insecure built-in default. Ie, the mistake is by the Rails framework developers, and Rails users have to explicitly secure it. Is that incorrect?

IMHO, it's a bad practice which is very widely promoted in Rails guides and scaffolding code.

Mass-assignment is very difficult to get right in any complex app (e.g. with multiple user roles) and the simplest way to avoid these problems would be to discourage the use of 'update_attributes' and teach people to explicitly set model properties.

(Of course, Rails folks probably hate the non-DRY aspect of this, but I prefer to look at my code and see exactly what's happening with user input.)

It is a Github issue in the fact that they didn't protect against this issue when they easily could have.

Others have made the comparison to PHP's `register_globals`.

Yes, Github had vulnerable code. It's also true that Rails apparently defaults to leaving that bit of code vulnerable. A saner default seems in order, if even highly competent Rails devs can be caught by this.

Reply on news.ycombinator.com