security.md

Proposal for Improving Mass Assignment

For a while, I have felt that the following is the correct way to improve the mass assignment problem without increasing the burden on new users. Now that the problem with the Rails default has been brought up again, it's a good time to revisit it.

Sign Allowed Fields

When creating a form with form_for, include a signed token including all of the fields that were created at form creation time. Only these fields are allowed.

To allow new known fields to be added via JS, we could add:

<%= f.allowed_fields "foo", "bar", "baz" %>

Mark accessible fields in the controller

The first strategy will not full satisfy apps that support a lot of HTTP requests that do not come from forms generated by Rails.

Because accessible fields is usually a function of authorization, and is not global, we should move allowed fields into the controller. The basic idea is:

class PostsController < ApplicationController
  # attributes can be marked accessible for the entire controller
  attr_accessible :foo, :bar

  def create
    # mass assignment can also be done on an instance basis
    # this can be used to override the class defaults
    attr_accessible(true) if user.admin?
    ...
  end
end

I would imagine that Rails authorization frameworks like CanCan could add sugar to make this even easier in common cases.

Conclusion

The core problem with Rails mass assignment is that attribute protection is an authorization concern. By moving it to the controller, we can have smart defaults (like signed fields in form_for) and in more advanced cases, make it easier to decide what fields are allowed on a per-user basis.

By moving it into the correct place, we will probably find other nice abstractions that we can use over time to make nice defaults for users without compromising security.

@turlockmike CACAN 2.0 is implementing what you suggest.

Resource Attributes

It is possible to define permissions on specific resource attributes. For example, if you want to allow a user to only update the name and priority of a project, pass that as the third argument to can.

can :update, :projects, [:name, :priority]

If you use this in combination with load_and_authorize_resource it will ensure that only those two attributes exist in params[:project] when updating the project. If you do this everywhere it will not be necessary to use attr_accessible in your models.

You can combine this with a hash of conditions. For example, here the user can only update the price if the product isn't discontinued.

can :update, :products, :price, :discontinued => false

You can check permissions on specific attributes to determine what to show in the form.

<%= f.text_field :name if can? :update, @project, :name %>

@bradphelan Excellent. That's great news.

Awesome!!

Just to add my 2 cents here.. I don't mind the idea of moving mass-assignment protection to the controllers (or even to a Django-style Form object). I don't think it's necessary, but I don't mind, it won't be much of a learning curve (and only a very minor inconvenience) if that happens.

I don't like the idea of defining the allowed attributes as a hidden form field, signed or not. Sure, it sounds like a great idea of you're using server-rendered ERB, Haml or Slim templates with the form_for helper.. but what happens if you're doing your front-end in Backbone or Ember and using Handlebars templates? How are we passing that hidden form field to the front-end templates that are rendered by Javascript in the browser and not by Ruby on the server? Are we just ignoring them in that case? If so, it defeats the purpose of having them in the first place. Considering that more and more apps are moving to a frontend Javascript framework, Rails really needs to continue to support both use-cases.. everything rendered on the server, or everything passed back and forth as JSON and rendered in the browser (and even the possible third use-case, mixing those two strategies).

@stevenh512 Even if I subscribe to most of what you said, beware of the argument of "don't put magic in views", here, as it can lead to consider all helpers as a annoyance. I love using mustache to share templates between server side and client side, too, but if mustache can't handle ruby helpers, that's a mustache problem (not that I think it should, for obvious client side reasons).

The solution, here, is usually to use a presenter or mvtc pattern, which call helpers and is used to compute variables for either the template, the json response or whatever you need. The hidden inputs could be implemented as a #allowed_fields_tags( *fields ), which would be called by #form_for, or directly in a view/presenter class.

I'm not arguing against putting "magic" in views, although I'd have to say that most existing Rails view helpers can be fairly easily duplicated with Ember views and Handlebars helpers without passing any extra information (like the allowed fields) back and forth in the JSON. What I'm arguing is that we shouldn't be putting "unnecessary magic" in the views when the logic belongs to the controllers and/or the models. The app on the server, whether the logic is in the model or the controller (or both), is more than capable of deciding what is and isn't allowed without some magic hidden form field or extra data being passed back and forth in the JSON.

The code is already there and has been for years, it's just a matter of people actually using it. It should be turned on by default. To use a real-world analogy here, how many carjackings have been prevented since the major auto manufacturers started making new cars do one simple thing that drivers have been unwilling to do on their own for years even though they had to know it was a security concern (locking the doors while the car is in gear)? The power door locks have been there for a long time, it's just a matter of wiring them up so they lock by default.

