C5d2ca4998259d200a48e38847d19f06

Gavin Morrice is a web and iOS developer from Edinburgh, Scotland.

more about me »

Blog Archive

My Suggestion for Dealing With Rails's Inherent MAA Vulnerability

If you didn't catch all of the action on Github last week - where have you been?

If you need a catchup, you can read more about the "attack" on GitHub here, and an interesting response to how Github handled it here.

In this post, I'd like to share my opinion on Rail's inherent vulnerability to mass-assignment attacks and suggest a solution which should be applied to Rails to help make Rails more secure for all of us.

Who's job is it anyway?

First up, I don't think this is a problem with Rails; I think this is a problem with Rails developers!

Rails is sometimes too convenient for us. Because Rails handles so much behind the scenes we can often miss out on some of the essential responsibilities we have when writing software.

While others have suggested solutions for improving the mass-assignment situation in Rails (including Yehuda Katz and Technoweenie), I don't agree that we need to change how we assign attributes from query parameters. The current Rails approach was a perfectly viable solution. The problem is that not everybody was using it!

Any n00b who's watched a few of Ryan Bates's (fantastic) Railscasts knows that sensitive attributes should be protected from mass-assignment. Yet from experience working with my own code and other seasoned developers, it's a point we often miss (along with applying constraints to database columns - another bugbear for a separate post).

In short, the solution to this problem is not to dramatically change Rails. The solution is to notify developers while they're using Rails.

My Proposal for Improving Mass Assignment in Rails

Rails's current attr_accessible and attr_protected methods offer perfectly adequate protection against mass assignment when creating or updating records from the params hash. The real problem lies in Rails developers overlooking their importance in the architecture of the system.

Every ActiveRecord::Base subclass is mass-assignment vulnerable by default.

What if we were to add this as an attribute to each ActiveRecord::Base subclass?

class User < ActiveRecord::Base

  self.mass_assignment_safe? # => false

end

Each time save is called on an instance of User, a warning is printed to the log to alert the developer that they haven't protected the model from mass-assignment attacks. Something along the lines of

@user.save

would log:

[Warning] User is not protected against mass-assignment attacks. Please use either attr_protected or attr_accessible.

Calling either attr_protected or attr_accessible within the class would set mass_assignment_safe? to true

class User < ActiveRecord::Base

  self.mass_assignment_safe? # => false

  attr_accessible :first_name, :last_name, :email, :password

  # or ... 
  attr_protected :created_at, :updated_at, :admin

  self.mass_assignment_safe? # => true

end

This would ensure that developers cannot inadvertently overlook applying at least some mass assignment protection to their model.

Of course there will be some models where no attributes should be protected. In these cases, having this warning message cluttering up the console a hundred times each time the test suite is run or showing up in the log all the time would be a pain in the ass.

To avoid this, the developer can explicitly state that the model should allow all attributes to be mass-assigned by passing :all => true to attr_accessible or :none => true to attr_protected.

class User < ActiveRecord::Base

  attr_protected :none => true

  # or ...
  attr_accessible :all => true

  # BTW ...
  attr_accessible :all => true, :username # => raises an exception!
  attr_accessible :none => true # => raises an exception too!

end

Conclusion

It's an obvious and simple solution but it requires no extra work from developers to implement, it won't break any existing code, and unlike the other suggestions I've seen it's DRY.

What's more, it keeps the implementation logic for attribute protection in the model where it belongs.

0 comments