blog.mhartl | Michael Hartl's tech blog

2008-09-21

Finding and fixing mass assignment problems in Rails applications

Filed under: Insoshi, mass assignment, Ruby on Rails — mhartl @ 18:14

Last week I received an email from Eric Chapweske (of Slantwise Design and the Rail Spikes blog) alerting me to mass assignment vulnerabilities in the Insoshi social network sourcecode. (See my post on mass assignment for a quick review of the concept, and don’t miss Eric’s mass assignment article for a more thorough treatment.) I quickly set to work fixing the problems, and within a few hours of receiving the email I’d pushed out a patched version to the Insoshi GitHub repository. Since the process was so instructive, and since mass assignment vulnerabilities are so common, I thought I’d share some of the details of what it took to fix them.

Fixing the models and controllers

The first step in solving mass assignment problems is to find them, so I whipped up a little find_mass_assignment plugin to make it easier:

$ script/plugin install git://github.com/mhartl/find_mass_assignment.git

(You’ll need Git and Rails 2.1 or later for this to work.)

This defines a Rake task to find mass assignment vulnerabilities. (It works by searching through the controllers for likely mass assignment and then looking in the models to see if they don’t define attr_accessible.) Let’s run it on the buggy Insoshi code and see what we get:

$ rake find_mass_assignment

/path/to/app/controllers/activities_controller.rb
    46      @activity = Activity.new(params[:event])
    68        if @activity.update_attributes(params[:event])

