Rails Commit: Whitelist all attribute assignment by default

https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac53c57a302b162fd736

by ricksta • 14 years ago

95 28 14 years ago

github.com

hasanove• 14 years ago

Thank you, @homakov

dmishe• 14 years ago

They should've at least thanked the guy.

ricksta• 14 years ago

I actually had "Thanks to Homakov" inside the title of this post but I guess I wasn't allowed to have that in the title and someone changed it.

dools• 14 years ago

Yeah there's a guideline on "editorialising in the title"

patio114• 14 years ago

I don't think hacking sites to do security activism is a good idea, at all. However, many people over a period of years said that this change needed to be made, in all of the friendly, ethical, polite OSS-approved ways. Tickets, blog posts, emails to security, etc etc. I got to the issue a few years late, saw the WillNotFix tickets, and wrote up - I kid you not - an on-dead-tree journal article which said:

[W]rite access to sensitive data should be limited to the maximum extent practical. A suitable first step would be to disable mass assignment, which should always be turned off in a public-facing Rails app. The Rails team presumably keeps mass assignment on by default because it saves many lines of code and makes the 15-minute blog demo nicer, but it is a security hole in virtually all applications.

http://queue.acm.org/detail.cfm?id=1964843

This doesn't justify the Bad Guys abusing third-party sites with this, but the Good Guys did everything right and did not achieve a fix as a result of doing so.

sc00ter3• 14 years ago

AKA "The end justifies the means". It wasn't ideal, but it achieved and end result that others have been asking for for years to no avail. That justifies the means in my book, and then some. I hope Egor will be given sufficient indemnity for his actions, as, even if misguided, they were clearly well intentioned.

patio114• 14 years ago

I strongly disagree both in general and as regards web vulnerabilities. For one, Github is in no way responsible for the decisions of the Rails project, and this stunt will cost Github five to six figures. (Anyone who thinks this is in any way exaggerated has not been on an Incident Response team or read what has to happen in industry. Heck, there are individual customers of Github who may have to push the Big Red Button right now.)

For another, there exist many, many applications against which one could demonstrate a bug in Rails or Ubuntu or SSL or just the finding "Lol, P=NP." Pragmatically, would you like that to be your application? I would very, very much not like someone to do one of these to my sites. It would cause an instant nightmare for me. The happy outcome is I lose thousands of dollars and don't sleep for most of a week. The unhappy outcomes are not upper-bounded by death of my business.

n.b. Busting into Basecamp to make a point about Rails would also be morally irresponsible.

teyc5• 14 years ago

This little exercise reminds us that the dysfunction seen inside corporations are just as easily present in open source communities. In the end, management holds the keys to what gets checked in and what doesn't.

The curious twist in this whole tale is that PHP- because of its history of being insecure- actually had a hardened distribution, while Rails didn't.

This type of smugness creates its own karma, as evidenced by the red-faced hack of a premier RoR site.

dos1• 14 years ago

Good for Egor Homakov. I don't necessarily agree with his methods in this case, but in the end, the net result of his actions was positive for web security. And as so many others have pointed out, when the proper channels aren't working, sometimes a little spectacle is just what you need to instigate change.

In other news, I'm not sure why the rails core team was so against this in the first place. Making things safe by default is usually a good idea. This change doesn't seem to add too much additional ceremony, and people had been asking for this for some time.

teyc5• 14 years ago

   I'm not sure why the rails core team was so against this in the first place.

Lack of humility.

Volpe• 14 years ago

That's a bit harsh, it is at least a grey area.

Mass-assignment by-default, with disabling fields you want to protect

Mass-assignment off by default, with enabling fields you want to mass-assign.

It's hardly just outright arrogance.

javascriptlol• 14 years ago

Unfortunately people never learn from the past. Security holes directly traceable back to the design of C are still trickling out after 30 years. The entire construction of the web has failed to learn from this lesson. The whole design is "fail open", and one mistake ends with site credentials being dumped on pastebin.

oomkiller• 14 years ago

It still auto-adds the attr_accessible list which is nearly as bad as allowing mass assignment in the first place. At most it should add an informative comment, and the error message about being unable to mass assign attributes should be made better, possibly with a URL to the existing mass assignment Rails guide.

Rails Commit: Whitelist all attribute assignment by default

Related Stories