Why slicing the params hash poses a security issue on mass-assignment?

Question

The official way of preventing security risks with mass-assignment is using attr_accessible. However, some programmers feel this is not a job for the model (or at least not only for the model). The simplest way of doing it in a controller is slicing the params hash:

@user = User.update_attributes(params[:user].slice(:name))

However the documentation states:

Note that using Hash#except or Hash#slice in place of attr_accessible to sanitize attributes won’t provide sufficient protection.

Why is that? Why a whitelist-slicing of params does not provide enough protection?

UPDATE: Rails 4.0 will ship strong-parameters, a refined slicing of parameters, so I guess the whole slicing thing was not so bad after all.

Inception

Well for starters it's just an inconvenience. With attr_accesible you can use :name in your model if you need to (albeit it without saving it), but if you .slice it off the params hash you can't do that. It's also much more semantic to use attr_accesible because it tells others the properties relationship with the model, whereas slicing it is much more cryptic. – Voidance

@Alex: I understand that attr_accessible is a convenient way of managing mass-assignment. Ok, but what's the security hole of using params[:xyz].slice? – Inception

For the record, attr_accessible now states "Note that using Hash#except or Hash#slice in place of attr_accessible to sanitize attributes provides basically the same functionality, but it makes a bit tricky to deal with nested attributes." Also, see the Edge API for advance Rails 4 docs, and see strong_parameters plugin for what to use prior to Rails 4. – Pankey

Answer 1

The problem with slice and except in controller might occur in combination with accept_nested_attributes_for in your model. If you use nested attributes, you would need to slice parameters on all places, where you update them in controller, which isn't always the easiest task, especially with deeply nested scenarios. With using attr_accesible you don't have this problem.

Bartholomeo

ah, nested attributes, it would be indeed an unpleasant work to filter the params in this case. – Inception

Answer 2

As of Rails 4, slicing the parameters will be the preferred method of dealing with mass assignment security. The Rails core team has already developed a plugin to deal with this now, and they are working on integrating support for nested attributes and signed forms. Definitely something to check out: http://weblog.rubyonrails.org/2012/3/21/strong-parameters/

Conscionable

"The basic idea is to move mass-assignment protection out of the model and into the controller where it belongs.". Nice. – Inception

Answer 3

Interesting gist from DHH on slicing in controller vs whitelisting alone:

https://gist.github.com/1975644

class PostsController < ActionController::Base
  def create
    Post.create(post_params)
  end

  def update
    Post.find(params[:id]).update_attributes!(post_params)
  end

  private
    def post_params
      params[:post].slice(:title, :content)
    end
end

Comment reinforcing the need to manage this within the controller:

https://gist.github.com/1975644#gistcomment-88369

I personally apply both - attr_accessible with slice to ensure nothing unexpected gets through. Never rely on blacklisting alone!

Bolyard

Answer 4

Just removing the :name from the params hash works to prevent setting that attribute for that action. It works only for the actions you remember protecting.

However, this practice doesn't protect you from abuse using all the methods automatically added for associations.

class User < ActiveRecord::Base
  has_many :comments
end

will leave you vulnerable for someone setting the comments_ids attribute, even when you delete the comments attribute from params.

Since there are quite a lot of methods added for associations, and since they might change in the future, the best practice is to protect your attributes on the model using attr_accessible. This will stop these kind of attacks most effectively.

Conscionable

Hash#slice is used for whitelisting: {:comments_ids => [1,2,3], :name => 'hello'}.slice(:name) #=> {:name=>"hello"}. I really don't see what could possible go wrong with it (dangerous phrase :-)) – Inception

Anyway, as I said, I understand why attr_accessible is a good way to handle mass-assignment security. But I was curious what was wrong with a slice, what extra security provides attr_accessible? – Inception

attr_accessible protects your model in every action, all the time, without you having to remember putting it in each controller. – Conscionable

I appreciate your answer, all you wrote it's true, but I am not sure that the docs saying "it won’t provide sufficient protection" means "it's not secure because you may forgive to put it". – Inception

I'm with @Inception here - can't understand how Hash#slice could be less secure than attr_accessible. And if you forgot about Hash#slice... well, then forgetting about Hash#slice is insecure, and not the Hash#slice itself. – Spradlin

Hash#slice would be used to keep specific params, not delete them, so your example doesn't really work – Foliolate

As of Rails 4, slicing the parameters will become the preferred method to deal with this. weblog.rubyonrails.org/2012/3/21/strong-parameters – Conscionable

Answer 5

@tokland your last comment is not correct to some extend. Unless your website has the browser as the only entry point where data comes in and goes out.

If your webapp has an API or communicates with other API's protection on the controller level leaves holes behind it and all data from other sources is not sanitised or checked. I recommend keeping the things as they are, turning on mass-assignment protection in application.rb and advancing ActiveSupport FormHelpers to work like Django/Python style.

Diagenesis

which one of my comments? I wrote a lot of them in this page :-) Anyway, I don't see the diferrence between having an API or a browser form. Of course the whole point is to avoid the user updating attributes he is not supposed to by tampering attributes. – Inception

@Inception The one where you say "The basic idea is to move mass-assignment protection out of the model and into the controller where it belongs." – Diagenesis

It's a quote from the linked article. But I agree, now I see this as a task for both the model and the controller. – Inception