Hacker News Reader

Comment by rsynnott | Hacker News Reader

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.

View on HN

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.

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.

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

Reply on news.ycombinator.com