6 ms·

Rails Commit: Whitelist all attribute assignment by default

95 points by ricksta 15y ago

hasanove 15y ago
Thank you, @homakov
deleted 15y ago
[deleted]
dos1 15y 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.
- teyc 15y ago
  I'm not sure why the rails core team was so against this in the first place. Lack of humility.
  - Volpe 15y ago
    That's a bit harsh, it is at least a grey area. Mass-assignment by-default, with disabling fields you want to protect vs Mass-assignment off by default, with enabling fields you want to mass-assign. It's hardly just outright arrogance.
    - Peaker 15y ago
      In other words: Security vulnerabilities by default, with the ability to patch the vulnerability on a project-by-project basis. vs Security by default, with the ability to allow potentially insecure assignment on fields you want to assign.
      - rapind 15y ago
        This seems like an intentional oversimplification. Having mass-assignment on by default and therefore requiring the developer to create a whitelist of attributes for each of their models has a complexity cost associated with it. Failing to mention this at all shows a lack of objectivity. I happen to believe it's worth it for the increased security, however I'm not going to pretend that there isn't an associated cost.
        teyc 15y ago
        It is 2012, we should no longer have to debate about the acceptability of unvalidated inputs in a web application. Even PHP ditched register globals long ago.
dmishe 15y ago
They should've at least thanked the guy.
- ricksta 15y 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 15y ago
    Yeah there's a guideline on "editorialising in the title"
patio11 15y 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 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.
- sc00ter 15y 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.
  - patio11 15y 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.
javascriptlol 15y 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 15y 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.

95 points by ricksta 15y ago

hasanove 15y ago

Thank you, @homakov

deleted 15y ago

[deleted]

dos1 15y 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.

teyc 15y ago
I'm not sure why the rails core team was so against this in the first place. Lack of humility.
- Volpe 15y ago
  That's a bit harsh, it is at least a grey area. Mass-assignment by-default, with disabling fields you want to protect vs Mass-assignment off by default, with enabling fields you want to mass-assign. It's hardly just outright arrogance.
  - Peaker 15y ago
    In other words: Security vulnerabilities by default, with the ability to patch the vulnerability on a project-by-project basis. vs Security by default, with the ability to allow potentially insecure assignment on fields you want to assign.
    - rapind 15y ago
      This seems like an intentional oversimplification. Having mass-assignment on by default and therefore requiring the developer to create a whitelist of attributes for each of their models has a complexity cost associated with it. Failing to mention this at all shows a lack of objectivity. I happen to believe it's worth it for the increased security, however I'm not going to pretend that there isn't an associated cost.
      - teyc 15y ago
        It is 2012, we should no longer have to debate about the acceptability of unvalidated inputs in a web application. Even PHP ditched register globals long ago.

dmishe 15y ago

They should've at least thanked the guy.

ricksta 15y 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 15y ago
  Yeah there's a guideline on "editorialising in the title"

patio11 15y 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 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.

sc00ter 15y 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.
- patio11 15y 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.

javascriptlol 15y 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 15y 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.