/path/to/app/controllers/comments_controller.rb
    20      @comment = parent.comments.new(params[:comment].

/path/to/app/controllers/messages_controller.rb
    50      @message = Message.new(:parent_id    => original_message.id,
    61      @message = Message.new(params[:message].merge(:sender => current_person,

/path/to/app/controllers/photos_controller.rb
    39      @photo = Photo.new(params[:photo].merge(person_data))
    61        if @photo.update_attributes(:primary => true)

/path/to/app/controllers/posts_controller.rb
    59        if @post.update_attributes(params[:post])
    157          post = @topic.posts.new(params[:post].merge(:person => current_person))
    159          post = @blog.posts.new(params[:post])

/path/to/app/controllers/topics_controller.rb
    28      @topic = @forum.topics.new(params[:topic].merge(:person => current_person))
    44        if @topic.update_attributes(params[:topic])

Yikes! That’s a lot of problems. How do we squash all these bugs?

One of the vulnerable models is the Post model, which is the base class for the ForumPost and BlogPost models. We’ll use the ForumPost model as our example. First we disable attr_accessible in the Post model, since we want to force all the derived classes to redefine it:

app/models/post.rb

class Post < ActiveRecord::Base
  include ActivityLogger
  has_many :activities, :foreign_key => "item_id", :dependent => :destroy
  attr_accessible nil
end

Then we set attr_accessible in the ForumPost model to allow only the post body to be set by mass assignment:

app/models/forum_post.rb

class ForumPost < Post
  .
  .
  .

  attr_accessible :body
  
  belongs_to :topic,  :counter_cache => true
  belongs_to :person, :counter_cache => true
  
  validates_presence_of :body, :person
  validates_length_of :body, :maximum => 5000
  .
  .
  .
end

Then in the Posts controller we update

  post = @topic.posts.build(params[:post].merge(:person => current_person))

to set the person attribute explicitly:

  post = @topic.posts.build(params[:post])
  post.person = current_person

Bypassing attr_accessible

This fixes the controller action, but unfortunately the corresponding RSpec specs fail. Having a good test suite proved invaluable in fixing the mass assignment problems, but the tests use mass assignment themselves, and much of that code fails. For example, here is part of the Post spec:

spec/models/post_spec.rb

describe ForumPost do
  
  before(:each) do
    @post = topics(:one).build(:body => "Hey there",
                               :person => people(:quentin))
  end
  .
  .
  .
end

This fails because of the attempt to set the person attribute by mass assignment. We could fix this as in the controller:

describe ForumPost do
  
  before(:each) do
    @post = topics(:one).build(:body => "Hey there")
    @post.topics.person = people(:quentin)
  end
  .
  .
  .
end

Unfortunately, the tests are riddled with this sort of code, and it’s a nightmare to make all such changes by hand. Moreover, inside the tests we simply don’t care about mass assignment vulnerabilities, so making a bunch of cumbersome changes is particularly annoying. Luckily, there’s a nice solution; after searching for a bit, I found an inspiring Pastie, which led me to open up ActiveRecord::Base and add some unsafe methods to create Active Record objects that bypass attr_accessible:

config/initializers/unsafe_build_and_create.rb

class ActiveRecord::Base

  # Build and create records unsafely, bypassing attr_accessible.
  # These methods are especially useful in tests and in the console.
  
  def self.unsafe_build(attrs)
    record = new
    record.unsafe_attributes = attrs
    record
  end
  
  def self.unsafe_create(attrs)
    record = unsafe_build(attrs)
    record.save
    record
  end
  
  def self.unsafe_create!(attrs)
    unsafe_build(attrs).save!
  end

  def unsafe_attributes=(attrs)
    attrs.each do |k, v|
      send("#{k}=", v)
    end
  end
end

(By putting in the config/initializers/ directory, we ensure that the additions will be loaded automatically as part of the Rails environment.)

With these methods in hand, we still have to update the tests by hand, but the edits are much simpler (and many can be done by search-and-replace):

describe ForumPost do
  
  before(:each) do
    @post = topics(:one).unsafe_build(:body => "Hey there",
                                      :person => people(:quentin))
  end
  .
  .
  .
end

We can use these methods in the controllers, too, of course, but if we do the word “unsafe” serves as a constant reminder that we’d better be really sure we want to bypass attr_accessible.

After making all the fixes, running our Rake task shows only one potentially vulnerable model:

$ rake find_mass_assignment
/Users/mhartl/rails/insoshi_core/app/controllers/photos_controller.rb
    40      @photo = Photo.new(params[:photo].merge(person_data))
    62        if @photo.update_attributes(:primary => true)

Checking the Photo model, we see that it defines attr_protected instead of attr_accessible (and explains why):

app/models/photo.rb

class Photo < ActiveRecord::Base
  include ActivityLogger
  UPLOAD_LIMIT = 5 # megabytes
  
  # attr_accessible is a nightmare with attachment_fu, so use
  # attr_protected instead.
  attr_protected :id, :person_id, :parent_id, :created_at, :updated_at
  .
  .
  .
end

With that, we’re done, and our application is secure. Huzzah!

About these ads

10 Comments

  1. […] by an email from Eric Chapweske of Slantwise Design, I recently audited the Insoshi social network for mass assignment vulnerabilities. Doing this manually was annoying, so in the process I developed a simple plugin to find likely […]

    Pingback by Mass assignment in Rails applications « Insoshi Ruby on Rails blog — 2008-09-21 @ 18:19

  2. Thanks for highlighting the testing problem, these are some great tips.

    Comment by Eric — 2008-09-21 @ 23:17

  3. Thanks for the tip on using attr_protected with attachment_fu. That saved me a little bit of aggravation.

    Comment by Sean Schofield — 2008-09-22 @ 18:16

  4. Michael, thanks for this – it’s very, very useful! I’m using Paperclip instead of Attachment_fu so I get to avoid those issues.

    I do have one suggestion (although, being a Rails newbie, I don’t know much about how the plugin works, or if this is even a worthy suggestion – but here goes…);

    Could you make the plugin search specifically for;

    @comment = Comment.update_attributes(params[:comment])

    …instead of

    @comment = Comment.update_attributes({:body, params[:comment][:body], :title, params[:comment][:title]})

    I found a couple of examples where the plugin told me I had MA holes when I was only assigning a couple of new attributes.

    Cheers

    Comment by Neil — 2008-09-23 @ 01:13

  5. @Neil: There’s no need for a plugin in your case. Just write

    Comment.update_attributes(:body => params[:comment][:body], :title => params[:comment][:title])

    Since you provide the hash keys explicitly, this statement is not susceptible to mass assignment vulnerabilities.

    Comment by mhartl — 2008-09-23 @ 09:55

  6. Sorry, Michael. I didn’t make myself clear enough on this one. The plugin (IIRC) highlights my second example as a ‘mass assignment’ loophole, when it isn’t. I was suggesting the plugin could be altered to exclude these instances in MA results. I think this is a fantastic plugin and should be a part of every pre-deployment check, so that’s why I think the change is worth doing.

    Comment by Neil — 2008-09-24 @ 00:11

  7. Oh, I see what you mean. There’s no way in general for the plugin to distinguish between a true mass assignment problem and code that just looks that way. It simply flags possible mass assignment issues. As noted in the final example from the post, that means that it catches some code in the Insoshi Photos controller that is actually safe. The programmer (in this case, me) has to realize it’s not actually a bug.

    Ultimately, the plugin’s output does rely on the programmer’s judgment to determine if the mass assignment in question is a problem or not. I’d love to improve its ability to discriminate between problems and non-problems, so if you think of any way to do that please fork the project on GitHub and send the changes my way.

    Comment by mhartl — 2008-09-24 @ 10:25

  8. Bless you, sir, for this write up and for the plugin!

    Used it on my current project, showed me exactly where I needed to tighten things up.

    Comment by Allan L. — 2008-11-15 @ 23:42

  9. […] been following this excellent post by M. Hartl and this post by E. Chapweske banishing mass assignment from one of my Rails applications due to […]

    Pingback by LRBlog » Blog Archive » Bypassing mass assignment for update_attributes — 2009-03-14 @ 14:45

  10. Ah, thanks! This cleared up some contradictions I’ve heard.

    Comment by Mister Cox — 2010-02-9 @ 12:25


RSS feed for comments on this post.

Sorry, the comment form is closed at this time.

The Shocking Blue Green Theme. Blog at WordPress.com.

Follow

Get every new post delivered to your Inbox.

Join 30 other followers

%d bloggers like this: