Mass Assignment Vulnerability -- Another Suggestion

rubyonrails-core
1

Hey guys,

Several of you chimed in on the blog post I wrote last week discussing mass assignment vulnerabilities (and how nested mass assignment created further exposure). Thanks for pitching in a hand on the discussion. I've submitted a patch to docrails to beef up the mass assignment section of the security document to include some more details and countermeasures.

One countermeasure strikes me as particularly interesting. Littered throughout the security doc are references to the concept that whitelisting is a good security practice. So, what if we made the decision to force users to whitelist their model attributes (via attr_accessible)? This can be achieved in a current rails app with a simple initializer containing a single line:

ActiveRecord::Base.send(:attr_accessible, nil)

What if we made this the default behavior? In other words, in order to allow mass assignment at all, developers would be required to explicitly include this in their model. The generators could be updated to include the attr_accessible declaration in auto-generated code. Even if the generators whitelisted all parameters by default, it would still serve the community to see the attr_accessible line in every single model.

With 2.3 being so close, I'd think we'd want to consider a change like this for 3.0. What do you guys think?

-John

2

With 2.3 being so close, I'd think we'd want to consider a change like this for 3.0. What do you guys think?

Definitely not keen on this for 2.3, but for 3.0 it's definitely an option. My main concern with adding attr_accessible to every model is that it'll just be seen as noise by most developers, who'll simply either remove the line, or not bother understanding why it's there. Defaulting to 'everything inaccessible' would be far too strong and would just lead users to paste in some 'enable everything' snippet they found on the internet.

My gut feeling on this is that education is really the only option, and perhaps we need to emphasise these settings in an introductory tutorial, but I'm open to other ideas.

3

With 2.3 being so close, I’d think we’d want to consider a change like

this for 3.0. What do you guys think?

Definitely not keen on this for 2.3, but for 3.0 it’s definitely an

option. My main concern with adding attr_accessible to every model is

that it’ll just be seen as noise by most developers, who’ll simply

either remove the line, or not bother understanding why it’s there.

Defaulting to ‘everything inaccessible’ would be far too strong and

would just lead users to paste in some ‘enable everything’ snippet

they found on the internet.

My gut feeling on this is that education is really the only option,

and perhaps we need to emphasise these settings in an introductory

tutorial, but I’m open to other ideas.

I would welcome a change that will force me to use attr_accessible in all my models. I know why I need to use it, so I’m past the educational phase, I just don’t always remember to use it, and if Rails could jog my memory, that would be great.

If there was an option in config, I would turn it on in every new project, or use a template that defaults to on.

If other people choose to turn if off, that’s obviously their choice or maybe they’re not interested to understand why the option is there. But some people would take that opportunity, they’ll pause to ask “why am I fiddling with this?” and that would be a chance to reach to them and explain.

Assaf

4

I think we are touching a deeper problem here apart from the accessible/protected issue. From the start we realized that we need to store valid models in the database so we wrote validations. Strangely, it did not occur to us that security issues are part of the validation process. They are not some extra special thing that can happen in some super firewall (controller). Now that AR gets more power, this thing is creeping out.

What John is raising here is that our security layer is brittle because it's not DRY. We need to write code to remove disallowed attributes in every controller that could potentially send offending data to some model. And now that we have nested attributes, forgetting a controller just became easier.

Authentication belongs to the controller. Securing models should belong in the validation cycle, ensuring that *all* code hitting the models goes through the same security validation process.

Gaspard

5

Authentication belongs to the controller. Securing models should belong in the validation cycle, ensuring that *all* code hitting the models goes through the same security validation process.

Gaspard, I'd argue that the issue at hand is authorization, and not necessarily validation. A set of edits to an existing record can be valid, but whether or not the logged in user has the authority to submit those changes is an entirely different issue.

What John is raising here is that our security layer is brittle because it's not DRY. We need to write code to remove disallowed attributes in every controller that could potentially send offending data to some model. And now that we have nested attributes, forgetting a controller just became easier.

This is precisely why I was suggesting that controllers should be responsible for exposure of nested mass assignment. The models should reject mass assignment unless it is explicitly circumvented. The problem with the current implementation is that it's an "all or nothing" proposition, which requires us to have to blacklist unwanted mass assignments instead of whitelisting them at a localized level (either limit it to a single action, or better yet to a single invocation of save!/attributes=).

I'd argue that it's ok to have to write this code in the controller. I agree that the controller is the proper place for authentication. Furthermore, I was always under the impression that authorization should be applied at the controller level too. I may be misinterpreting you, but it seems to be that you're suggesting differently.

In many cases, it'd be silly to push authorization down to the model level. Consider any basic rails app where there are users and admins. You don't beef up all of your models with code that allows them to be aware of who is logged in. Instead, you create before_filters in the controllers, and restrict/grant access to specific actions. In other words, the controller decides who is authorized to create/edit different kinds of models. The models are clueless about the authorization distinctions between admins and users.

However, as authorization/authentication becomes more complicated (consider implementing an ACL), this data does get pushed out to models (as opposed to being hard-coded in your controllers). The usual route is to write an Authorizer class that is responsible for granting access. However, the models/data that it protects (those data not associated with actually storing the permissions) should continue to remain oblivious to this fact.

I'm curious what everyone views as the "best practice" for where/how to apply authorization. For what it's worth, conversation continues to trickle in on the original blog post: http://blog.smartlogicsolutions.com/2009/02/24/rails-23-nested-object-forms-im-not-crazy-about-them