Run a business? Check out LessAccounting, our bookkeeping system that'll save you hours per week.

Announcing:

We built the entrepreneur's bookkeeping system, it's called LessAccounting. You'll probably love it.

Use attr_protected or we will hack you

written by Steven on March 11, 2008

A good friend of mine recently asked me to look at his open source project and tell me what I think. While looking through the very nice code I discovered a security hole and promptly created a user account with administrative privileges using one command. Here is the command: curl -d "user[login]=hacked&user[is_admin]=true&user[password]=password&user[password_confirmation]=password&user[email]=hacked@by.me" http://url_not_shown/users That's right, that's all it took. Try it. Take this line and point it at your favorite website and see if you can create an admin account. There are a few easy things anyone can do to prevent this hack. In order of importance: # Use attr_protected. # Don't use mass assignments for your users table. # Don't have a users controller. # Split the users table into a users table and a profiles/people table. At Less we do all four. h2. Use attr_protected The attr_protected method in Rails will prevent the fields from being assigned via mass assignment. Here is an example: Bad:


class User < ActiveRecord::Base
#no attr_protected here
#this will allow the creation of your hacked admin user
end

class Users < ApplicationController
  def create
      @user = User.create params[:user]
  end
end

Good:


#this will not allow the creation of your hacked admin user
class User < ActiveRecord::Base
  attr_protected :is_admin
end

class Users < ApplicationController
  def create
      @user = User.create params[:user]
#user is created, but the is_admin flag is not set
  end
end

Wasn't that easy? You should all do this. h2. Don't use mass assignments for your users table No where in your code should you allow the users table to use mass assignments. What this means is that when a user account is created, assign each field individually. Here is an example: Bad:


class Users < ApplicationController
  def create
      @user = User.create params[:user]
  end
end

Good:


class Users < ApplicationController
  def create
      @user = User.new
      @user.login = params[:user][:login]
      @user.password = params[:user][:password]
      @user.password_confirmation = params[:user][:password_confirmation]
      @user.save
#user is created, but the is_admin flag is not set
  end
end

I know what you're thinking "Steve, that is too many lines of code, can't we use the built in stuff that Rails gives us to make thing easy?" My wife's cousin is an Assistant Warden at a prison in Texas. While taking a tour I noticed a sign that says "Security is not convenient." This is also true for security code. It is more lines of code, it is harder to write, it should fail securely. If you need to have an admin screen where admins set is_admin on users, then that action should also not use mass assignment and it should be protected so that only users who are already is_admin can access it. h2. Don't have a users controller This one is stupid. Obfuscation is the worst form of security, but combined with the others it is not too bad and worth doing. This will prevent all the script kiddies I just created from attacking your site because they will not know the name of the controller to hit. Of course if they just looked at the url that your signup form submits to they will know. That is why this is the weakest form of security. In fact it's so weak it's not really even security. h2. Split the users table into a users table and a profiles/people table Keep user settings that the system needs in the untouchable users table and user configurable settings in a separate table called profiles or people or something. Doing this means the only place the users table gets written to is on account creation and if you have an admin screen. The fewer access points you have, the safer you are. It also means that for profile data, you can use your profiles controller which allows mass assignment and keep the code nice and tight. Why are you still reading? Go, fix your code right now. My spider is already loose, looking for your rails app. Go fix it already!!

Business Owners: save hours per week with LessAccounting. It's like Quickbooks, just not total shit.

14 Comments

E. James O'Kelly
E. James O'Kelly said on March 15, 2008

Very good point. I see this a lot in the code reviews do, or if the coders are using a better design then, :is_admin (which sucks design-wise) and using STI (which is a little better, but roles would be even better) and don’t protect the :type attribute then you can make yourself a user with any subclass.

Instead of attr_protected I would suggest using attr_accessible in every class, thereby defining what you have access to in the model. You fight a lot of weird bugs at first cause shit is nil when it shouldn’t be but after you get used to specifying what should be accessible it becomes second nature.

Caleb
Caleb said on March 15, 2008

When I picked up Rails, the mass assignment feature scared me. I had already seen the horrors of mass assignment when people passed the entire $GET and $POST PHP variables into objects or queries to be run against the database.

