Skip to content

Instantly share code, notes, and snippets.

@peternixey
Created March 5, 2012 13:10
Star You must be signed in to star a gist
Save peternixey/1978249 to your computer and use it in GitHub Desktop.
How Homakov hacked GitHub and how to protect your application by Peter Nixey

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


Please note: THIS ARTICLE IS NOT WRITTEN BY THE GITHUB TEAM or in any way associated with them. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my personal Tumblr at peternixey.com.

If you'd like to follow me on twitter my handle is @peternixey


@homakov’s 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.

NB this is also true for .new and .create

This strength and vulnerability in Rails is referred to as Mass Assignment and can happen when you pass a hash of arguments into either .new or .create as well as .update_attributes. The latter tends to be the most vulnerable but all are susceptible.

There is good advice including an even cleaner solution than the one below on the official Rails documentation for protecting against mass assignment attacks.


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 be could 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. This was the same hack he used for posting from the future.

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.


NB everything described here also applies to .create and .new

Althugh the GitHub hack was done using update_attributes, almost everything that's described here is also true for .create and .new too.

If you are not careful with the inputs that you feed into your object creation methods they too will initialize attributes on the object which you might not want to be initalized. For instance User.create( params[:user] ) woud also be vulnerable to role: :superadmin being passed as a parameter.

The ability of Rails objects to assign multiple variables simultaneously is referred to as mass assignment (docs).


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
  • Delayed Job (fix from @borski): add the following line to make the appropriate delayed_Job attributes work:
Delayed::Job.attr_accessible :priority, :payload_object, :run_at, :locked_at, :failed_at, :locked_by

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.

What should the default authorization setting to be, on or off?

Yehuda Katz makes the point that this is an authorization issue which is not a framework issue. I think the question here though is not “where should authorization be handled” but “what should the default setting for authorization be”.

In most other public-facing interfaces in Rails 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.

There are a lot of sites vulnerable and more being built every day

Rails is a brilliant framework designed by brilliant contributors. One of the reasons I like coding in it is that I feel that I always learn from the code I find (in PHP I generally wanted to rewrite it). This one weakness has always bugged me though and I feel it doesn't do Rails justice.

If GitHub, one of the best Rails teams on the planet can be taken out so easily, in so many places 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.


Author: Peter Nixey
Twitter: http://twitter.com/peternixey
Blog: http://peternixey.com


###Update - an update has been added to Rails to whitelist by default

6th March 2012

Part a result of everything that happened in the past few days, new projects in Rails will default to whitelisting all attribute arguments by default:

https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac53c57a302b162fd736

This is a slightly different fix than the one described here and almost certainly better - I would advise using the official Rails solution which simply involves a change in your config file:

config/application.rb

  ...
  config.active_record.whitelist_attributes = true

It will not affect existing projects which will still need to address their issues independently however it will mean that new Rails projects will be required to whitelist attributes by default. The commit is planned to be included in Rails 3-2-stable


Further reading:

@stevegraham
Copy link

Only since Rails 3.1, and this is a patch that affects the generator scripts not ActiveRecord itself, so only newly generated models have this default. Still so many vulnerable Rails apps out there!

@descentintomael
Copy link

Does anyone know if this works with Rails 2.3.8?

@mikehoward
Copy link

Nice writeup, but it's not that simple to solve.

Also, all authorization code significantly complicates development.

As a first guess, I think the behavior should be different in the three standard rails environments - development, test, and production.

development should allow mass assignment, but generate warnings
test should probably generate mass assignment failures when the default mass assignment happens w/o white lists.
production should only honor white lists.

Then there is the issue of associations: issue #4755. The mass assignment settings effect association updates, so this has to be thought through carefully.

@jonah-williams
Copy link

