Wednesday, July 29, 2009

Rails: Configuring Admins, Checkboxes, and attributes= Vulnerabilites

I'm using acl_system2 together with authlogic to provide ACLs for my Rails app.

I wanted to provide a list of checkboxes so that an admin can configure the list of roles assigned to a user. Since Rails really prefers to work with one model per form, and the list roles are separate from the user record itself, it took me a while to figure out how to do this. Thankfully, I found this post which showed me how to do it. The code looks like:
<% restrict_to "admin" do %>
<%= form.label :role_ids, "Roles" %>
<% Role.all.sort.each do |role| %>
<% field_id = "role_#{}" %>
<br />
<%= check_box_tag "user[role_ids][]",, @user.role_ids.include?(, {:id => field_id} %>
<%= label_tag field_id, h(role.title) %>
<% end %>
<% end %>
Without any changes to my controller, it just worked. However, that worried me.

I suddenly realized that if you give a user access to edit a record from ModelA and ModelA has_and_belongs_to_many ModelB, then a user can hack his form to pick which records from ModelB he has. I.e., my app was totally insecure--and, probably, so was everyone else's! I started freaking out ;)

Fortunately, the guys on the SF Ruby mailing list showed me this blog post. Apparently, I was right. It's a huge security vulnerability that even the third edition of "Agile Web Development with Rails" doesn't cover very well.

After analyzing all the options, I created config/initializers/disable_mass_assignment.rb with:
# Force every model to make use of attr_accessible.
# See:
ActiveRecord::Base.send(:attr_accessible, nil)
Then, I added calls to attr_accessible in my models like:
attr_accessible :username, :email, :first_name, :last_name, ..., :password, :password_confirmation
I ran my tests, and I ended up with this ugly error message:
Mysql::Error: Column 'session_id' cannot be null: INSERT INTO `sessions` (`data`, `created_at`, `updated_at`, `session_id`) VALUES('BAh7BiIKZmxhc2hJQzonQWN0aW9uQ29udHJvbGxlcjo6Rmxhc2g6OkZsYXNo\nSGFzaHsABjoKQHVzZWR7AA==\n', '2009-07-29 19:08:46', '2009-07-29 19:08:46', NULL) (ActiveRecord::StatementInvalid)
It took me a while to figure it out, but finally I stumbled across this blog post. I added the following to the bottom of config/initializers/disable_mass_assignment.rb:
ActiveRecord::Base.send(:attr_accessible, :session_id)
That fixed it.

I had to fix a few remaining issues, usually by tweaking the attr_accessible calls in my models. Finally, my code that would allow an admin to create another admin broke, because attr_accessible wasn't allowing access to role_ids. That was a good sign ;) I tweaked the controller like:
def update
restrict_to "admin" do
@user.role_ids = params[:user][:role_ids] || []
if @user.update_attributes(params[:user])
I even wrote the following Cucumber test to verify that everything worked:
Scenario: a user cannot hack his form to trick Rails into making him an admin
Given I am logged in as admin
When I follow "My Account"
And I follow "Edit"
And I check "admin"
And I check "subscriber"
And someone else strips me of my admin role
And I press "Update"
Then I should not see "Roles"
And I should not see "admin, subscriber"
Voila! All is happy in Rails land again!


Shannon -jj Behrens said...

By the way, I decided not to use the approach of controlling which params my controllers can see, which is used by this plugin:

There are many times where I want a controller to have access to certain params, I just don't want them assigned in a call to attributes=.

On the other hand, I can definitely appreciate the approach the plugin takes. I like the idea of different actions restricting access to different model attributes.

Shannon -jj Behrens said...

Here's another very well thought-out post on the subject: