How Homakov hacked GitHub & the line of code that could have prevented it
gist.github.com
gist.github.com
Rails devs (some, not all of them) had dismissed his complaint because "Every Rails developer hears about attr_accessible." Well, I'll be the first to say that I can't remember the last time that this update_attributes vulnerability had been pointed out to me. I certainly can remember all the times that Rails docs reminds me to use their sanitizing helpers when making ActiveRecord queries.
To be fair, I haven't developed apps that required the use of user-facing access to update_attributes, and maybe when I got around to using that, I would've wisely consulted the dev guides to make sure I was following best practice. But knowing me, I probably would've likely thought, "Well, that seems simple enough, here goes."
It's not that the logic behind this vulnerability is hard to understand...in retrospect, it's as clear and blatant as the processes that lead to SQL injection.
But surgical patients die because elite surgeons sometimes forget to wash their hands (Google "Atul Gawande checklist"). It's not an impossibility that a skilled dev team would overlook the update_attributes issue.
The Rails team was right in arguing that this wasn't a security risk given a half-competent dev. But they were looking at the problem from the wrong perspective and assumed that everyone is as familiar with Rails best practices as they were. So how else could Homakov convince them otherwise other than pricking a high-profile dev group?
What if Homakov managed to alert the Github team, and they managed to fix it quietly? Github would be safe but thousands of Rails sites would still be operating in ignorance. It truly stinks for the Github group that they had to respond to a five-alarm emergency on a Sunday...on the other hand, I think there are going to be a lot of Rails devs who are thankful that they (involuntarily) took one for the team. Thanks to Homakov, it was a small hit.
What you're describing is people doing work that can be automated. That can happen in software, but only in incompetent development teams, or under very special circumstances.
Surgery is different from programming because it's manual labor, and it still exists as a human activity because we don't yet have a feasible way to automate it.
http://guides.rubyonrails.org/security.html
One might counter that nobody reads these guides back-to-back. I must admit I haven't read every word of every Rails guide. But I have read the security guide in its entirety, and I think every developer owes it to herself and her clients to do the same.
The status quo of Rails is that everything is sanitized if you use the helpers. And validators on the models only look at data integrity. So all this protection, at least for me, kind of lulls you into feeling secure, because SQL inject is generally the typical, awful-case scenario.
update_attributes is not really a SQL inject attack vector (since the actual values are sanitized)...it's partially a social engineering scheme.
If Microsoft left a network accessible default passworded Admin account in Windows Server but documented it and told people to change it, would that be okay simply because it was documented? Documentation is no panacea for bad defaults.
Part of the OP's point (and he's absolutely right) is that this incident proves beyond a shadow of a doubt that just warning about the issue is clearly not enough. If the GitHub team screwed this up, what hope do the majority of the unwashed masses of Rails developers have, warning or no warning?
Of course, everyone should have really had a firewall anyway, so this was obviously cool right? After all, it's up to the user to secure their machine.
(Disclosure: That was sarcasm.)
For those not familiar with Rails, it boils down to this: You as the programmer need to use a security feature built into Rails called mass assignment security. If you fail to use this feature, you have a vulnerability. In other words, the default is insecure by design. The alternative would be to make Rails secure by default, but that would mean pretty much nothing would work until you explicitly granted access where necessary. I guess the core team figured "not working by default" was worse than "insecure by default."
Homakov obviously disagreed with this design decision. I can understand why, and I mostly feel the same way.
So Homakov posted an issue to the Rails repo Github (https://github.com/rails/rails/issues/5228) suggesting the default be changed. He made a good case and was initially polite. A few days passed, and nobody else had posted to his thread.
So, presumably to draw attention to this issue, he exploited the fact that Github had failed to use mass assignment protection. Specifically, he posted a comment with a far-future timestamp, which obviously should be impossible. (I think that's what he did, although Github seems to to have fixed the timestamp now.) He then said this should be proof enough that the Rails defaults need to be changed.
The problem with Homakov's argument, as pointed out in subsequent comments in the thread, is that Homakov's hack only demonstrated a mistake on Github's part, not a bug in Rails. It didn't prove anything about Rails that we didn't already know. The only thing surprising he demonstrated was that Github had left open a rather serious vulnerability.
TL;DR: Rails has some less-than-secure defaults which all Rails developers are expected to understand and deal with. Homakov found out that Github failed to do so in at least one instance, and he wanted to use that as proof the Rails defaults should be changed.
But I disagree that Homakov's hack "only demonstrated a mistake on Github's part." It rebutted the Rails team opinion (and again, not all of them disagreed with Homakov) that this was a trivial, edge-case problem.
IIRC, one of the changes in Rails 3 was that interpolation in ERb templates were html sanitized by default: http://stackoverflow.com/questions/4731992/rails-3-how-to-re...
The fact that web devs write templates vulnerable to XSS is not Rails fault, but apparently the problem was prevalent enough that HTML sanitizing was turned on by default.
Apparently, there wasn't empirical evidence to show that update_attributes had the same rate of mistakes to justify a change in defaults...Homakov's hack was a powerful rebuttal.
You make a good point that showing a hugely popular app with mistake X suggests that mistake X should be prevented at the framework level.
The point of my post was not to give Rails a pass, and I apologize for misleading if it came off that way. Rather, I was trying to clarify the situation for those who are less familiar with Rails. The discussions surrounding this issue (including Homakov's own words) seem to erroneously suggest that Homakov discovered a previously unknown vulnerability in Rails. I was merely clarifying that he instead found a vulnerability in a specific Rails app.
Now, it's a matter of opinion as to whether the Rails default should be called a "vulnerability." I say yes, but reasonable people can disagree. What's clear, though, is that no previously unknown vulnerabilities in the framework have been revealed.
This is the problem. We think of a security problem as "the developer made a mistake." Often it's the software architect who made the mistake, and if we insist that frameworks weed out the bad architects we are all better off.
The advantage of this approach is that even otherwise ordinary security wholes become hard to exploit in useful ways. For example, the set of interesting attacks you can pull off from SQL injection when the SQL permissions are tied to your application login are quite a bit less than they are ordinarily and while you can still do nasty things, the attacks tend to require greater internal knowledge of the database, and the scope of vulnerability is narrowed. Get rid of string interpolation in your queries to the extent possible and another issue goes away.
Be paranoid about security and that will serve you well.....
So when I read that a framework is insecure by default, I naturally suppose that I have good reason to stay away from it.
Given the amount of logging that occurs if you do set whitelist_attributes, it's not like this is a huge problem to fix. And, that logging (and the fact that your app mysteriously doesn't work) serve as a loud signal as to what action to take. On the other hand, the "insecure by default" solution is a silent and potentially catastrophic failure.
Compare to how brake pads squeal: even the least mechanically savvy driver brings their car to a mechanic when their pads are running thin.
Finally, the suggested fix (which, frankly, wouldn't have helped github) was simply to update the default generator to set whitelist_attributes, rather than merely including a comment to the effect. The "everything is broken" list would be introductory guides, full stop. So, novice developers would be held up until the guides could be updated with good security practice. Experienced devs, who supposedly all know about this, wouldn't have any problem on new apps.
And the core team have basically said "meh, too much trouble." Apparently, they haven't been chasing html_safe! calls through their views, which is frankly way more of a pain than attr_accessble'ing data fields.
user.name = params[:user][‘name’]
Why isn't the default the opposite?
Same reason that "enum" and "int" are pretty much interchangeable in C, that arithmetic conversions and truncations are implicit -- it's not very safe, but it is more convenient.
we all know how it ended up.
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.
I have seen worse though :-P
BigCo's should take a note.
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
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
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.
@current_key.update_attributes(params[:key])
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
@key = PublicKey.find(params[:id])
def get_key
@key = current_user.public_keys.find(params[:id]) if params[:id]
end
before_filter :get_key
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.
PublicKey.with_permissions_to(:write)
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.
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.
The params passed in is auto-deserialized and thus can be a list or a hash with query options instead of your expected "id" value.
But as a security guy, given the power of Public Key assignment in the context of a system for managing access to Git repositories, I can't help but be a little surprised that model objects that touch Public Keys weren't more thoroughly reviewed.
If nothing else, folks everywhere will be thinking a little harder about authorization logic this week.
It was not, in Github's case. It seems like a glaring error in retrospect but it's easy to see how this code (or the pattern) would move over from the private to public-facing interface and not trigger any errors or notice.
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.
I imagine there are many, many sites that are vulnerable to this. I hope the high profile hack (at least on HN) quickly spreads the kind of panic that gets other devs to check their repos today.
For example, if you are doing this in your controller:
@user = User.new(params[:user])
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.
The only reason why a cracker hasn't tried this is because it seems too simple to work.
`attr_accessible` should only be used to protect the attributes that are NEVER modified by users. Access to the rest of the attributes may differ by user role, and should be handled by the controller. Trying to use `attr_accessible` to protect everything leads to enough frustration to make one eventually give up on security.
But web app developer may not be able to know in advance what new columns might be added on the database, possibly by some other team. If I understand this right, in the absence of attr_accessible, any new columns are completely writable by the HTTP request.
So having a default-deny whitelist approach is the only sane strategy.
Trying to use `attr_accessible` to protect everything leads to enough frustration to make one eventually give up on security.
Or give up on Rails. Usually the basic security of the database is not negotiable.
But wouldn't that mean, that the commit would display the username of the user with the `user_id' that he used?
So presumably it would have shown the rails developer pushing a change authored by Homakov.
config.active_record.whitelist_attributes = true
Also, this isn't the first time someone's been bit by this: http://www.kalzumeus.com/2010/09/22/security-lessons-learned...
https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac...
The sensible way to do updates, in my opinion, is
User.update(:name => params['user']['name'])
User.update(params['user'])Deleted comment