While mass-assignment is definitely a security issue, it's not the same kind of issue as XSRF where you actually do need some magic value to be passed back and forth (and even then, it's not sent to the client in the JSON, there's a meta tag for that.. but the same solution wouldn't work here).

With Rails 3.1+ scoped attr_accessible, I honestly don't see why the logic even needs to be moved from models to controllers.. with the code as it is now you're more than capable of handling mass-assignment at the authorization level in your controllers while keeping attr_accessible in the model (and keeping it DRY, which I thought was supposed to be a Rails convention)... but even moving it to the controllers makes a lot more sense to me than adding unnecessary magic to the views that does nothing but complicate things for anyone who needs to send data to the server in any way other than through a form generated with the form_for helper. Let's not just think about Javascript frontends here.. what if you want to build an app with an API? Are you going to expect everyone who consumes your API to handle this magic value that's really of no use to them (which means at least one extra API call just to get the magic value before they can submit any data.. which means more load on your server and theirs) when your model or your controller can and should be taking care of it? So what about those APIs? Do we just turn this off, like people typically do with the XSRF protection in that situation? I think turning this off for API calls without some other layer of protection the model and/or controller would be much more dangerous than turning off the XSRF protection is in the same situation.. and again, if the model or the controller can do it already why do we need this magic (and if they can't, then this magic is clearly the wrong answer anyway)?

What I really think needs to be done, and it actually needed to be done a long time ago (how long has it been known now that mass-assignment is a security issue? since before Rails 1?) is to actually enforce whitelisting by default, which a recent Rails commit at least takes a step toward doing even if I don't think they went far enough with it.

I can see two trends here: one repelling that all validation goes on the model, for those who want to make it dependent on the context, and one repelling that all params sanitation goes on the controller, for those who want to make it dependent on the internals of the business logic. The solution here offered has been a new layer taking care of this issues, á la Dyango Forms, but this could lead to view logic messed up with everything else, this layer becoming a kind of glue between everything.

I indeed support the introduction of a new layer in Rails (Conductor/Presenter/Mediator/Decorator/Template), but adding a new perspective. I advocate a ResourceAccessor, which not only stays between a model and its controller, but besides incarnates the concept of Resource (which can be understood as made of parts of one or several related models). This resource should relate each attribute to its respective model's field and should make validations depending on the user's authorization level. It would be called by the controller on GET actions and passed onto the view, which could automatically rely on the ResourceAccessor's field definitions to display the appropiate type of field (either for scaffolded display or in forms). It would receive the params from the controller, sanitize and handle them out to the respective save methods of each model, usually inside a transaction. Mass assignment would become impossible, since only explicitly defined (and eventually authorized) fields could be set.

This approach solves said issues of mass assignment, bloated models, and repetitive view declarations.
It also integrates the new Rails 4.0 verb PATCH and with the new meaning of PUT together, giving full sense to the creation of a method replace_resource, which could be meaningfully applied to such ResourceAccessor instead of the current update_attributes whenever a full replace is meant (see my take on that).

Feel free to elaborate on the concept. Just my two cents.

Kind of this idea by @alexeymuranov, but extending it so that the accessor can reach more than a single model out. It does not have to break backwards compatibilty, since you can always call directly a model, but you'd be encouraged not to, because the code would be much more simple, beautiful and testeable.

And nested parameters get included for free, since you can include the children inside the ResourceAccessor itself, or call their respective ResourceAccessor to take care of everything related to them.

@miguelsan I like that approach. I don't necessarily disagree with anyone who thinks it belongs in the controller or anyone who thinks it belongs in the model, either of those is fine with me (I just don't think it belongs in the view when it's backend logic 😁). Something like this should satisfy the needs of anyone arguing either side of that question.

That's what I think as well. Both sides have their right, so the solution must be obviously in the middle. There is no better option than adding a full fledged layer before the model providing security, simpler views and separation of concerns. The biggest advantage being the decoupling between the concepts of Backend Model and REST Resource, for which there is no solution provided until now. Actually, many developers aren't even aware of the difference.

But, are the almighty Rails' gurus going to dare adding yet another layer, one which almost throws the holy MVC acronym on the ground? How long will they defer it? Til Rails15? I wish not.

@miguelsan The problem is that there are three different things being discussed here.

1. Defining which attributes are accessible in general
2. Deciding where to put rules concerning the logic of role based assignments for attributes
3. Easily displaying different views for forms based on role based assignments of attributes

All of these problems are different, but highly related.

One way to solve this problem is to add another layer, but to keep with the MVC framework the simplest solution is:

1. define private attributes in the model (application logic)
2. define attributes which can only be written based on roles in the controller (business logic)
3. Any # of solutions for the views ranging from multiple views, to helper methods which take into account roles.

The only other way to do this would be to add another layer, but it seems like the information to do this already exists.

@miguelsan I like this concept. Have you ever tried an implementation of it as a proof of concept ?

Oh, and for the rails core integration, no need to wait, rails is flexible enough to let you implement it as a third-party gem ;)

