Rails Mass Assignment Issue: A PHP Perspective

Now more than ever is the security of your rails application under close scrutiny. Github user homakov (Egor Homakov exposed a vulnerability of Rails (that no doubt also lives in an app near you) with this playful commit in the Rails project itself.. I fell in love with this commit purely for the meme fest that ensued. However, the reaction from the Rails community in general has left me a little surprised. Since the attack, there have been countless solutions posted on blogs ranging from trivial to DSL extravagance.

So What Happened?

In case you missed all the hoo-ha, the mass assignment vulnerability has lived in Rails or more specifically ActiveRecord since year dot. Every time you write code in a controller such as:

You have opened yourself up to the now almost infamous hack. Mass assignment is convenient, in fact it’s so convenient I shudder at the prospect of updating model attributes any other way via forms. I will be first to stand up and admit my shame (or lack of) in the fact I have written/generated code like this, and it’s still in production today.

My background is in PHP and most small scale applications I wrote were sans framework. I would reuse a database class that handled all the nuts and bolts of of reading and writing to the databse, then I simply inherited my model code from that. Sound familiar? Now all that reading and writing was handled via PHP Data Objects (PDO). Because of my experience with PDO, this article should really be called “Mass assignment vulnerability in Rails and how PHP PDO made sure I was never caught out like Github”, but that is verging on the controversial let alone ridiculous.

PDO 101

For those unaware of PDO, it is a layer of abstraction between my code and the database. Much like ActiveRecord regardless of what database I end up using, by implementing my data access via PDO I can shift between SQLite, MySQL, PostgreSQL etc. One of the fundamental components of using PDO is prepared statements. A prepared statement looks something like:

What we have here is a snippet of a function that accepts two parameters ($some_variable and $another_variable), these are subsequently escaped and injected into our SQL statement.

In Rails, or more specifically ActiveRecord (even more specifically ActiveModel) this is probably the equivalent to the silver bullet that is attr_accessible. Much like attr_accessible in the above prepared statement, we have essentially white listed what parameters will be used with our SQL statement.

The same protection in a Rails application would look like:

class SomeModel < ActiveRecord::Base attr_accessible :first_name, :surname end 

In both PHP and Rails versions, if the model SomeModel had an is_admin attribute, it would be protected from mass assignment. The only real difference, and it’s significant, is the far fewer lines of code we actually have to type. Yes, Rails is a framework/eco-system, so that will be the case. I just wished for a moment all that PDO code when I wrote it many moons ago was as terse.

The point is, in relation to the mass assignment vulnerability, it would never have happened using rudimentary, simple, data access as we see in the PHP prepared statement. Is there an argument that the ease of ActiveRecord has bred complacency? My opinion is that for novice rubyists, certainly (although they will be blissfully unaware of the fact) and for the more experienced among us, probably. However, I’ll let you be the real judge of that proposition.

All Done, Not Quite

Up to this point, all that has been discussed is the mechanics of placing data into the database. Is that where the smoking gun for this vulnerability lies? We know how to prevent it now, we always have known, it’s even commented in every recent Rails application we create. In our config/application.rb file we have the commented out line :

# config.active_record.whitelist_attributes = true 

By uncommenting that one line, we are safe, phew. But the application is probably going to break. It is at this point we see the real culprits, and they don’t live in models. There has been a frenzy in creating new, almost elaborate techniques to protect from mass assignment using attr_accessible or kissing cousin attr_protected, while the real offenders are still at large. The following code, in controllers, is convenient but wrong .

There are two offending items here. First of all, by using update_attributes or save in a controller breaks the first law of SOLID code, single responsibility. But hey, it’s Rails we do this kind of stuff every day without even a nod to the SOLID principles. We would only extract this kind of thing to the model anyway.

Now we are at my main gripe with this code snippet. The parameters, passing params[:whatever] directly to a method on a model is a real shoddy piece of work. It’s lazy and reckless. We are all expecting ActiveRecord to do the hard work for us and just work while we happily pass it external data.

Murder She Wrote

First of all, I’m more a Columbo (early stuff) kind of guy, but I’m sure pretty much everyone reading this will be thinking we have already covered a sufficient remedy for accepting the params hash into a model in the form of attr_accessible. Yet I have never used attr_accessible in a single application. I know a couple of ‘quick, dirty’ apps are still susceptible to mass assignment attacks but there are a few which I have secured, without using attr_accessible or attr_protected.

Remember I mentioned the article title should be along the lines of how my experience with PDO would protect me from mass assignment attacks in Rails? Well, back in the day I yearned for mass assignment, binding each parameter for a PDO prepared statement is tedious at best. So I wrote a helper method.

The execute method for prepared statements accepts an optional associative array (hash if you do ruby), thus I could get the convenience of mass assignment passing the result of build_params to the data access class.

Of course, this was not without it’s flaws. I am back in the position of being open to mass assignment attacks, and every element (submit buttons etc. anything in the form that had a named attribute) was being passed to the prepared statement. The fix was easy enough.

I have protection again from mass assignment and, more importantly, the controller is responsible for that protection. When it came to learning and using rails I took this simple idiom with me.

I was going to post my example code in Ruby (basically a straight port of the PHP) then I noticed DHH posted a gist with the same functionality but nicer, using Array#slice.

It really becomes a matter of taste, belief of where the logic for mass assignment protection lies. Naturally, I would never frown upon someone using attr_accessible. In fact, I like the simplicity of white listing all attributes in the model itself. But always in the back of my mind I hear the voice of Jim Weirich reminding me about SOLID principles and the single responsibility that is often lost in Rails for convenience. I am of the feeling a model should not be responsible for the data it is passed. If controller is the “glue” between our models and the views, surely that is where the responsibility lies?

Of course, I could not finish this article without pointing out some great resources for security, specifically with Rails. Feel free to add any more in the comments.

Win an Annual Membership to Learnable,

SitePoint's Learning Platform

  • http://szeryf.wordpress.com/ szeryf

    In your PHP examples, whatever you put in the params, you’re still on the safe side, because your INSERT statement is hard-coded. There’s no way for the attacker to inject other attributes, which is the core of Rails’ mass assignment issue. If you were building the INSERT statement dynamically, then of course you could be vulnerable and use of white list would be necessary.

    Also, I found interesting info in one of the comments to DHH’s gist about “:as => role” that you can use to define access level for mass assignment. Too bad it’s so hard to find any docs about it.

    • http://bangline.co.uk Dave Kennedy

      Hey szeryf,

      Yes, you are quite right continuing to use a the example prepared statement will always protect from mass assignment, sorry it was just not what I was driving at so didn’t want to get into a PHP tutorial. Back in the day the PHP db class I use built up the prepared statement and bound parameters dynamically… then I started using Doctrine ORM :)

      RE the `:as => role` do you mean access to state in the controller? In the gist he does show an example using a `current_user` helper.

  • Pixoo

    Hi, I do not agree with the idea that the controller should be responsible for the data filtering.

    You may find yourself creating/updating your models from other parts of your app like delayed tasks, rake tasks, observers, etc.

    By using in-model filtering, you keep the protection rules in one place.

    An application may not always be a simple CRUD, even when you’re doing RESTful stuff.

    That’s my opinion and I respect yours.

    • http://bangline.co.uk Dave Kennedy

      Hey Pixoo,

      Thanks for taking the time to read and its cool to agree to disagree, and of course not everything needs to be a simple CRUD (except article examples of course) but the above method works just as well for more complex forms (nested models) as well.

      C’mon give it a try.

    • Nick

      Completely agree here, seems like common sense to me. There are many situations when you may use a model elsewhere, or include it within another controller… or even app! You shouldn’t have to re-create the filtering over and over every time. That’s just code replication and doesn’t make sense.

    • Thomas

      The controller is not such a bad place and even better than the model itself for defining the filtering of the user input.

      It is not the responsibility of the model to filter such an input, since from the point of view of the model, there is nothing wrong about assigning values to the properties it exposes.

      The error is in params: it contains fields, it should not contain. It is the job of the controller to assign only those values to the model, which it expects from the request. What it expects, depends on the controller and not the model and can vary for the same model for different types of requests.

      It is true, that the model offers a central place for such code, but this is not a good reason and you can easily find another central place too (in a module).

  • http://bangline.co.uk Dave Kennedy
  • http://wilmoore.com Wil Moore III

    Glad to see this being talked about. Yes, PDO does a lot for us; however, it is always good to be educated on what what it is exactly that we are being protected from so we don’t inadvertently cause ourselves to be vulnerable.

    BTW, the build_params method could also be expressed like so:

    function filter_params(array $params = [], array $whitelist = []) {
    return array_intersect_key($params, array_flip($whitelist));
    }

    SEE: https://gist.github.com/2140151

    The original version is more explicit due to prepending the ‘:’ (colon) to the key, which actually isn’t necessary for PDO (or anything build on top of PDO such as Zend DB, Doctrine DBAL).

    Further, the terse version is much easier to reason about which makes it less prone to be incorrect and mitigates the need for other team members to attempt to simplify (which opens the door to a regression).

    • http://bangline.co.uk Dave Kennedy

      Wil, I like your version better as well

  • Dev

    This software is written for the web. As such, there is no guarantee that we will get what we expect.

    Consider how web browsers behave. They can take malformed HTML and still render a page in many cases. The browser expects malformed data. However, when I write HTML, I make sure it is valid. That is because I have no idea how the browser will handle malformed HTML.

    Similarly here, it makes sense to have a model validate input. The model only cares about certain attributes, which is part of the model’s functionality. The controller doesn’t want the model to do anything unexpected. If the controller wants a model to update only specific fields, then the controller should be specific.

    Checking in multiple places is redundant, but maybe in this case redundancy is good? It is the approach: “Never have a single point of failure.”