##How Homakov hacked GitHub and the line of code that could have prevented it

[@homakov’s](http://homakov.blogspot.com/) explot 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.

----

**TL;DR: 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)

*(this fix is not without its pitfalls - see later for things to watch for)*

**What the initializer does**

The initalizer forces you to declare parameters that can be updated via the `update_attributes` method. Rails’ default position is that any attribute on a model (except for a few of the ActiveRecord core attributes) 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 can trigger whitelisting on the model by declaring at least one attribute `attr_accessible`.

The initializer switches this round and makes whitelisting the default setting. With the intializer switched on, `update_attributes` will only update attributes on your models which are declared `attr_accessible`.

----

**Why this is needed**

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


**Why the User class is vulnerable**

    > u = User.create name: ‘Peter Nixey’, role: :subscriber;
     => #<User id: 1, role: :subscriber, 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 into it - usually via `params[:model_name]`.  It’s wonderfully quick and simple but open to abuse:

`update_params` will for instance happily update not only your name but also your role:

    > u.update_attributes name: ‘Jenson Button’,  role: :superadmin;
     => #<User id: 1, role: "superadmin", name: "Jenson Button", 
          created_at: "2012-03-05 09:39:31", updated_at: "2012-03-05 09:40:53"> 

*By fiddling with the user update form we just updated our role from subscriber to superadmin.* 

This is not good.

### How Homakov used update_attributes to hack the Rails GitHub account 

Since, by default, `update_attributes` will update any parameter that’s passed into it, Homakov realised he could use it to switch an SSH key for his own account to being one of the list of keys associated with one of the Rails GitHub account members.

Homakov assumed (correctly) that GitHub had a table containing users’ public keys. Each key has a `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. 

*schematic of what the GitHub PublicKey#update method might look like*:

    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`. The user_id he used was that of a member of the Rails repository members. 

The controller then simply updated all of the parameters Homakov passed it, including the new `user_id`. With an SSH key on his machine registered to the repository  of a Rails member all he then needed to do was [push](https://github.com/rails/rails/commit/b83965785db1eec019edf1fc272b1aa393e6dc57). This was the same hack he used for [posting from the future](https://github.com/rails/rails/issues/5239).

**Why don't I just avoid `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 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.

**How to protect update_attributes: 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 to prevent 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 

The problem is that `attr_protected` only protects us on attributes we actually declare to be `attr_protected`. 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.

**A bit safer protection: attr_accessible**

`attr_accessible` is the recommended method of tackling this problem. It’s actually a little bit of a misnomer since it's less about making an attribute accessible (it already was) and more about making it inaccessible. 

Delcaring any attribute as `attr_accessible` 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

`:role` is now protected since we haven't declared it `attr_accessible`:

    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


    class AddAccountType < ActiveRecord::Migration
      def change
        add_column :users, :account_type, :string
      end
    end

and leave our model unchanged:

    class User < ActiveRecord::Base
      attr_accessible :name
    end

then 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've not locked ourselves out of our own model. We can still update role directly it’s simply that it’s not vulnerable to being injected during `update_attributes`

    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 protects us by default

Create a new file:  
*config/initializers/disable_mass_assignment.rb*

    ActiveRecord::Base.send(:attr_accessible, nil)

The beauty of this 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 otherwise.

**Possible issues you might have with the initializer**

Once you setup the initializer, the first thing you’re going to need to do is declare all relevant attributes as `attr_accessible`.

A good test suite will help a lot here but either way you need to go through each model adding each parameter that you want to be accessible to your `attr_accessible` arguments:

    class User
      attr_accessible :email, :first_name, :last_name, :full_name
    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
- Nested attributes: [Instructions here](http://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html#label-Using+with+attr_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. However...

The argument has been made several times that it is up to the app builder to secure their own app. I don't agree with this though. Rails’ mantra is convention over configuration. 

**If the Rails team are going to stand by the 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](https://gist.github.com/1974187) that this is an authorization issue which is not a framework 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 issue 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 it `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.com/peternixey  
Blog:    http://peternixey.com  

----

*Further reading:*

* [Egor Homakov’s how-to on how he hacked GitHub](http://homakov.blogspot.com/2012/03/how-to.html)
* [Rails Spike’s “Is your application safe?”](http://railspikes.com/2008/9/22/is-your-rails-application-safe-from-mass-assignment)
* [Yehuda Katz’s proposal for addressing mass assignment](https://gist.github.com/1974187)