As @larzconwell pointed out; I think it would be preferable to encourage the use of config.active_record.whitelist_attributes = true in initializers/application.rb. With a number of Rails developers re-discovering this issue I think there's value in providing a consistent and clear solution. Many existing Rails apps will already have that line in their application.rb as it is included (commented out) in the Rails templates and it is a solution anyone who reads http://guides.rubyonrails.org/security.html#countermeasures is likely to be familiar with. I think that adding new initializers with different but equivalent configuration changes will only add to the confusion about how to prevent mass assignment exploits and what tools are already provided to do so as part of Rails.

@jewilmeer
Copy link

I think there are better practices to secure your code like dhh stated: https://gist.github.com/1975644

and just fetching any key is very bad practice. You should always try to retrieve the object through the currently authorized entity like this:

@current_key = current_user.public_keys.find params[:key][:id]

@vinbarnes
Copy link

+1 for spelling dude's name right

@homakov
Copy link

homakov commented Mar 5, 2012

Good explanation.

p.s. stay tuned. other guys got same bug.

@skimbrel
Copy link

skimbrel commented Mar 5, 2012

The bigger question I have is this: why doesn't Rails have form validation at the controller level? When you accept user input you should really indicate exactly what you're expecting to receive and what it should look like.

As an example, let's take web.py's forms library: http://webpy.org/form

This way you explicitly declare which fields you want and how to validate them. The cleaned form data doesn't even contain anything you didn't ask for.

@Veejay
Copy link

Veejay commented Mar 5, 2012

@homakov By the way, nice of you to bring this to people's attention. Good stuff.

@AstonJ
Copy link

AstonJ commented Mar 5, 2012

I like how Homakov rose to instant Rails fame through this :D

On a serious note, everyone should read the security section in the Rails Guides: http://edgeguides.rubyonrails.org/security.html

I agree with @petenixey - setting it on as default for the next release is a good step, then maybe going with Yehuda's suggestion later, where the form knows which attributes it should allow/accept - that and attr_accessible should cover it for most applications surely?

@mikehoward
Copy link

While some of you seem pleased with the way Homakov acted, he's actually damaged his credibility.

He could have achieved the same effect by contacting Github prior to pushing anything to the rails repository. Github would have responded, he would have been a hero, and still have a github account.

As is, he's damaged the credibility of the rails repository and will cause the rails team a lot of unnecessary work verifying that he hasn't added anything else 'interesting'.

@AstonJ
Copy link

AstonJ commented Mar 5, 2012

@mikehoward I don't think he was malicious with his 'hack', and in all fairness he did post days earlier about the problem.

I guess he didn't feel like the issue was being taken seriously enough and so went about demonstrating his point by exposing the vulnerability on the site of one of the worlds most high profile Rails dev teams. It just so happened to directly impact Rails - which in turn made them think very seriously about the issue.

Of course nobody condones hacking - but I don't think you can call this that.

@mikehoward
Copy link

mikehoward commented Mar 5, 2012 via email

@vinbarnes
Copy link

@mikehoward uh... @homakov just commented here... to this gist, as himself...

@mikehoward
Copy link

So if he didn't warn github, then what he did is a hack - notwithstanding that the hack he published is not malicious.

It's polite to warn the site owner of an issue prior to hacking one of the site's clients.

Isn't that so?

This is also getting blown out of proportion: usually critical data will be protected by some sort of authentication and authorization scheme. That scheme has to be tight and it can then be used to filter access to data by authority. This is a case where there was a hole in the security scheme.

Most data - when accessed under proper authentication & authorization is safe to mass assign.

I like the rails defaults as they are - but then I'm willing to take responsibility for the authentication and authorization framework.

@pejrak
Copy link

pejrak commented Mar 5, 2012

I am new to rails. If I whitelist my attributes with attr_accessible, does it mean that they are vulnerable to this method of writing through update_attributes? Example, if I whitelist :name, does it mean someone can change the model's name attribute of any record?

@larzconwell
Copy link

@pejrak

class User < ActiveRecord::Base
  # Attribute list :id, :created_at, :updated_at, :name, :email, :password

  # Available attributes
  attr_accessible :name, :email, :password
end

If you do it like the above then the only attributes available ANYWHERE is :name, :email and :password. This keeps the :id, :created_at and :updated_at attributes safe from tampering of users through forms, or something like cURL.