Wouldn't a fourth item be validations? It may be that you want to control which validations are run, based on the attributes expected to be received.

@tulockmike I think the conversation about attributes is missing the abstraction. We really only have three things going on

1. Authentication ( Who you are )
2. Authorization. ( Are you allowed to affect some state change which for clarity should be named )

3. Validation. ( Is the current state of some object valid )
   

Authentication happens in the middleware. It is a global state.
Authorization happens on specific controller actions. CANCAN makes this clear

def update
    authorize! :update, @some_model_instance
end

Validation happens after a state change has been affected. It assumes the state change is authorized and is a sanity
check on the object fields.

However it is clear that in many cases it is not enough to just authorize! update, @some_model_instance and
then blindly apply update_attributes. An example makes it clear. Say we have the following model.

class User < ActiveRecord::Base
    has_many :images
end

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title
end

We have two contexts in which we may wish to update an image object.

* change the owner
* change the title

Both of these actions can be done through update_attributes passing in the params
hash. Easy to do. However it is evil. What you really should have on the object
are methods to perform specific state changes.

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    def change_owner params
        update_attributes params.slice(:owner_id)
    end

    def change_title
        update_attributes params.slice(:title)
    end
end

Now our controller would have to look like

class ImagesController < ApplicationController

    load_resource

    def change_owner
        authorize! :change_owner, @image
        @image.change_owner params[:image]
    end     

    def change_title
        authorize! :change_title, @image
        @image.change_title params[:image]
    end

end

So now we have correct separation of concerns. The model is fully responsible for defining it's state changes and the api for those state changes. The controller is fully responsible for deciding who is allowed to invoke the state change.

Of couse this results in our controllers slipping away from the standard

index
delete
update
show
destroy

suite of methods. In theory you could create a set of nested controllers to implement the above image controller and in fact it probably is the right thing to do. Imagine a web form to change the ownership of the image

class ChangeOwner < ApplicationController

    # Nested under  /images/:id/change_owner/new
    def new
        authorize! :change_owner, @image 
        render :new
    end

    def create
        authorize! :change_owner, @image
        @image.change_owner params[:image]
    end
end

Again there are no attribute authorizations made in the controller only authorizations made on specific state changes which invoke methods on the model. To make this kind of thing easier on the developer I would suggest method generation helpers on the models to change

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    def change_owner params
        update_attributes params.slice(:owner_id)
    end

    def change_title params
        update_attributes params.slice(:title)
    end
end

to

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    state_change :change_owner, :owner_id do params
        update_attributes params
    end

    state_change :change_title, :title do params
        update_attributes params
    end
end

which could be compressed in default as

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    state_change :change_owner, :owner_id 
    state_change :change_title, :title
end

but then again too much magic got us where we are today.

@bradphelan sorry, guy, after all the work of yours writing that comment, it can all be chopped down to:

class ChangeOwner < ApplicationController
    # ...
    def create
        authorize! :change_owner, @image
        @image.update_attribute :owner, params[:image][:owner_id]
    end
end

Where CanCan helps you like this:

can :change_owner, :images, :attributes => :owner_id

There is no need for this at all:

state_change :change_owner, :owner_id 
state_change :change_title, :title

I mean, that can be already done today. But we hate having to write a different controller and authorization rule for each attribute, that's why we resort to update_attributes. So, you are right in that you are proposing best practices, but the result is long and boring, and Rails should provide a way to make it short and beautiful, I guess.

My vote goes for a new layer that underlines the difference between model and resource, also allowing for multiple models exposed as a single resource. And no, @oelmekki, I haven't written any code yet. I just got the idea yesterday, but I also wouldn't have the time, sorry.

