The scaffold generator creates code like the following, which is allowedly easier to handle, but vulnerable:
@user = User.new(params[:user])
With this code, Rails will create a new user based on the values that the user entered. Any corresponding attributes in the parameter hash params will be set in the user model. Arbitrary properties of the new user can be set by an attacker, the user's privileges, for example. Given you have a user registration form like this:
<form method="post" action="http://www.website.domain/user/register">
<input type="text" name="user[name]" />
…
</form>
An attacker could change the form (by saving it to disk, for example) to the following:
<form method="post" action="http://www.website.domain/user/register">
<input type="text" name="user[name]" />
<input type="text" name="user[admin]" value="1" />
…
</form>
If the attacker knows that the User model has an “admin” column, the newly created user will have administrator rights. One solution to this problem is, not to use mass-assignment and assign each value individually.
User.new(:first_name => params[:user][:first_name])
Another solution is, to protect several properties so they can't be assigned using mass-assignment, but have to be set individually. The following line in your model will protect the “admin” attribute, i.e. it will be ignored during mass-assignment.
attr_protected :admin
If you want to set a protected attribute, you will to have to assign it individually:
@user = User.new(params[:user])
@user.admin = false
You can also use the whitelist approach (highly recommended), which allows attributes to be mass-assigned, instead of forbidding access to them. Use attr_accessible with the attributes you want to allow access to, instead of attr_protected to do this.
10 responses so far ↓
1 matt lyon // Mar 20, 2007 at 17:02
people should be testing their controllers for this stuff, but likely aren’t.
If you don’t want to use the broad kudgel that is attr_protected, you can also use hash filters like I devised here: http://blog.caboo.se/articles/2006/6/11/stupid-hash-tricks
2 Dan Kubb // Mar 20, 2007 at 17:27
My vote is for the whitelisting approach of
attr_accessible. The safer approach for form validation (and other tasks like configuring a firewall) is to deny everything first, with specific exceptions for input you expect.3 Scott Fleckenstein // Mar 20, 2007 at 20:37
Assigning things individually is only marginally more secure, given the fact that your example contains no server side validation of data or authorization. Almost invariably, your application would have some sort of administrative interface to choose set admins, and that action could be divined in the same way that the admin attribute could be. Granted, guessing that you post to /users/make_admin/1 to make an administrator is more obscure than guessing the user[admin] field, but we know better than security through obscurity.
The bottom line is that you should check your incoming data. even something as naive as the following is much more secure:
before_filter :only => [:update, :create] do |c|
raise "access_denied" if c.params[:user].has_key?("admin") && !logged_in_user.admin?
end
My first paragraph seems a bit combative; I don’t mean it to be, but can’t find a sufficient alternative to the warning. I’m firmly in the camp that the less code I have to write the better, and if I can funnel all of my changes to a given model through one vanilla action, so much the better. That means there is only one place to secure properly, one place to update as things change, and one place to test. If you package your authorization code into some declarative-style extensions to your controllers, the code can be quite simple, flexible, and composable.
My ideal psuedo-code would be something like:
class UserController :unauthorized_access # see the rails plugin exceptional (http://nullstyle.com/home/exceptional)
def update
User.update(params[:id], params[:user])
end
end
-Scott Fleckenstein
4 Scott Fleckenstein // Mar 20, 2007 at 20:38
Well that didn’t work out so well, lets try it with pres:
First broken code block
before_filter :only => [:update, :create] do |c|
raise “access_denied” if c.params[:user].has_key?(”admin”) && !logged_in_user.admin?
end
second broken code block:
class UserController :unauthorized_access # see the rails plugin exceptional (http://nullstyle.com/home/exceptional)
def update
User.update(params[:id], params[:user])
end
end
5 Alexandre // Mar 21, 2007 at 0:58
Thank you for this entry. May I suggest the use of @params is deprecated since a while now. ( use params instead).
6 Steffen // Mar 21, 2007 at 3:27
It’s amazing how many potential security holes exist in every web application. Of course I’ve fixed this one in my application now - thanks to you!
Keep up the good work - can’t wait for your next posting
7 More on logins » Ruby on Rails Security // Mar 24, 2007 at 15:36
[…] for your comments. Scott writes in a comment: Assigning things individually is only marginally more secure, given the fact that your example […]
8 Martin Tepper // Apr 4, 2007 at 2:15
I stumbled upon the mass assignment issue too while working on a community site in Rails for a project at my university. I used the SaltedHashLoginGenerator and found it lacking a lot of the issues discussed here. BTW, it’s a great site, I wish I had found it earlier !
My simple solution was to make a whitelist as an Array and just delete everything not in there from the params hash…
I found this Firefox Add-on very useful for testing attacks:
https://addons.mozilla.org/en-US/firefox/addon/966
9 Curtis Miller // May 5, 2007 at 10:09
Ryan at Railscasts.com has a video dealing with mass assignment.
Check it out: http://railscasts.com/episodes/26
10 Ryan Lowe // Oct 1, 2007 at 9:55
After reading this post last week I created a simple plugin to search your Rails projects for this exploit and I thought some of you may find it useful. I called it audit_mass_assignment:
http://code.google.com/p/audit-mass-assignment/
rake audit:mass_assignment
The audit goes through your models and checks for attr_accessible. If the model doesn’t use it, the model fails the audit. The plugin is obviously only for people that use the whitelist approach.
I could see a whole family of “audit” plugins that could check certain aspects of Rails projects for bad/insecure patterns. You could include (or omit) any kinds of audits that you want and run them before you deploy your app with “rake audit”. Obviously not every exploit or bad pattern can be audited this way but many can be.
Leave a Comment