@pboling
Copy link

pboling commented Mar 5, 2012

@homakov - consider changing your handle to hakamov? ;) Nice work.

@courtenay
Copy link

We're using this for rails 2.3.x - lets you define roles for assignment. https://github.com/zenhob/mass_assignment_backport

@paulca
Copy link

paulca commented Mar 6, 2012

Awesome. Whitelisting seems like such overkill though. I think I do prefer the controller level fixing of this ... usually in my Rails apps I have an admin namespace where I DO want to essentially trust everyone.

@natejenkins
Copy link

As a new rails user, I'm having a little trouble understanding why the line:
before_filter :authorize_user
doesn't prevent a malicious user from accessing the update method in the PublicKeyController. Can someone please explain this in a little more detail?

@peternixey
Copy link
Author

@natejenkins - that line's really wrapping a load more code. The authorize_user filter itself runs before all methods in the controller and stops execution if the user is unauthorized.

You might more commonly see this filter written as load_and_authorize_resource which is the filter that Ryan Bates uses in CanCan

@natejenkins
Copy link

@petenixey: Thanks Pete for the reply. Just to make sure I understand things completely, the section of code:

class PublicKeyController < ApplicationController
...
end

has as an intended use the ability for a signed-in user to update his own public key. The filter :authorize_user is only verifying that the user is signed in. However, a malicious, signed in user can replace his own user id in the params hash with an id of whomever he would like and then the line:

@current_key = PublicKey.find_by_id params[:key]['id']

will return the key of the id that the malicious user specified. He can then update this key with his own public key, giving him push ability to a repo he would normally not have access to.

Please correct me if I'm wrong in this reasoning. Also, sorry to be reiterating what you stated in the gist above, a lot of this is new to me.

Finally, I've been following the rather excellent ruby on rails tutorial by Michael Hartl and in chapter 9 it deals with user authentication and authorization (obviously I have a lot to learn on this subject). Would an alternative solution to the GitHub vulnerability be something along the line of section 9.15 in the tutorial (http://ruby.railstutorial.org/chapters/updating-showing-and-deleting-users?version=3.2#code:correct_user_before_filter) where the :authorize_user filter in your example is replaced by:

class UsersController < ApplicationController
  before_filter :signed_in_user, only: [:edit, :update]
  before_filter :correct_user,   only: [:edit, :update]
...
  private
    def correct_user
      @user = User.find(params[:id])
      redirect_to(root_path) unless current_user?(@user)
    end
end

thanks,
nate

@peternixey
Copy link
Author

@natejenkins

As far as I can tell, your recap of the hack is what I posted above and understood from @homakov's blog post (and what he confirmed here). Michael Hartl's tutorial is indeed excellent and is how I learned Rails.

The code that you showed though still wouldn't protect against the mass assignment bug described because the current_key being updated does in fact belong to the attacker at the time of the update. The filter would therefore still allow the attacker to update their key.

It's only by the time the update has completed that it's been switched to belong to the other user so I don't think that your filter would catch that. You still need to protect against the mass assignment.

@courtenay
Copy link

courtenay commented Mar 8, 2012 via email

@borski
Copy link

borski commented Mar 13, 2012

DelayedJob dies a horrible death if you disable mass assignment by default. We're working on a fix for it, and I will update you when we have one, but just FYI. We had a nasty surprise. :)

@peternixey
Copy link
Author

@borski - I don't remember having to deal with that though no doubt your fix will jar my memory. If you can briefly describe it when you have the fix I'll update the instructions accordingly. Thank you

@borski
Copy link

borski commented Apr 10, 2012

Sorry I never responded to this! Totally forgot. Anyway, DJ needs the following methods accessible:

Delayed::Job.attr_accessible :priority, :payload_object, :run_at, :locked_at, :failed_at, :locked_by

Throwing that in the initializer should work fine. :)

@peternixey
Copy link
Author

Thanks @borski, I'll add that in too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment