Rails core - Mar 2009 - Mass Assignment Vulnerability -- Another Suggestion

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
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> 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

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Sun, Mar 1, 2009 at 9:55 PM, Michael Koziarski
<michael@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.

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

>
>
>
> --
> Cheers
>
> Koz
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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


On Mon, Mar 2, 2009 at 6:55 AM, Michael Koziarski <michael@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
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> 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
>
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---