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.

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