@migueisan +5 for funny but I didn't mean to confuse you with the DSL magic and make you not notice that the example was deliberately degenerate.

My main issue is that too much responsibility is given to the update method when, with a complex model (which my example is not ) you should be calling object methods with a tightly defined parameter list to effect state change and not be using update_attributes with role based attr_accessible Just reading DHH's preferred method at http://rubydoc.info/gems/strong_parameters/0.1.3/frames and it makes sense with my suggested approach except I would move the strong parameter signatures into the model under explicit method names.

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    def change_owner params
        update_attributes params.require(:image).permit(:owner_id)
    end

    def change_title params
         update_attributes params.require(:image).permit(:title)
    end
end

and in CANCAN

can :change_owner, Image if user.has_role? :admin
can :change_title, Image if user.has_role? :user

CANCAN enabled controller reduces to

class ImageController < ApplicationController
    load_and_authorize_resource

    def change_title
        @image.change_title params
    end

    def change_owner
        @image.change_owner params
    end
end

@wycats your idea to set the allowed attributes in the form is very good!
So I've written a gem https://github.com/webhoernchen/massassignment_security_form for our rails project.
It extends the form_helpers, adds the allowed attributes to the form and removes all other values of the params hash!

I am actually a security engineer that happens to use rails a lot. I can assure your that setting the allowed fields in the form is terrible from a security perspective. You can read why 50 comments ago. Just to reiterate, the protection mechanism as untrusted data in the attacker's control is a recipe for failure. Developers rarely get crypto right. Why would you think this case is any different?

…

On Apr 2, 2012, at 4:15 AM, Christian ***@***.*** wrote: @wycats your idea to set the allowed attributes in the form is very good! So I've written a gem webhoernchen/massassignment_security_form for our rails project. It extends the form_helpers, adds the allowed attributes to the form and removes all other values of the params hash! --- Reply to this email directly or view it on GitHub: https://gist.github.com/1974187

A failure mode with your form signing is the following scenario. I have an object X that I have rights to update all fields for. I save the encrypted hash for that form and then apply it against an object I only have partial rights for. I am now have elevated my privilege level. I'm pretty sure your code doesn't deal with this problem after a quick review. Though its probably possible to fix for this scenario it is kind of scary to do this sort of thing. On Apr 2, 2012 2:31 PM, "Dominique Brezinski" < [email protected]> wrote:

…

I am actually a security engineer that happens to use rails a lot. I can assure your that setting the allowed fields in the form is terrible from a security perspective. You can read why 50 comments ago. Just to reiterate, the protection mechanism as untrusted data in the attacker's control is a recipe for failure. Developers rarely get crypto right. Why would you think this case is any different? On Apr 2, 2012, at 4:15 AM, Christian ***@***.*** wrote: > @wycats your idea to set the allowed attributes in the form is very good! > So I've written a gem webhoernchen/massassignment_security_form for our > rails project. > It extends the form_helpers, adds the allowed attributes to the form and > removes all other values of the params hash! > --- > > Reply to this email directly or view it on GitHub: > https://gist.github.com/1974187 --- Reply to this email directly or view it on GitHub: https://gist.github.com/1974187

@dominiquebrezinski: The allowed attributes will be encrypted in the form automatically and not allowed attributes in the controller removed.
You can find a description in the README:

In your erb file:

<% form_tag({:action => "update"}) do %>
  <table>
    <tr>
      <td><label for="user_first_name">First name:</label></td>
      <td><%= text_field :user, :first_name %></td>
    </tr>
    <tr>
      <td><label for="user_name">Name:</label></td>
      <td><%= text_field :user, :name %></td>
    </tr>
  </table>
<% end %>

It creates the following html form:

<form method="post" action="/route/to/users/update">
  <table>
    <tr>
      <td><label for="user_first_name">First name:</label></td>
      <td><input type="text" value="Christian" size="30" name="user[first_name]" id="user_first_name"></td>
    </tr>
    <tr>
      <td><label for="user_name">Name:</label></td>
      <td><input type="text" value="Eichhorn" size="30" name="user[name]" id="user_name"></td>
    </tr>
  </table>

  <input type="hidden" value="EncryptedHashWithFormFieldsForUser" name="massassignment_fields">
</form>

The controller can parse the "EncryptedHashWithFormFieldsForUser" to "{'user' => ['first_name', 'name']}".
All other attributes of the params[:user] will be remoevd!

