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.

@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