Re: Mass Assignment Vulnerability -- Another Suggestion

John Trupiano
2009-03-02T20:06:57Z

> 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


>
>
> Gaspard
>
> On Mon, Mar 2, 2009 at 6:55 AM, Michael Koziarski
<mich...@koziarski.com> wrote:
>
> >> 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.
>
> > --
> > Cheers
>
> > Koz