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.
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 (
$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
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
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.
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
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.