I hope those who ripped into Homakov initially are feeling more mellow towards him. They certainly were justified in thinking him being rash, but his action raised far more awareness about this issue than the typical proper channel.<p>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.<p>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."<p>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.<p>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.<p>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?<p>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.
Homakov kind of gave the impression he had discovered a previously unknown vulnerability in Rails. This is not the case. Rather, he discovered an instance in which one very prominent Rails app (Github) failed to implement a standard Rails security practice.<p>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 <i>mass assignment security.</i> If you fail to use this feature, you have a vulnerability. In other words, the <i>default</i> 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."<p>Homakov obviously disagreed with this design decision. I can understand why, and I mostly feel the same way.<p>So Homakov posted an issue to the Rails repo Github (<a href="https://github.com/rails/rails/issues/5228" rel="nofollow">https://github.com/rails/rails/issues/5228</a>) 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.<p>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.<p>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 <i>Github's</i> 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.<p>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.
Is it really frowned upon to do:<p><pre><code> user.name = params[:user][‘name’]
</code></pre>
?? Call me old fashioned, but this is called 'defensive coding' and should (in my opinion) be the norm when dealing with client-generated input. It might be more verbose and not 'The Rails Way', but update_attributes seems like too much magic for my paranoid taste.
It's surprising to me (not a very experienced Rails developer) that the default behavior should be so open to abuse.<p>Why isn't the default the opposite?
Just a heads up for those who only read the TL;DR: Please be aware that if you just put that single line of code in your initializers for your existing app, your app will break anywhere you are using update_attributes(). They do call it out later in the article, but you have to set attr_accessible on all your models.
Is it just me, or doesn't this sound very similar to SQL injection, only as applied to an ORM instead?<p>That is, if my understanding is correct, they're taking user posted data and trivially turning it into a command to update data.<p>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 must say that this episode is the best example of how to handle such cases.
Homakov found the issue and made his point without any sign of maliciousness.
Github, also handled is extremely professionally by accepting it and fixing the problem and then publishing a full report on it, rather than get into a pissing match with Homakov and getting law enforcement and lawyers involved.<p>BigCo's should take a note.
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.<p>The 'schematic' of what the public key update looks like from the original post:<p><pre><code> 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
</code></pre>
The correct way to code this is as follows:<p><pre><code> 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
</code></pre>
This has two updates to it that protect against this exploit:<p>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:<p>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.<p>It's really very simple: you don't let people access stuff they don't own.<p>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 posted a link to the relevant part of it (<a href="http://news.ycombinator.com/item?id=3665429" rel="nofollow">http://news.ycombinator.com/item?id=3665429</a>) on another thread regarding this exploit already, but the official Rails Security Guide covers this and other common security pitfalls really well. It is worth reading over thoughtfully if you are building a Rails app:<p><a href="http://guides.rubyonrails.org/security.html" rel="nofollow">http://guides.rubyonrails.org/security.html</a>
I'm not a Ruby or Rails guy, so I can't comment on how likely a typical (or atypically good) Rails dev might be to code this sort of bug into their app.<p>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.<p>If nothing else, folks everywhere will be thinking a little harder about authorization logic this week.
I'm a Python/Django developer and don't really know much Ruby or Ruby on Rails. Does anyone have an outsiders/non-rubyist explaination of how this hack was carried out? Did he modify HTTP headers? POST parameters?
I wonder how many Rails apps there are out there that is still vulnerable to this sort of flaw. Both GitHub and Posterous has fixed it, but there's probably thousands of smaller less known Rails sites/apps that still haven't been patched.
I find it hilarious that just in the last few days they've removed register_globals from PHP for good. It seems every language/framework ends up retracing the same set of security/convenience tradeoffs over and over again.
Since this vulnerability has been known about for years and experienced crackers have presumably checked for it before, is there any reason to be concerned about more malicious corruption of other repositories on github?
DHH does it in half a line: <a href="https://gist.github.com/1975644" rel="nofollow">https://gist.github.com/1975644</a><p>`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.
It's sad that this same bug was resolved in CakePhp <i>7</i>years* ago:
<a href="http://groups.google.com/group/cake-php/browse_thread/thread/03ce2bc624d335b9/eeb5f847369a74d1" rel="nofollow">http://groups.google.com/group/cake-php/browse_thread/thread...</a>
"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."<p>But wouldn't that mean, that the commit would display the username of the user with the `user_id' that he used?
I don't find this a good solution. A much better solution is to generate a list of form inputs that are present on a page, then when the form is submitted, check if any of the form inputs were changed / any were added.
Is the solution given in the article any different than using this?<p>config.active_record.whitelist_attributes = true<p>Also, this isn't the first time someone's been bit by this:
<a href="http://www.kalzumeus.com/2010/09/22/security-lessons-learned-from-the-diaspora-launch/" rel="nofollow">http://www.kalzumeus.com/2010/09/22/security-lessons-learned...</a>
The real culprit here is HashWithIndifferentAccess, a crutch that throws away a difference that Ruby has for a reason.<p>The sensible way to do updates, in my opinion, is<p><pre><code> User.update(:name => params['user']['name'])
</code></pre>
But there's no way in Rails to keep that syntax while disabling<p><pre><code> User.update(params['user'])</code></pre>
Am I the only one who does not understand what is this about? Oh, no, looks like rails team does neither.
Stupid code can be written in any framework/language. How much experience does one need to understand a simple rule - _never_ use user input directly.
If you have an urge to trust your users - I'd suggest better way:
`params[:command]`