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

In Django (and Python in general) you have a form abstraction that validates the user input and filters out unwanted fields, preventing problems like this. On Rails (and Ruby in general) they don't use such abstraction (I do myself, using an implementation of forms abstractions[1] very similar to what Django provides).

Instead they send the params dictionary (which contains url captures, POST and GET values) directly to the model instance, and expect the model to deal with it. The problem with this approach is that it gives too much responsability to the model. Other than forms not necessarily mapping directly to models, making this more complicated, it is also prone to security issues, like the one Github suffered. ActiveRecord (Rails ORM) allows you to whitelist and blacklist fields at the model level (which IMO is the wrong way to do this, Django got it right), but a lot of people don't do it.

[1] https://github.com/tizoc/bureaucrat

GitHub really needs to clearify if access to private repos was compromised, for how long and if such access would be traceable.

The problems with update_attributes have been known, and messing with things like timestamps probably has been done before, but Rails itself didn't violate any acls. All users still can only access their own resources via Rails.

This attack was a combination of Rails and git/ssh keys. There's a bit of a clever aspect to it, one that isn't implied merely by understanding the vulnerabilities of update_attributes. While I think it is likely someone else has thought of it before, it is a little more exotic than something your average cracker is going to try.

Yes, if there is anything else in the public key table that you don't want to be updated via form fields, then you should be protecting yourself against it. However, every developer - or at minimum, every experienced developer - should know the basic maxim "never trust the data from your users".

But this method does protect resources from being arbitrarily assigned to other users.

In any controller where the user should be authenticated, I would suggest the following two guidelines (let's assume the model is PublicKey, as before):

1. In the index action, where you get a list of resources, use:

    @keys = current_user.public_keys

2. In all of the actions (such as show, edit, update, etc.) where the method starts with:

    @key = PublicKey.find(params[:id])

Remove that method and instead create a private method in your controller:

    def get_key
      @key = current_user.public_keys.find(params[:id]) if params[:id]
    end

And at the top of your controller:

    before_filter :get_key

This should just be a habit. With these two modifications, you've just scoped the resource down to the user everywhere that it is used. Additionally, you've DRYed out your single resource-specific actions (show, edit, etc.) because the code to find the resource only exists in one place.

Of course, you still need to think about attr_accessible. But even this is not a panacea. Consider the case where a user has a role_id field that specifies whether they are an admin, manager, or regular user. In this three role scenario, managers are allowed to create managers or regular users, but not admins. Admins can create users of all three roles.

This means that you may want to allow the role_id to be updated by mass assignment. You just have to ensure that users cannot update the role_id if the role they have picked is more privileged than their current role. You could just add a validator to the user model that does exactly that.

Alternatively, you could keep role_id as a blacklisted attribute, but in your controller you could check for the new role in the params, and then only assign it if the user should be able to assign it. Both approaches have merit. The bottom-line is that you still have to THINK.

Is it just me, or doesn't this sound very similar to SQL injection, only as applied to an ORM instead?

That is, if my understanding is correct, they're taking user posted data and trivially turning it into a command to update data.

This doesn't sound like a problem with Rails, in the same way that if I turn data I receive from the user straight into an SQL statement, the fact that people can abuse it isn't a problem with SQL.