Yes, you can (should) use attr_protected, but even that is relying on the security of attr_protected. What happens if (when?) a flaw in attr_protected is ever found? If one never is, then great. But I’d much rather depend on the reliability of good old fashioned if’s, else’s, and assignment operators. Yes, I know that even the fundamentals of languages have had security flaws in them. But, in general, as abstraction increases (1’s 0’s → assembly → C/C++ → interpreted languages → Frameworks) so does the potential for security flaws in the code written.

Always, always, always treat anything that is inputted into your code as poison. Assume that every parameter is attempting to overflow a buffer, perform SQL injection, or contains embedded code (ie: <script>alert(‘hacked!’);</script>). Always validate input, and always try to understand the how’s and why’s behind framework features.

Faheem
Faheem said on March 19, 2008

@James

using attr_accessible instead of attr_protected makes sense and guard against human factor (leaving any attribute accessible). But, how would you mass define everything else attr_protected which are not attr_accessible. Is there any way to do that in active record?

probably something in activerecord::base?

Ben
Ben said on March 19, 2008

One more vote for attr_accessible here – it’s all too easy to forget to protect something added after the fact, so it’s much safer just to protect everything by default.

Incidentally, sites that don’t use this are vulnerable even if your hackers aren’t command-line-users – with extensions like FireBug, they can edit form markup directly to have it submit whatever they like.

Roland
Roland said on March 19, 2008

attr_protected smells, use attr_accessible.

whitelist approaches are much better when it comes to security.

Heiko
Heiko said on March 20, 2008

Another vote for the whitelist approach of attr_accessible:
http://www.rorsecurity.info/2007/03/20/do-not-create-records-directly-from-form-parameters/

mercy
mercy said on March 20, 2008

Great! Thank’s for that!

After reading your post, 2 other issues that apply to this topic came to my mind:

http://blog.odeley.com/?p=51

ken
ken said on March 21, 2008

“‘Steve, that is too many lines of code’ […] ‘Security is not convenient.’ This is also true for security code. It is more lines of code, it is harder to write, it should fail securely.”

Too many lines of code isn’t bad because it’s inconvenient. It’s bad because the chances of a mistake go up linearly! (Can you tell me you’ve never seen one person write redundant code like that, and then had somebody else later update just one line?)

In this case, it’s not hard to write a 1-liner that’s just as secure as the 5-line one:

@user = User.create params[:user].reject{|k,v| !OK_TO_SET.member?(k)}

Then declare OK_TO_SET=[:login, :password, …] in just one place.

sheesh ken
sheesh ken said on March 21, 2008

Come on ken, your argument is the stupidest thing I read all day, and I read some posts on reddit! Your one liner is not secure or maintainability, it is 5 lines in one. It is not readable, it is dense and annoying.

When will you rubyists grow up and learn that 1-line does make a good method.

Less Name
Less Name said on March 21, 2008

>Use attr_protected.
>Don’t use mass assignments for your users table.
>Don’t have a users controller.
>Split the users table into a users table and a profiles/people table.
>At Less we do all four.

All I can think of is: over-reaction. The first step solved your problem, and the rest is only running around panickingly, half-proud of having found this “immense vulnerability” and pretty scared as well because this might have become a catastrophe, but your saved the day with pure smartness.

Daniel Fischer
Daniel Fischer said on March 22, 2008

Here’s one more for attr_accessible :) good post btw!

Steve
Steve said on March 24, 2008

The real problem is the ‘user[is_admin]=true’ part of the URL. Personally, I’d say that it’s ok to allow a person to generate user accounts directly from a form, but just not any account with any real power. The user account ‘is_admin’ flag should be controlled from some sort of admin interface that is done outside of the normal user webpage which has security enabled for the entire session.

Jon
Jon said on April 01, 2008

I have to second Steve’s comment. Wouldn’t the entire problem be resolved if the administrative flag is in another table where it belongs? The role_requirement plugin works excellently, and let’s you separate the key pieces of logic: user creation, and permissions assignment.

And of course, appropriate validation and testing!

Steven Bristol
Steven Bristol said on April 01, 2008

@Jon,

If it’s just in another table, you could have the exact same problem.

(Besides, I f—-king hate keeping roles in the db. IMHO they belong hard coded in the controller.) Let the flame-war begin!

steve

Leave a Comment

About Steven
Steven Bristol has written code for the past 20 years. He like green vegetables and kittens, oh and butterflies too. He loves to throw ninja stars at his enemies.

You Should...

Follow Steven on Twitter
Friend Steven on Facebook
Subscribe
LessEverything Copyright 2011 LessEverything.com
We don't like footers, they're kinda boring