Re: [Rails-core] strong parameters safety issue enables accidental mass assignment
>From my Android phone on T-Mobile. The first nationwide 4G network.
Original message
From: Prem Sichanugrist
Date: 20/10/2014 18:07 (GMT-08:00)
To: rubyonrails-core@googlegroups.com
Cc: rubyonrails-core@googlegroups.com
Subject: Re: [Rails-core] strong parameters safety issue enables accidental
mass assignment
We already have plans to make Parameters not inherited from Hash before Rails
4.2, but we couldn't do it because it breaks backward compatibility. What we
did, instead, was to make sure we have test to cover those cases, and
reimplement those methods that leaked Hash object.
https://github.com/rails/rails/blob/v4.2.0.beta2/actionpack/test/controller/parameters/accessors_test.rb
https://github.com/rails/rails/blob/v4.2.0.beta2/actionpack/test/controller/parameters/mutators_test.rb
If we missed something, please let us know.
After 4-2-stable has been cut out, and master becomes 5.0, then we'll make
Parameters not inherited from Hash.
Thanks,
Prem
On Thu, Aug 7, 2014 at 3:19 AM, Matt Jones wrote:
On Aug 6, 2014, at 12:51 PM, johannes.schlumber...@appfolio.com wrote:
[snip]
Why does that matter?
It matters because it is possible for a developer to accidentally lose that
capability accidentally very easily on the way from the controller (where
permit happened and the capability gets created) to the model (where the
capability gets used). This loss does happen silently and effectively disables
mass assignment protection.
How does that happen?
The only class aware of the 'permitted?' capability is
ActiveController::Parameters, if we call a method that is not aware of the
capability it can get lost as a side effect:
class SomeModel < ActiveRecord::Base
#fields :name, :protected_secret_field
include ActiveModel::ForbiddenAttributesProtection
end
#imagine a request w/ params = {'name' => 1, 'protected_secret_field' => 2}:
params.reject!{|k,_|['controller', 'action'].include? k}
params.permit(:name)
SomeModel.new(params) #Exception, world is OK
SomeModel.new(params.symbolize_keys) #No Exception, secret overwritten
This is a bug in `symbolize_keys`. It appears to have been fixed (accidentally?
on purpose?) on master:
https://github.com/rails/rails/commit/f1bad130d0c9bd77c94e43b696adca56c46a66aa
by starting the loop with `self.class.new` instead of `{}`.
There’s some additional future changes coming up in Ruby 2.2.0 with methods
like `reject`:
https://www.ruby-lang.org/en/news/2014/03/10/regression-of-hash-reject-in-ruby-2-1-1/
that seem likely to further complicate the situation.
—Matt Jones
--
You received this message because you are subscribed to the Google Groups "Ruby
on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups "Ruby
on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] strong parameters safety issue enables accidental mass assignment
We already have plans to make Parameters not inherited from Hash before Rails
4.2, but we couldn't do it because it breaks backward compatibility. What we
did, instead, was to make sure we have test to cover those cases, and
reimplement those methods that leaked Hash object.
https://github.com/rails/rails/blob/v4.2.0.beta2/actionpack/test/controller/parameters/accessors_test.rb
https://github.com/rails/rails/blob/v4.2.0.beta2/actionpack/test/controller/parameters/mutators_test.rb
If we missed something, please let us know.
After 4-2-stable has been cut out, and master becomes 5.0, then we'll make
Parameters not inherited from Hash.
Thanks,
Prem
On Thu, Aug 7, 2014 at 3:19 AM, Matt Jones wrote:
> On Aug 6, 2014, at 12:51 PM, johannes.schlumber...@appfolio.com wrote:
> [snip]
>>
>> Why does that matter?
>> It matters because it is possible for a developer to accidentally lose that
>> capability accidentally very easily on the way from the controller (where
>> permit happened and the capability gets created) to the model (where the
>> capability gets used). This loss does happen silently and effectively
>> disables
>> mass assignment protection.
>>
>> How does that happen?
>> The only class aware of the 'permitted?' capability is
>> ActiveController::Parameters, if we call a method that is not aware of the
>> capability it can get lost as a side effect:
>>
>> class SomeModel < ActiveRecord::Base
>> #fields :name, :protected_secret_field
>> include ActiveModel::ForbiddenAttributesProtection
>> end
>>
>> #imagine a request w/ params = {'name' => 1, 'protected_secret_field' => 2}:
>> params.reject!{|k,_|['controller', 'action'].include? k}
>> params.permit(:name)
>> SomeModel.new(params) #Exception, world is OK
>> SomeModel.new(params.symbolize_keys) #No Exception, secret overwritten
>>
> This is a bug in `symbolize_keys`. It appears to have been fixed
> (accidentally? on purpose?) on master:
> https://github.com/rails/rails/commit/f1bad130d0c9bd77c94e43b696adca56c46a66aa
> by starting the loop with `self.class.new` instead of `{}`.
> There’s some additional future changes coming up in Ruby 2.2.0 with methods
> like `reject`:
> https://www.ruby-lang.org/en/news/2014/03/10/regression-of-hash-reject-in-ruby-2-1-1/
> that seem likely to further complicate the situation.
> —Matt Jones
--
You received this message because you are subscribed to the Google Groups "Ruby
on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] strong parameters safety issue enables accidental mass assignment
Hi everyone, I went ahead and made a strong_parameters compatible gem [1] whose Parameter class does not inherit from Hash anymore. It does not have any ancestors. The syntax for permit statements is identical to strong_parameters, the require feature is supported and parameters can be accessed in a Hash like syntax using the []-operator. We have used this gem for about a month now in some of our biggest rails apps without problems. My main concern, that I expressed when opening this thread, is resolved by this gem. If it is useful to the Rails Project or to anyone else, it is under MIT license. --Johannes [1] https://github.com/appfolio/shields_up -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] strong parameters safety issue enables accidental mass assignment
On Aug 6, 2014, at 12:51 PM, johannes.schlumber...@appfolio.com wrote:
[snip]
>
> Why does that matter?
> It matters because it is possible for a developer to accidentally lose that
> capability accidentally very easily on the way from the controller (where
> permit happened and the capability gets created) to the model (where the
> capability gets used). This loss does happen silently and effectively disables
> mass assignment protection.
>
> How does that happen?
> The only class aware of the 'permitted?' capability is
> ActiveController::Parameters, if we call a method that is not aware of the
> capability it can get lost as a side effect:
>
> class SomeModel < ActiveRecord::Base
> #fields :name, :protected_secret_field
> include ActiveModel::ForbiddenAttributesProtection
> end
>
> #imagine a request w/ params = {'name' => 1, 'protected_secret_field' => 2}:
> params.reject!{|k,_|['controller', 'action'].include? k}
> params.permit(:name)
> SomeModel.new(params) #Exception, world is OK
> SomeModel.new(params.symbolize_keys) #No Exception, secret overwritten
>
This is a bug in `symbolize_keys`. It appears to have been fixed (accidentally?
on purpose?) on master:
https://github.com/rails/rails/commit/f1bad130d0c9bd77c94e43b696adca56c46a66aa
by starting the loop with `self.class.new` instead of `{}`.
There’s some additional future changes coming up in Ruby 2.2.0 with methods
like `reject`:
https://www.ruby-lang.org/en/news/2014/03/10/regression-of-hash-reject-in-ruby-2-1-1/
that seem likely to further complicate the situation.
—Matt Jones
signature.asc
Description: Message signed with OpenPGP using GPGMail
Re: [Rails-core] strong parameters safety issue enables accidental mass assignment
Generally speaking I believe developers should be careful/responsible for
handling what they are sending to their models for mass assignment, and
there's where strong params help. The ideal solution indeed would be for
Parameters not to inherit from Hash, which is something Rails will likely
be changing in the near future. This would avoid people calling methods
that are inherited from Hash in the Parameters object, such as
symbolize_keys. It won't avoid people actually converting the Parameters
object into a Hash with, say, a to_h/to_hash call, but at least that would
make it more explicit.
Thanks for sharing your thoughts on that.
On Wed, Aug 6, 2014 at 1:51 PM, wrote:
> Hello,
>
> Recently I was looking into upgrading one of our Rails 3.2 apps to use
> strong_parameters. I encountered what seems like a flaw to me and I would
> like
> to spark discussion about this, hoping for personal learning and potential
> improvement of the rails framework.
>
> The switch from protected attributes to strong parameters looks from the
> outside
> like access control for mass assignment moves from the model to the
> controllers
> (where, I agree, they belong). However looking at the implementation of
> strong
> parameters, it seems easy for developers to accidentally introduce bugs.
>
> How is that?
> @params in a controller is of type ActiveController::Parameters, which
> inherits
> from HashWithIndifferentAccess which in turn inherits from a Ruby Hash.
> Strong
> Parameters extends ActiveController::Parameters to have a 'permitted?'
> function,
> essentially acting as the whitelist of permitted parameters for mass
> assignment
> [1].
>
> Strong Parameters also overrides ActiveRecords sanitize_for_mass_assignment
> function, where it checks if the attributes respond to 'permitted?'. The
> implementation assumes that if the attributes Hash does not respond to
> 'permitted?' the mass assignment decision should be deferred to
> ActiveRecord. If
> it responds to 'permitted?' and the mass assigned attributes are not on the
> whitelist it will raise an exception [2].
> The 'permitted?' function here acts as a weird capability [3]. It is weird
> because a capability that is not present will usually deny an action (mass
> assignment) while here the action gets permitted in the absence of the
> capability.
>
> Why does that matter?
> It matters because it is possible for a developer to accidentally lose that
> capability accidentally very easily on the way from the controller (where
> permit happened and the capability gets created) to the model (where the
> capability gets used). This loss does happen silently and effectively
> disables
> mass assignment protection.
>
> How does that happen?
> The only class aware of the 'permitted?' capability is
> ActiveController::Parameters, if we call a method that is not aware of the
> capability it can get lost as a side effect:
>
> class SomeModel < ActiveRecord::Base
> #fields :name, :protected_secret_field
> include ActiveModel::ForbiddenAttributesProtection
> end
>
> #imagine a request w/ params = {'name' => 1, 'protected_secret_field' =>
> 2}:
> params.reject!{|k,_|['controller', 'action'].include? k}
> params.permit(:name)
> SomeModel.new(params) #Exception, world is OK
> SomeModel.new(params.symbolize_keys) #No Exception, secret overwritten
>
> symbolize_keys returns an object of type Hash, that no longer has the
> 'permitted?' function, so strong parameter protection is effectively
> disabled.
>
> Both of these patterns are found quite a bit in our codebases - often in a
> not
> such simple form though.
>
> In some sense the problem is, that there is non-framework code on the
> codepath between where the whitelist is defined and where it is used, and
> it is
> easy to accidentally lose the whitelist which disables mass assignment
> protection. This problem did not exist with protected attributes, since
> there
> was no codepath.
>
> Using 'attr_accessible :name' on the model would have prevented these
> problems,
> however I am under the impression that strong_parameters is to replace
> this old
> way.
>
> Am I the only one worried about this (I have not found any discussion
> online) or
> is this a known problem? If it is known, what is the proposed solution?
> I understand that developers need to take care when handling user input,
> and
> such should not call e.g. symbolize_keys. But the same is true for mass
> assignment vulnerabilities as a whole - in a world where programmers do
> not make
> mistakes mass assignment errors do not exist.
>
> A possible solution could be to not attach this capability onto the
> parameters
> Hash but store it separately and then retrieve it at mass assignment time
> - so
> it is not the developers responsibility to babysit it. In an alternative
> implementation 'permit/permit!' could be used to declare a list of mass
> assignable parameters for a given action in a controller (optionally on a
> per
> model basis, default empty list)
[Rails-core] strong parameters safety issue enables accidental mass assignment
Hello,
Recently I was looking into upgrading one of our Rails 3.2 apps to use
strong_parameters. I encountered what seems like a flaw to me and I would
like
to spark discussion about this, hoping for personal learning and potential
improvement of the rails framework.
The switch from protected attributes to strong parameters looks from the
outside
like access control for mass assignment moves from the model to the
controllers
(where, I agree, they belong). However looking at the implementation of
strong
parameters, it seems easy for developers to accidentally introduce bugs.
How is that?
@params in a controller is of type ActiveController::Parameters, which
inherits
from HashWithIndifferentAccess which in turn inherits from a Ruby Hash.
Strong
Parameters extends ActiveController::Parameters to have a 'permitted?'
function,
essentially acting as the whitelist of permitted parameters for mass
assignment
[1].
Strong Parameters also overrides ActiveRecords sanitize_for_mass_assignment
function, where it checks if the attributes respond to 'permitted?'. The
implementation assumes that if the attributes Hash does not respond to
'permitted?' the mass assignment decision should be deferred to
ActiveRecord. If
it responds to 'permitted?' and the mass assigned attributes are not on the
whitelist it will raise an exception [2].
The 'permitted?' function here acts as a weird capability [3]. It is weird
because a capability that is not present will usually deny an action (mass
assignment) while here the action gets permitted in the absence of the
capability.
Why does that matter?
It matters because it is possible for a developer to accidentally lose that
capability accidentally very easily on the way from the controller (where
permit happened and the capability gets created) to the model (where the
capability gets used). This loss does happen silently and effectively
disables
mass assignment protection.
How does that happen?
The only class aware of the 'permitted?' capability is
ActiveController::Parameters, if we call a method that is not aware of the
capability it can get lost as a side effect:
class SomeModel < ActiveRecord::Base
#fields :name, :protected_secret_field
include ActiveModel::ForbiddenAttributesProtection
end
#imagine a request w/ params = {'name' => 1, 'protected_secret_field' => 2}:
params.reject!{|k,_|['controller', 'action'].include? k}
params.permit(:name)
SomeModel.new(params) #Exception, world is OK
SomeModel.new(params.symbolize_keys) #No Exception, secret overwritten
symbolize_keys returns an object of type Hash, that no longer has the
'permitted?' function, so strong parameter protection is effectively
disabled.
Both of these patterns are found quite a bit in our codebases - often in a
not
such simple form though.
In some sense the problem is, that there is non-framework code on the
codepath between where the whitelist is defined and where it is used, and
it is
easy to accidentally lose the whitelist which disables mass assignment
protection. This problem did not exist with protected attributes, since
there
was no codepath.
Using 'attr_accessible :name' on the model would have prevented these
problems,
however I am under the impression that strong_parameters is to replace this
old
way.
Am I the only one worried about this (I have not found any discussion
online) or
is this a known problem? If it is known, what is the proposed solution?
I understand that developers need to take care when handling user input, and
such should not call e.g. symbolize_keys. But the same is true for mass
assignment vulnerabilities as a whole - in a world where programmers do not
make
mistakes mass assignment errors do not exist.
A possible solution could be to not attach this capability onto the
parameters
Hash but store it separately and then retrieve it at mass assignment time -
so
it is not the developers responsibility to babysit it. In an alternative
implementation 'permit/permit!' could be used to declare a list of mass
assignable parameters for a given action in a controller (optionally on a
per
model basis, default empty list) and store it on the request or as a thread
local variable. The mass_assignment part on the model would then read this
variable and decide if the mass assignment should be allowed or not. This
way
the capability/whitelist can not be accidentally lost by transforming the
parameters hash anymore.
best,
Johannes
[1]
https://github.com/rails/strong_parameters/blob/master/lib/action_controller/parameters.rb
[2]
https://github.com/rails/strong_parameters/blob/master/lib/active_model/forbidden_attributes_protection.rb
[3] http://en.wikipedia.org/wiki/Capability-based_security
--
You received this message because you are subscribed to the Google Groups "Ruby
on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rub
