strong parameters safety issue enables accidental mass assignment

rubyonrails-core
1

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

2

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.

3

[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

4

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] GitHub - appfolio/shields_up: This gem provides an alternative and improved implementation of strong_parameters.

5

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.

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