Best Practices: a strong case for [redacted] part 1

Posted by trevor Wed, 24 May 2006 03:10:00 GMT

Tonight I decided to tip over my blog-post idea pickle jar and see if anything interesting fell out. This is the first one that didn’t get crumpled up and thrown in the trash (gotta love that pickle jar).

Instead of just blathering on I’d like to show you a few lines of code and invite you to answer a pop quiz:

class UserController < ApplicationController
  def save
    current_user.update_attributes(params[:user])
  end
end

class ProjectController < ApplicationController
  def destroy
    @project = Project.find(params[:id])
    if ( @project.user_id == current_user.id ||
      current_user.is_administrator? )
      @project.destroy
    end
  end
end

class User < ActiveRecord::Base
  attr_protected :is_administrator
  has_many :projects
end

class Project < ActiveRecord::Base
  belongs_to :user
end

Before anyone gets too bent out of shape – don’t worry about the stuff you don’t see. Everything you need is there.

Here’s the requirements we’ve been given:

  1. Users should not be able to grant themselves administrative capability
  2. Only administrators or the project’s owner can delete a project

And here’s the pop quiz: are those requirements satisfied? Why or why not?

Bonus points (for what they’re worth) will be given if you can tell me what I’m going to make “a strong case for” in the title of this post.

I’ll post my answers tomorrow (Wednesday) at 10:00 PM GMT.

[Edit] someone asked so I’ll clarify: the current_user method in the controllers always returns the correct user.

[Edit] another clarification: we can assume that actions not shown in ProjectController such as edit/save perform the same checks as ProjectController#destroy before doing anything.

[Edit] Part 2 is available now.

Comments

Leave a response

  1. Tim Case said about 1 hour later:
    i'm going to guess that your is_adminstrator field might be defaulting to true on save, so you are going to suggest explicit attribute assignment on save instead of update_attributes?
  2. Tim Case said about 1 hour later:
    Also the is_adminstrator attribute defined in the User model doesn't use a question mark, but the if statement in the delete method of the controller is to current_user.is_adminstrator?
  3. trevor said about 2 hours later:
    Tim, As for the is_administrator field defaulting to 'true' - no it doesn't. As for the question mark in current_user.is_administrator? - that's part of Rails' syntactic sugar for boolean fields. As for suggesting explicit attribute assignment - no, I'm not going to suggest that. I don't want to give too much away - I actually figured I'd get a right answer pretty much immediately. You're on the right track but you haven't spotted the real danger.
  4. j`ey said about 3 hours later:
    The needs look satisfied to me!
  5. Ted said about 10 hours later:
    You can hack the URL for (or create a dummy form than submits to) /users/save and passes in is_administrator=true which will set the database field and allow you to run amok.
  6. François Beausoleil said about 12 hours later:
    Ted, that's not right. User contains the line "attr_protected :is_administrator", which means :is_administrator is protected from mass assignment. Trevor, you don't show ProjectController#edit. Nor does Project protect :user_id. Therein lies the problem. I can change the project's owner to myself, and then delete the project.
  7. trevor said about 12 hours later:
    François - you don't need to see ProjectController#edit. Everything an attacker needs is in the code I gave.
  8. Andrew Williams said about 18 hours later:
    (Rails newbie so please be nice :) When you save the user in the user controller, you are using update_attributes ... couldn't I hack/pass additional projects in the POST making myself a project owner, then delete? Maybe a strong case for "has_one"??
  9. David Felstead said about 18 hours later:
    The vulnerability lies in the 'update_attributes' call in UserController#save - it turns out that you can pass parameters to update for your association classes, e.g. your has_manys and your belongs_tos. For example, a ruby call that looked like this: current_user.update_attributes(:projects => []) would delete all the user's projects, so all the attacker would have to do is construct that hash via form variables. I'm not sure if you can traverse the association hierarchy via an attributes hash, but if you could that would open up a REALLY NASTY can of worms. So, my guess is that you're after "a strong case for attr_protected on association collections to be set by default". Do I get a cookie?? :)
  10. trevor said about 19 hours later:
    Andrew - you may be on track for a kewpie doll but you'll have to flesh out your answer more for me to be sure...
  11. trevor said about 19 hours later:
    David - oh so close but no cookie for you. I'll give you all a hint: the code allows me to construct a URL that will allow me to assume ownership of an arbitrary project. After that I can delete it at will by calling ProjectController#destroy action.

RSS feed for this post trackback uri

(leave url/email »)