from Hacker News

Hacked: commit to rails master on GitHub

by stwe on 3/4/12, 5:26 PM with 225 comments

  • by wycats on 3/4/12, 6:10 PM

    Here's my proposal for improving the situation: https://gist.github.com/1974187

    Merb's approach was to have mass assignment protection in the controller, and I personally think it's self-evident that it belongs there. Moving it into the controller will also make it easier to solve the tension between reducing the friction of getting up and running quickly and having good security defaults.

    In general, Rails' convention over configuration make a stock Rails app more secure by default (CSRF protections, XSS protection, timing attacks, session fixation, etc.). This is a case where there's a real tension, but I think that we can solve it by applying some brainpower to the question.

  • by teej on 3/4/12, 7:53 PM

    Posting it as an issue on the Rails repo and then exploiting GitHub with it is a great way to get attention, but not necessarily the most responsible.

    I disclosed a vulnerability to GitHub before. I dropped it into their Issues system marked private with the heading "URGENT". It was a Sunday and I got a response + a fix from Tom Preston-Wener himself within a few hours. That, in my mind, would have been a more responsible approach.

  • by danmaz74 on 3/4/12, 9:29 PM

    I would say that homakov's angry and not very mature reaction to his warning being ignored just did a very big favor to a lot of rails developers, that reading about his exploit on HN (and other places) will rush to check their websites and will fix a LOT of serious vulnerabilities they didn't have any idea they had. But which somobody could have already been secretly exploiting.

    Understandably, Github would have liked much more to be one of those companies that will be able to quietly fix this vulnerability without anybody knowing, but now that the damage to their image is done I really hope they'll not add to that damage by persisting in their banning of a 18 year old that acted irresponsibly yes, but maliciously - definitely not.

    From a PR perspective, I guess that having titles like "Silicon Valley company rewards Russian teenager who helps them eliminating a security risk" could even spin the episode in their favor.

  • by shearn89 on 3/4/12, 6:39 PM

    This is clearly a problem: with Rails' approach being 'right from the start', having no protection by default is not the right way to do it. This issue may be well known among the type of people that use github and read HN, but if someone had read about Rails being an awesome framework to make db-driven websites, they might not be aware of such a thing as a "mass assignment vulnerability."

    If by adding a line or 2 to the code for generators can stop this, even if it includes a comment saying "Removing this line will do x y z", then I think the rails team could've treated the bug with a little more respect.

    As @ericb said, if strong devs make this mistake, there's something wrong with the code.

    I think it should also be noted that he didn't do anything malicious like trash repos, and even says on his blog:

        "Then I could wipe any post in any project. That wasn't that funny but pretty
    dangereous[sic]. It got more curious."
    All he did was add a 3 line file to the master repo of a project that he was frustrated with. It generated all this attention, and will probably make them rethink the approach...

    Finally: big props to the GitHub team for patching their vulnerability in <1hr on a Sunday...

  • by holman on 3/4/12, 6:04 PM

    We've patched and fixed this on GitHub.

  • by bretthopper on 3/4/12, 6:26 PM

    Everyone might as well take this opportunity to add attr_accessible to your models.

    Models: find app/models -type f -name \*.rb | wc -l

    Models with attr_accessible: grep -r -m1 "attr_accessible" app/models | wc -l

    If those numbers aren't the same, and the missing model files inherit from ActiveRecord::Base, then look into adding attr_accessible.

  • by mojombo on 3/4/12, 8:33 PM

    I have written a blog post outlining the exploit and our mitigation procedure: https://github.com/blog/1068-public-key-security-vulnerabili...

  • by sad_panda on 3/4/12, 9:03 PM

    This is a pretty huge security issue with wide-reaching implications. People everywhere pull and compile from master branches on Github without second thoughts. It's a big hole. But everybody's running defense for GitHub.

    Contrast this with the enormous hue and cry against FB, MS, et al. when they have comparatively minor holes in their systems.

    I'm not saying that we need to tar and feather GH, but we should at least be equal-opportunity in our condemnations and realize that everybody is capable of mistakes. So, if you're OUTRAGED about Apple making a minor boo-boo, you should be equally outraged about this.

  • by teyc on 3/4/12, 11:41 PM

    I love the delicious irony when all sorts of rails dev argue for status quo only to find their master hacked because the source control system has the same vulnerability.

  • by patrickaljord on 3/4/12, 6:25 PM

    Funnily, the first Diaspora release had the same issue and the devs were ridiculed and called noobs by a big part of the HN community and security "experts" wrote big posts about it. The different reaction here is interesting to say the least.

  • by hundredwatt on 3/4/12, 10:38 PM

    I threw together a quick 'n dirty Rails generator that will generate the code for white/black listing all model attributes with attr_accessible/attr_protected.

    Here's the file: https://gist.github.com/1975167, just add to lib/generators in your Rails 3 app, then do rails g mass_assignment_security -h

    Hopefully others find this helpful

  • by wandernotlost on 3/4/12, 11:49 PM

    This is basic security, folks: never drive your application from stuff that comes over the wire (or any untrusted channel). Passing a params array directly to update_attributes is a fundamentally flawed approach for this reason. Instead, inspect incoming data for exactly what you expect to be there. By doing this, malformed input will either fail or be ignored without exposing a security vulnerability.

    It should be obvious that you can't anticipate every potential attack vector at design time. Therefore, a well-designed system is one for which, when expected or normal conditions are not met, the resulting action is nothing or error, not an unexpected action.

    This principle is also known as fail-safe: http://en.wikipedia.org/wiki/Fail-safe

  • by arturadib on 3/4/12, 5:45 PM

    I'm confused. Is this a generic Github vulnerability or is this a vulnerability in tools outside of Github used by Rails? The 'hacker' seems to suggest it's the former ("Github pwned"), which would be pretty serious stuff.

  • by codesuela on 3/4/12, 9:59 PM

    I don't get how many of you seem to take this issue lightly. Imagine what would've happened if this guy was a black hat. I use github for private code hosting and this a definite breach of trust and if I don't trust github how can I pay them? Sure they fixed the issue within an hour but still this could have ended far worse.

  • by Estragon on 3/4/12, 9:36 PM

    So, the vulnerability was public for at least days in homakov's bug report, and probably for years to anyone who wanted a crack at github badly enough to do a little research. Is it paranoid to worry about malicious commits in other important github repositories?

  • by nate on 3/4/12, 6:21 PM

    Shouldn't rails by default protect belongs_to associations. There is probably a minute number of cases where someone wants mass assignment changes to include the parent's id of that record.

  • by raggi on 3/4/12, 10:47 PM

  • by bbeausej on 3/4/12, 11:05 PM

    How can you be sure Homakov is the first person to notice GH was vulnerable if the vulnerability was present for many years in the codebase?

  • by tlogan on 3/5/12, 12:48 AM

    Can somebody explain why this is a Rails bug?

    Meaning using mass assignment is very similar to SQL injection: you pass variables from user input directly to model without even verifying them. Duh?

    Now regarding GitHub: yes there is a security hole and they fixed it.

    However, hacking a site after finding its vulnerability is definitely illegal and hope there will be consequences. And he did not even report a problem to GitHub.

  • by tvon on 3/4/12, 8:13 PM

    Nothing makes you look like more of a jackass then throwing gifs into rails commit comments.

  • by deanpcmad on 3/4/12, 10:25 PM

    How has he hacked Github? He's a contributor on rails/rails see here and search for homakov - https://github.com/rails/rails/contributors

  • by comechao on 3/4/12, 7:06 PM

  • by orblivion on 3/4/12, 8:34 PM

    > "Since you can commit to master, you could just fix the vulnerability :) "

    I like it, this would be a great way to be snarky and semi-responsible at the same time.

  • by mvasilkov on 3/4/12, 9:03 PM

    IMO this attitude of GitHub is the best motivation to sell 0day exploits in private instead of ever trying to get dev's attention.

    No, I mean really, "malicious attack". I can't help but laugh, he committed 3 lines of text grand total, this is what you call malicious? Seriously, WTF.

    The guy is a proper white-hat hacker, even if somewhat childish, Y U ban him.

  • by zyfo on 3/4/12, 5:49 PM

    Here's the guy's blog post about the hack: http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.htm...

    "Today I can pull/commit/push in any repository on github. Jack pot."

  • by javascriptlol on 3/4/12, 8:50 PM

    Even a genius eventually makes a mistake. Systems should be safe by default unless there's a good reason (e.g. performance). Why bother with high level tools if they're not even protecting you from mistakes?

  • by shearn89 on 3/4/12, 6:44 PM

    Also just noticed this at the bottom of his resume (http://homakov.blogspot.com/p/service.html):

        "<s>Discount for girls</s>"

  • by apl on 3/4/12, 5:57 PM

    It's unfortunate that the guy stumbling upon this apparently noteworthy vulnerability happens to be so utterly immature. [EDIT: Unsurprisingly, the dude's 18. See http://homakov.blogspot.com/p/about-me.html for reference.]

  • by klodolph on 3/4/12, 5:49 PM

    If this is a GitHub exploit, and I were GitHub, I would be talking to law enforcement. This is not how adults disclose software vulnerabilities.