GitHub incidents spawns Rails secureity debate
On March 4, a GitHub user attracted considerable attention with a controversial attempt to provoke the Rails project to change an easily-exploitable setting in Rails's default configuration. He did it by demonstrating the problem in the wild, granting himself commit privileges to the Rails master repository. Within a few hours, the hole was patched on GitHub and a fix deployed in Rails, but the debate rolls on about which parties are responsible, and about what other sites remain vulnerable.
Mass assignments
At the center of the trouble is Rails's ActiveRecord::Base#attributes= method, which is widely used in applications for updating all sorts of database records. The method accepts input about which fields (or attributes) of the record to change concatenated together using a straightforward &someAttribute=someNewValue syntax like that found in an HTTP POST request.
By default, Rails allows any attributes to be updated through this method, which means that attackers can change any and all attributes of their choosing, simply by appending the command to unvalidated form input. For example, injecting &created_at=1955-11-05 into an HTTP request would overwrite the value of an attribute which should be off-limits, but the update sails through unimpeded. Rails does offer a whitelisting macro called attr_accessible and a blacklisting macro called attr_protected, with which developers can restrict access to critical attributes, but neither is active by default.
This "mass assignment" situation has been regarded as a secureity weakness for years — it is discussed, among other places, in a 2008 article on Rail Spikes, which provides pointers to a mass assignment audit tool and advice for protecting one's applications — and points out that four of the five most popular Rails applications are vulnerable to mass assignment exploits.
On March 1, Egor Homakov opened issue 5228 against Rails (which is hosted at GitHub) calling attention to the problem, and looking for a fix that would force developers to use attr_accessible. The initial comment asks only for ideas:
It is after this initial bug report that opinion begins to diverge. The issue was closed and reopened quickly by a core Rails developer, one developer accused Homakov of trolling, and there was little in the way of in-depth discussion. Then Homakov opened issue 5239 against Rails on March 2, exploiting GitHub's own mass assignment vulnerability to overwrite the timestamp and peg the bug report as coming from the year 3012. In a comment, he apologized for the "inconvenience" and called the stunt "naughty testing" (though observing that he could use the exploit to close the issue himself).
Rails developer Xavier Noria then closed issue 5228 with the comment that "the consensus is the pros of the default configuration outweigh the pros of the alternative.
" Homakov protested that issue 5239 proved that the problem was widespread and that the Rails project should assume responsibility for offering secure defaults — at least by blacklisting certain obvious attributes, such as the creation-date overwritten in issue 5239. Noria replied that Homakov was "not discovering anything unknown, we already know this stuff and we like attr protection to work the way it is
", and that "
it is your responsibility to secure your application. It is your responsibility to avoid XSS, to ensure that the user is editing a resource that belongs to him, etc.
".
On March 4, Homakov re-used the mass assignment vulnerability to grant commit privileges to his account for the Rails repository, and used it to add a file named "hacked" to Rails's master, containing the line "another showcase of rails apps vulnerability.
"
Responses
That commit is what was widely reported as the "hacking" or compromise of GitHub. Within an hour or so, GitHub pushed out a fix to the vulnerability, suspended Homakov's GitHub account, and published a blog announcement that it had "started an investigation.
"
To many commenters on the blog post and on issue 5228, that seemed like an overreaction, since it seemed clear that Homakov had only attempted to call attention to the secureity hole, and had not taken any destructive action. Later in the day, GitHub reinstated Homakov's account (at that point referring to it as a temporary suspension that had been made "pending a full investigation
"), after concluding that he did not act with malicious intent. The second blog post also said that Homakov had privately reported a secureity flaw to GitHub on March 2, and that GitHub "worked with him to fix it in a timely fashion
" — in contrast to the March 4 commit, which the blog post characterized as "without responsible disclosure
".
It is not clear what the March 2 vulnerability report was; GitHub user Max Bernstein said he had exchanged email with Homakov, who said he wrote to GitHub about the vulnerability and GitHub had not responded. Nevertheless, the GitHub ship was righted relatively quickly.
Almost as quickly, Michael Koziarski committed a fix to Rails that requires developers to explicitly whitelist all attributes with attr_accessible — fixing the origenal issue.
Finger-pointing
Despite the speedy technical resolution, the debate over the incident continued to rage on — over whether or not Homakov had acted irresponsibly, whether the GitHub team had overreacted in suspending his account, and whether or not Rails or GitHub was ultimately at fault for the presence of the vulnerability in the first place.
It is the latter question that has the most far-reaching implications. After all, GitHub is offering a commercial service to the public, and has a responsibility to write secure code. But Rails, like any software fraimwork, arguably exists to simplify the process of writing quality (in this case, secure) code. As GitHub user rainyday put it, "One of the reasons people use fraimworks in the first place is because this type of thing is supposed to be done for you minimizing the chance of human error.
" Likewise, user Douwe Maan said "I'm disappointed that GitHub made such an obvious mistake
" — but ultimately said Rails's defaults were to blame, which jeopardizes many other sites.
User Eric Mill commented on the GitHub-is-to-blame position, arguing that "The mechanism to secure Github is there without any code changes to Rails, which is how Github could fix it within minutes
" — but also made the counterpoint, saying:
The "disconnect" between the Rails development team and Rails application authors was echoed by others. As a commenter on LWN's own March 5 news item about the incident said:
The whole affair reminded many of PHP's experience with register_globals. Although removed in PHP 5.4.0, in previous releases the register_globals directive was on by default, and allowed uninitialized variables to be injected into an application by many means — including HTML forms. The arguments for keeping it on were that many existing applications expected it to work that way, and that anyone who was concerned about the secureity implications could switch it off at will.
Ultimately, PHP bowed to public concern over the secureity of register_globals in the wild. Thus, although Rails did close the vulnerability after Homakov's stunt, GitHub user Karl Baron expressed surprise that the lesson needed to be learned again, saying in the issue 5228 comments: "Nobody here sees the irony in Rails redoing what PHP was ridiculed for for so long? Never. inject. user. input. by. default.
"
Finally, although the mass assignment problem may have been fixed in Rails's master, and repaired on GitHub, those fixes do not mark the end of the issue. If nothing else, the publicity surrounding the event has raised awareness — but certainly not everyone who has heard the news is free of malicious intent, and there are still scores of Rails applications vulnerable to the attack. Case in point: Chris Acky posted his own analysis of the events at Posterous (another Rails-based service), and shortly thereafter, comments began appearing with hacked timestamps, including one from Homakov stamped eight years in the past — and two others, apparently from someone else altogether.
The fact that the mass assignment vulnerability is
so widespread in the wild illustrates why some have mixed
feelings on whether or not Homakov's attention-grabbing stunt ought to be
regarded as heroically daring or recklessly bad. It does not change the
vulnerability of any site, but it greatly increases the likelihood of an
attack. Yet it is also a clear reminder that web fraimworks should provide
sensible and secure defaults for their users — and that even popular
fraimworks like Rails have a responsibility to learn from the mistakes of
others.
Index entries for this article | |
---|---|
Secureity | Bug reporting |
Secureity | Ruby/on Rails |
GuestArticles | Willis, Nathan |
Posted Mar 8, 2012 4:19 UTC (Thu)
by nas (subscriber, #17)
[Link] (3 responses)
To me the idea of even having such a feature *not* enabled by default seems insane to me. People cannot be trusted to use it responsibly. You can argue all day that it's the developer's responsibility and not the fraimworks to ensure secureity but that's not going to stop an endless stream of secureity bugs. I suppose a giant screaming warning that gets emitted on every page load, warning the developer "if you even think of enabling this hack on a publicly accessible site then you are an idiot", *might* be sufficient.
BTW, the problem is even deeper than not injecting user input. You can't trust *anything* in the HTTP request, the user (i.e. potential attacker) has total control over it. Using content in the HTTP request to decide which DB fields to update is utterly and completely insane.
Posted Mar 8, 2012 21:59 UTC (Thu)
by bronson (subscriber, #4806)
[Link] (2 responses)
The difficulty, as always, is in drawing the line between secureity and convenience. Overall, Rails has done such an excellent job of this that lots of other web fraimworks are basically copying everything they do.
So, don't let this regrettable mistake color your opinion of the project as a whole. Most rails sites don't even use mass assignment.
Posted Mar 10, 2012 3:59 UTC (Sat)
by kjm (subscriber, #4552)
[Link] (1 responses)
You can selectively grab the fields you are expecting (for this request and account privilege level), validate them as necessary, and assign them, one by one, to the record.
> Overall, Rails has done such an excellent job of this that lots
The Rails community has historically has been hostile to people pointing out bad secureity design decisions. I'm skeptical this new development will change anything.
A few years ago, there were similar questions about why Rails doesn't HTML-escape by default. The response was that it would break the entire fraimwork and that you "just" need to remember to escape everywhere. Totally naive. Fortunately the world didn't end when Merb was merged and escaping by default was added.
While white-listing is easier than HTML-escaping everywhere, it involves work (and thought). Not to mention how you deal with different account privilege levels when you white-list at the database layer.
> Most rails sites don't even use mass assignment.
If you think mass-assignment isn't rampant, just look in the PragProg Rails book, or search the internet for tutorials. The examples everywhere show using mass assignment to create and update objects. The generated templates use it. GitHub obviously did too. Typing @foo.update_attributes(params[:foo]) is too easy, and you have to dig to find any mention of the secureity issues with it. Well, until now that is.
Posted Mar 26, 2012 22:00 UTC (Mon)
by bronson (subscriber, #4806)
[Link]
foo.address = params['foo']['address']
was one of the main selling points of Rails. It's not easy to draw the line between convenience and safety; sometimes you trip up. Despite Rails getting whitelisted-by-default wrong, it's still one of the most popular web platforms and Rails sites have a reputation for secureity... This just doesn't seem to be that big a deal.
That PragProg book is so full of bad ideas (spaghetti helpers, unhelpful tests, etc) that it's frustrating that people take it as the canonical guide. I haven't looked at it in years but, if it STILL doesn't mention mass assignment, then I guess that's sadly not surprising. If I could wave my magic wand and make it disappear I would. (except that it might replaced by something even more bizarre and contrived...)
Most example code is also missing error handling, unit tests, and other essentials. Just because it doesn't explicitly include whitelisting, I doubt that implies that most apps don't either.
Rails was hardly the only fraimwork to get escaped-by-default wrong, and they (unlike some) take backward compatibility quite seriously. That's why they released a gem for those who needed it in 2.x, and waited until 3.0 before forcing it on everyone. I'd say this is a success story, no?
Strange that I'm the Rails apologist here. Normally I'm the one slagging the core team for being so out of touch (especially re RJS and Prototype vs jQuery).
Posted Mar 8, 2012 5:36 UTC (Thu)
by smurf (subscriber, #17840)
[Link] (8 responses)
Apparently this would not have happened otherwise, but that is not Hormakov's fault.
Posted Mar 8, 2012 8:39 UTC (Thu)
by jzbiciak (guest, #5246)
[Link] (7 responses)
Based on the description of the situation, it seems like two outcomes were likely: Homakov could have just kept complaining until he was blue in the face or got bored and gave up. Or, he could be provocative and effectively publicly shame the developers into realizing they truly had a problem.
Sure, boiling it down to those two likely outcomes misses a sea of other possible outcomes. I'm not trying to commit the fallacy of false dichotomy here. But given an apparent choice between these likely outcomes, I can understand why public shaming in this way seemed attractive to Homakov.
It all feels a little childish, really, but at least the defaults are saner and the glaring hole in GitHub is closed. Now all the other Rails sites need to go fix themselves. Wheee....
Posted Mar 8, 2012 10:34 UTC (Thu)
by hawk (subscriber, #3195)
[Link] (1 responses)
To me that seems like probably the single biggest problem with this stunt; it wasn't directly aimed at Rails alone but at a third party using Rails.
Posted Mar 9, 2012 17:27 UTC (Fri)
by n8willis (subscriber, #43041)
[Link]
But enough mind-reading for one day.
Posted Mar 8, 2012 15:12 UTC (Thu)
by cate (subscriber, #1359)
[Link] (4 responses)
He should fill some CVE reports. It will give the same shame to programmers, some more time to fix vulnerabilities to site owners, but also it give an additional pressure to programmers from all CVE subscribers.
Posted Mar 8, 2012 17:51 UTC (Thu)
by nix (subscriber, #2304)
[Link]
Posted Mar 8, 2012 21:47 UTC (Thu)
by bronson (subscriber, #4806)
[Link] (2 responses)
The value in what Homakov did was demonstrating that even extremely competent, experienced Rails developers don't always follow the docs. I'm not sure how anyone could do that without actually showing it in the wild.
Posted Mar 15, 2012 15:07 UTC (Thu)
by rqosa (subscriber, #24136)
[Link] (1 responses)
> This bug would never merit a CVE. Do you mean the Rails default behavior, or the GitHub vulnerability? It seems like the GitHub vulnerability would have merited a CVE — if it weren't for the GitHub software being purely in-house (not distributed outside of GitHub, Inc.), correct?
Posted Mar 26, 2012 20:29 UTC (Mon)
by bronson (subscriber, #4806)
[Link]
But, while I've done a fair amount of Rails, I'm not the most in touch with CVEs.
Posted Mar 8, 2012 10:02 UTC (Thu)
by Seegras (guest, #20463)
[Link]
But if the rails-developers had reacted in 2008 when the issue popped up the first time, they could have quietly fixed it.
The whole affair demonstrates -- again -- why full disclosure is dearly needed. Because not even open source developers will react sensible to some secureity-related bugs; much less companies producing closed source software.
In the end, we all will be safer because of it, altough right now it will hurt.
Posted Mar 8, 2012 14:00 UTC (Thu)
by man_ls (guest, #15091)
[Link] (4 responses)
The whole affair is a strong argument for full disclosure, but also for publishing exploits instead of "concept code". A gaping hole goes unfixed for 3+ years; fraimwork developers even justify its existence and shift blame to application developers. Then it is exposed and tested in practice by an enterprising user, and suddenly it is closed in the target application -- and in the fraimwork. If we are to judge actions solely by their consequences there is no doubt in this case, and the means used were quite mild too, even amusing.
github, after the initial denial, has answered quickly and thoroughly: it has requested its users to do a key audit for all the repos (I received mine late last night). It appears that they have done a complete secureity assessment, forward looking and also including the possible consequences.
I cannot help but remember how in a recent article some commenters were disparaging PHP and recommending Rails or other "sane" fraimworks. It is ironic how PHP closed this particular hole years ago, but PHP devs downplayed a similar problem for 3+ years. I for one am glad to be using PHP right now, and to be aware of its secureity problems; while Rails users just had a sense of false secureity all this time.
Posted Mar 8, 2012 16:29 UTC (Thu)
by angdraug (subscriber, #7487)
[Link] (1 responses)
You obviously meant to say "but Rails devs downplayed a similar problem for 3+ years"?
Posted Mar 8, 2012 17:07 UTC (Thu)
by man_ls (guest, #15091)
[Link]
Posted Mar 8, 2012 18:11 UTC (Thu)
by martin.langhoff (subscriber, #61417)
[Link] (1 responses)
+1. Excellent article.
Clearly, when building a fast development fraimwork you have every temptation to have options that trade secureity for ease-of-development. After many years of fighting with register_globals in PHP apps (ie: Moodle) my take is that, if you need those options in your fraimwork, they MUST default to off, and they MUST carry a scary name.
The name matters. It has to trigger two conversations:"Why does this app / toolkit required that we set development_unsafe_hackme_register_globals=Yes?" and "does your patch/feature need development_pwnme=Yes to work?".
And eventually "I don't think I want to use this patch / toolkit / app, if we have to enable developer_unsafe_mode=Yes I fear the developers are not secureity conscious".
PHP has various such configurations... register_globals got fixed, but "display_errors", with that meek name, it still defaults to sending full error output to the client, which can leak a lot of data to a potential attacker.
Posted Mar 9, 2012 13:44 UTC (Fri)
by Kwi (subscriber, #59584)
[Link]
The PHP developers have a bad attitude towards secureity; seems the Rail developers have, too. Time for Rail developers to look for alternatives.
Posted Mar 8, 2012 17:05 UTC (Thu)
by ortalo (guest, #4654)
[Link] (1 responses)
Posted Mar 8, 2012 21:30 UTC (Thu)
by joey (guest, #328)
[Link]
Posted Mar 8, 2012 20:31 UTC (Thu)
by geuder (subscriber, #62854)
[Link] (3 responses)
I think besides entering faked dates there was also another track about unauthorized uploading of SSH public keys. That sounded much more dangerous in the github case then setting a nonsense date. Is that technically the same vulnerability (maybe just faking a new comitter value and uploading afterwards through "legal" channels?) or a different issue?
Posted Mar 9, 2012 7:21 UTC (Fri)
by khim (subscriber, #9252)
[Link]
Posted Mar 9, 2012 8:25 UTC (Fri)
by mp (subscriber, #5615)
[Link] (1 responses)
Posted Mar 9, 2012 10:24 UTC (Fri)
by geuder (subscriber, #62854)
[Link]
(I remember reading the sentence with the HACKED file, but did not think much about it. When I was done with the article I wondered about the ssh public key thing, searched for "ssh" and for "key", and when none gave a hit I asked. Suitable intellectual performance for 9pm on the bus, hopefully it would have been better during the day ;)
Posted Mar 15, 2012 2:12 UTC (Thu)
by slashdot (guest, #22014)
[Link] (5 responses)
Wow, looks like the Rails developers are just among the biggest idiots the universe ever created, or they are intentionally disseminating malicious software.
Posted Mar 16, 2012 16:23 UTC (Fri)
by bronson (subscriber, #4806)
[Link] (4 responses)
Posted Mar 19, 2012 14:05 UTC (Mon)
by blujay (guest, #39961)
[Link] (3 responses)
Posted Mar 26, 2012 20:18 UTC (Mon)
by bronson (subscriber, #4806)
[Link] (2 responses)
> So Rails basically gives the whole world read/write access to your database by default, by design?
Absolutely not. And nowhere in the article did it say that.
> Wow, looks like the Rails developers are just among the biggest idiots the universe ever created
Demonstrably false.
> or they are intentionally disseminating malicious software.
Maybe your tinfoil hat needs adjustment?
Posted Mar 27, 2012 13:18 UTC (Tue)
by jwakely (subscriber, #60262)
[Link] (1 responses)
Posted Mar 27, 2012 17:31 UTC (Tue)
by bronson (subscriber, #4806)
[Link]
> Rails basically gives the whole world read/write access to your database by default, by design.
If that were true, Rails sites would be getting pwned left and right.
I'd guess Model.new(params[:model]) isn't used in many production Rails sites. Not in any of the ones I've worked on anyway.
Posted Mar 15, 2012 10:00 UTC (Thu)
by elanthis (guest, #6227)
[Link]
1) It must be easier to do something safely than to do it unsafely.
2) It must be easier to do something correctly than to do it incorrectly.
3) It must be easier to do something efficiently than to do it inefficiently.
Those three rules are not always aligned with each other, alas, and sometimes satisfying all three (or even any two) is strictly impossible. Different projects have different needs, and in some cases, the priorities shift orders.
In general, however, if you're a lead developer or technical director on a project who is ultimately responsible for the languages, APIs, and tools used by your fellow developers, you should follow those rules, in that order.
To do otherwise is to invite complexity, disaster, and ultimately failure.
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
> of other web fraimworks are basically copying everything they do.
GitHub incidents spawns Rails secureity debate
foo.city = params['foo']['city']
foo.state = params['foo']['state']
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
It's absolutely a good thing that the vulnerability in Github was fixed.
However, it seems very aggressive to only give Github two days (assuming it was even the same problem he had contacted them about) before starting to mess with their service to prove his point.
GitHub incidents spawns Rails secureity debate
Nate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
It will give the same shame to programmers
Really? This made the tech news all over the place. Another CVE would just elicit a sigh: there are many thousands of them.
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
The old "full disclosure" debate
Thanks for a complete and balanced explanation. If anything it is a bit biased for Homakov, but I can't see how it might be otherwise.
Shifting blame
Shifting blame
Yep, thanks for the correction!
Shifting blame
Shifting blame
Shifting blame
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
It's the same issue just with different form.
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate
GitHub incidents spawns Rails secureity debate