undefined | Better HN

0 pointsrsynnott14y ago0 comments

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.

0 comments

GrumpySimon14y ago

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.

rsynnottOP14y ago

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.

thibaut_barrere14y ago

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

acqq14y ago

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.

Blaisorblade14y ago

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.

nailer14y ago

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?

flomo14y ago

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

j / k navigate · click thread line to collapse