if I read encrypted forms I immediately need to think about: "what about rest clients talking to a restful rails server ? how do they protect their models ? do I need to implement some kind of encryption on all my rest clients as well ?"

@bradphelan with your approach you'd need two new methods (one for model and one for controller) and an entry under abilities.rb for each attribute. That's far from clean and dry code. Can't buy it.

@Miguesan I am suggesting validating actions on models based on the abstract name of the action occurring not on the individual attributes being set. Form signing is at the wrong level of abstraction.

There is nothing wrong with the role based attr_accessible method currently in rails except that it defaults to permissive, which has now been fixed. If you want to have a role based mapping to individual attributes, there is nothing really wrong with that.

def update
    authorize :update, @model
    params = params_for_role
    @model.update_attributes params
end

def params_for_role
    case current_user.role
    when :admin
        params.require(:model).permit :x, :y, :z
    when :user
        params.require(:model).permit :x
    default:
        raise "arggh!"
    end
end

and have a blanket

can :update, Model

the other way to achieve the same thing is

controller

def do_admin_stuff
    authorize :do_fancy_stuff, @model
    @model.do_fancy_stuff params.require(:model)
end

def do_user_stuff
    authorize :do_simple_stuff, @model
    @model.do_simple_stuff params.require(:model)
end

model

class Model < ActiveRecord::Base
    def do_simple_stuff
        update_attributes params.permit(:x)
    end

    def do_fancy_stuff
        update_attributes params.permit(:x, :y, :z)
    end
end

ability.rb

# Can do fancy stuff if admin or own the object
can :do_fancy_stuff, Model if user.role? :admin
can :do_fancy_stuff :user_id => user.id

# Can do simple stuff to a model if a user
can :do_simple_stuff if user.role? :user

Note in this case there are no attributes authorizations in CANCAN. Valid attributes
are defined only once as valid argument lists to model methods. This is a very simple
way to achieve this new layer. No need to over-engineer a solution.

@bradphelan You could say that Rails is overengineered when compared to Sinatra. Of course there are simpler cases where any solution seems feels like "too much". And those simple cases are actually the mayority. That's why a new layer must be able to seem transparent in its "default" mode and at the same time offer powerful features.

You are not dealing, for instance, with the problem of attributes from nested models (as many have noted). Or, as I said, with different models acting as a single resource for the end user. There is where a not overengineered solution comes into play.

I agree with @dominiquebrezinski that storing an encrypted version of the allowed fields on the client is probably not a good idea. It reminds me of ASP.NET where application state was also stored on the client. Practical Padding Oracle Attacks, developed based on a theory by Vaudenay completely devastated the security there.

Within the application, we know as a fact exactly what fields are allowed. Sending this information on a round trip to the client and back would only weaken what was already established as a hard fact. On an abstract level, cryptography would be used trying to prove something to ourselves that we already knew without it in the first place.

Note that this is very different from signed authentication tokens - there it's actually the user trying to prove something (their identity) to us (the application). In the "encrypted/signed fields" case it's just us trying to prove something to ourselves. In addition, the crypto would put more computational effort on the server and more data would be sent over the wire for each request/response, state would have to be tracked and, as we all know, it's very easy to make mistakes. For example when dealing with MACs, it can be very easy to make yourself vulnerable to timing attacks. If the fields are only encrypted and neither MACed nor signed, then we're basically screwed due to the malleability of ciphertexts. Then there's replay attacks and whatnot else.

But even if everything is done right, exposing ciphertexts like this to the client offers the possibility for brute-forcing your way in. There is no restriction on how many ciphertexts an attacker could obtain from the application - they would behave like ordinary users. A really sophisticated solution that would exploit volume and parallelism would certainly have a chance to brute-force keys within an acceptable time frame.

The particular encryptor used in this suggestion does appear to be unverified and susceptible to chosen plaintext attacks. Not only that, but it will generate an uncaught exception in OpenSSL's block cipher decrypt routine when ciphertexts are tampered with; it thus has a padding oracle.

Rails includes a well-reviewed message encryptor in ActiveSupport::MessageEncryptor; MessageEncryptor#sign_and_encrypt includes an HMAC-SHA1 signature on the ciphertext, which defeats these attacks. typing AES into your Rails app (or extension) is indeed a code smell.

Agreed. Im having some trouble about this.

# Attr :name is protected in model, but in some cases
# would be easy to access by this way:
Tag.create(name: 'foo')

# Actually, I needed to do this:
tag = Tag.new
tag.name = 'foo'
tag.save