The line of code that could have protected Github
@homakov’s attack on GitHub was simple and straightforward. Calling it an attack makes it sound malicious whereas the truth was that GitHub bolted its front door but left the hinges on quick release. Homakov released the hinges, walked in and shouted to anyone who would listen that they had a problem.
He was right. The Rails defaults are vulnerable and there’s no better illustration of this than when when one of the best Rails teams in the world is severely compromised.
How to protect your Rails application from the GitHub attack
Add the following initializer:
config/initializers/disable_mass_assignment.rb
ActiveRecord::Base.send(:attr_accessible, nil)
TL;DR: What the initializer does
The initalizer forces all models in your application to whitelist parameters that can be updated via the update_attributes method. Rails’ default position is that any attribute on model is updatable via update_attributes. If you want to protect attributes from being updated you either need to single them out using attr_protected or you trigger whitelisting on the model by declaring at least one attribute to be attr_accessible.
The initializer switches this round and makes whitelisting the default position. With the intializer switched on, update_attributes will only update attributes on your models which are declared attr_accessible.
(also see the last paragraph of this article for for possible issues with enabling this)
How update_attributes works by default
Let’s take a simple User model:
create_users.rb migration:
class CreateUsers < ActiveRecord::Migration
def change
create_table :users do |t|
t.string :role
t.string :name
t.timestamps
end
end
end
and a very simple User class:
class User < ActiveRecord::Base
end
We now have a user with the following attributes:
User
- role
- name
How the default behaviour is vulnerable (& where the whole hoo-ha began)
> u = User.create name: ‘Peter Nixey’, role: :subscriber;
> u
=> #<User id: 1, role: :admin, name: "Peter Nixey", created_at: "2012-03-05 09:39:31",
updated_at: "2012-03-05 09:39:31">
By default, update_attributes (which is what you’ll probably use in your update method) updates any attributes that are passed in via http params. It’s wonderfully quick and simple but also vulnerable.
We can for instance not just update our name but also our role:
> u.update_attributes name: ‘Jenson Button’, role: :superadmin;
> u
=> #<User id: 1, role: "superadmin", name: "Jenson Button", created_at: "2012-03-05 09:39:31",
updated_at: "2012-03-05 09:40:53">
We just updated our role from subscriber to superadmin. This is not a good thing and is not really what we’d like as our default setting.
Do I have to use update_attributes?
Why all the fuss about update_attributes? If it’s so insecure why use it, why not manually update stuff using code such as
user.name = params[:user][‘name’]
This way surely everything would only be updated if we specifically updated it?
We could do but it would take five lines where update_attributes only takes one line. update_attributes is also for better or worse, the Rails Way and so it’s a good idea to understand why it’s vulnerable and how to secure it.
update_attributes was how Homakov added himself to the Rails GitHub account
Since update_attributes will update any parameter that’s passed into it, Homakov’s could use it to add an SSH key on his machine to the list of keys associated with the Rails GitHub account.
Homakov assumed (correctly) that GitHub had a table containing users’ public keys. Each key has a key_value and a user_id. Homakov also correctly postulated that he might be able to update his own public key to have the user_id of one of the Rails GitHub account members.
outline of what the GitHub update action looks like for the PublicKey controller:
class PublicKeyController < ApplicationController
before_filter :authorize_user
...
def update
@current_key = PublicKey.find_by_id params[:key][‘id’]
@current_key.update_attributes(params[:key])
end
end
Homakov PUT an update to his own existing public key which included a new user_id. In this case he used the user_id of a member of the Rails repository team.
The update controller then simply took the user_id attribute that Homakov passed and updated his key. With an SSH key on his machine registered to the repository of a Rails member all he then needed to do was push.
How to protect against this: attr_protected (not recommended)
Everything that happened happened because the user_id attribute should not have been updatable via update_attributes. Rails has a method for exactly this and it’s called attr_protected.
class User < ActiveRecord::Base
attr_protected :role
end
With that line added it doesn’t matter whether we pass the role in via a PUT, it still won’t update:
u = User.create name: "Peter Nixey", role: :subscriber;
u.update_attributes role: :superadmin
WARNING: Can't mass-assign protected attributes: role
=> true
however, attr_protected only protects us on attributes we actually add it to
The problem with attr_protected is that it only works when we remember to add it. If we don’t put it in we don’t get protection.
I prefer to know I’m protected and safe until I chose to be unsafe and that (in theory) is what attr_accessible gives us.
attr_accessible protects all attributes unless we declare otherwise
attr_accessible is the recommended method of tackling this problem. It’s actually a little bit of a misnomer since its value is not in making an attribute accessible (it already was). Its value is that that adding it implies that all the other attributes are not accessible. Think of its real value less as being attr_accessible and more as being attr_whitelist
class User < ActiveRecord::Base
attr_accessible :name
end
u = User.create name: "Peter Nixey", role: :subscriber;
u.update_attributes role: :superadmin
WARNING: Can't mass-assign protected attributes: role
=> true
The nice thing about attr_accessible is that all new attributes are protected by default. If we add an account type to our database (but leave the model unchanged)
class AddAccountType < ActiveRecord::Migration
def change
add_column :users, :account_type, :string
end
end
class User < ActiveRecord::Base
attr_accessible :name
end
... the :account_type field is automatically protected
u = User.create name: “Peter Nixey”, account_type: ‘free’
u.update_attributes account_type: ‘paid’
WARNING: Can't mass-assign protected attributes: account_type
=> true
Either way you can still manually update attributes
We can still update role directly it’s simply that it’s not vulnerable to an update through our update controller:
u.role = :superadmin
u.save
(0.2ms) UPDATE "users" SET "role" = 'superadmin',
"updated_at" = '2012-03-05 10:11:05.042023' WHERE "users"."id" = 1
=> true
However, even attr_accessible only protects us when we remember it
The problem with attr_protected was that it only protected us when we remembered to add it to the attribute.
The problem with attr_accessible is that it only protects us when we remember to add it to the model. Sometimes (as GitHub showed us) it’s easy to forget to do that.
The disable_mass_assignment initializer gives security by default
The beauty of the initializer:
config/initializers/disable_mass_assignment.rb
ActiveRecord::Base.send(:attr_accessible, nil)
is that it effectively adds attr_accessible to every model we create (actually what it does it take it away by default but it comes to the same thing). No attribute can be updated unless we declare it attr_accessible. We’re secure until we decide not to be.
Possible issues you might have with the initializer
If you add attr_accessible the first thing you’re going to need to do is to declare all attributes accessible which are accessible.
A good test suite will help a lot here but with or without one, go through each model adding each parameter that you want accessible to your attr_accessible arguments:
class Xyz
attr_accessible attribute_a, attribute_b, attribute_c
end
You’re going to have some frustrations. There are going to be things that you don’t see coming which will fail silently. Problems I’ve had are:
- Authlogic: you need to remember to make attributes like password, email etc accessible
- Paperclip: remember to make paperclip attributes accessible
I’m sure you’ll hit other issues too but you can generally knock them off by adding attributes one by one to the list of accessible ones.
How Rails could address this
I wouldn’t pretend to have anything like the oversight of the Rails landscape that the Rails core team do. I’ve only built a very few apps and I’m no guru.
The argument that it is up to the app builder to secure their own app is however dangerous. Rails’ mantra is convention over configuration. If the Rails team are going to stand by this mantra then they also need to accept that the Rails Way to handle updates is conventionally insecure until configured otherwise.
Enforcing that attributes have to be declared attr_accessible by default would immediately make things better.
The question is not "where should authorization be handled" it is what is the default setting
Yehuda Katz makes the point that access is an authorization issue. The question here though is not “where should authorization be handled” but “what should the default setting for authorization be”.
In almost everywhere else the default setting for authorization is unauthorized. You can’t even reach a controller method unless you explicitly create a route for it. update_attributes however defaults to authorized. This is not an academic design choice, it’s one that’s carrying a real world cost right now.
Arguing over what layer of the app is responsible is like BP blaming Transocean for the Deepwater Horizon. The issues is that oil is leaking.
There are a lot of sites currently vulnerable and more being built
If GitHub, one of the best Rails teams on the planet can be taken out so easily by such a simple hack then there is a real and present issue. As Yehuda says, not all security vulnerabilities can be fixed by the framework however this one can and it would make sense to do so.
A three step suggestion for how the Rails team could address the isssue:
- On the next release start raising warnings in development when an attempt is made to update an attribute without declaring attr_accessible
- On the release after raise warnings in production and errors in development
- On the third release make attr_accessible on by default
Author: Peter Nixey
Twitter: http://twitter
Blog: http://peternixey.com .com/peternixey
Further reading: