I agree that the default for Rails should be more secure, but this was a really basic mistake by the GitHub team. Yes, mistakes do happen, but there's a very simple way to avoid the exploit that is postulated in this article.
The 'schematic' of what the public key update looks like from the original post:
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
The correct way to code this is as follows:
class PublicKeyController < ApplicationController
before_filter :authorize_user
...
def update
@current_key = current_user.public_keys.find params[:key]['id']
@current_key.update_attributes(params[:key])
end
end
This has two updates to it that protect against this exploit:
1. Rather than calling "find_by_id" on the PublicKey model, which searches all public keys, you call it on the current user's list of public keys. This scopes the search down to the public keys that they own. Thus, if you pass in the id of a key they do not own, it will not be found, leading us to:
2. Using "find" instead of "find_by_id" will trigger an ActiveRecord::RecordNotFound error (404) if the resource is not found. Of course, find_by_id will just return nil in this instance, so the update_attributes part would still fail, but triggering a 404 is an easier, cleaner way of dealing with this, I think.
It's really very simple: you don't let people access stuff they don't own.
Now, this does not protect you against faked timestamps, or against privilege escalation by passing in a faked "role" parameter, and so on. You still need to use attr_accessible to protect yourself from that stuff, but scoping resources down to the user who owns them is a simple technique that should be standard practice for applications with authentication.
I own the key. I set the owner of the key to you. You now own the key that I have on my machine. I commit to your repo.
I'm sure the github team has authorization at a model level, preventing access to other user's resources. This wasn't an instance of accessing another user's resources via rails, it was assigning your own resources to another user, then abusing that fact via git.
Actually, that's true. I stand corrected. I think I was distracted by the rather obvious issue in the example code that the OP posted, which did not include this basic protection.
still mean that an attacker can pass in arbitrary fields to be updated in the public key table? If that's the case then doesn't it open room for an attacker to change a field that is assumed not accessible from the outside? For example, would it be possible to change an _id field that references another table?
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.
and then define the scoping rules in the declarative auth file:
authorization do
role :user do
has_permission_on :public_keys do
to [:write, :read]
# user refers to the current_user when evaluating
if_attribute :user_id => is {user.id}
end
end
end
This is a bit more DRY, because you are abstract out the conditions of access. This is especially useful in situations where you have readonly access or other types of acl.
The 'schematic' of what the public key update looks like from the original post:
The correct way to code this is as follows: This has two updates to it that protect against this exploit:1. Rather than calling "find_by_id" on the PublicKey model, which searches all public keys, you call it on the current user's list of public keys. This scopes the search down to the public keys that they own. Thus, if you pass in the id of a key they do not own, it will not be found, leading us to:
2. Using "find" instead of "find_by_id" will trigger an ActiveRecord::RecordNotFound error (404) if the resource is not found. Of course, find_by_id will just return nil in this instance, so the update_attributes part would still fail, but triggering a 404 is an easier, cleaner way of dealing with this, I think.
It's really very simple: you don't let people access stuff they don't own.
Now, this does not protect you against faked timestamps, or against privilege escalation by passing in a faked "role" parameter, and so on. You still need to use attr_accessible to protect yourself from that stuff, but scoping resources down to the user who owns them is a simple technique that should be standard practice for applications with authentication.