8 ms·

Hacked: commit to rails master on GitHub

655 points by stwe 15y ago

mojombo 15y ago
I have written a blog post outlining the exploit and our mitigation procedure: https://github.com/blog/1068-public-key-security-vulnerability-and-mitigation https://github.com/blog/1068-public-key-security-vulnerabili...
- white_devil 15y ago
  So you're actually proud of writing that sleazy piece of damage-control distraction bullshit? You're completely misrepresenting what happened. Someone pointed out that Rails makes all Rails applications astoundingly insecure by default since forever, and got condescendingly dismissed several times by the people in charge. He then proceeded to make a point by demonstrating the severity of the security flaw, and you made him look like some malicious hacker that got swiftly punished by the ever-vigilant GitHub team. Public Key Security Vulnerability, really? You detected the attack and expunged the unauthorized key? What a load of bullshit.
- teyc 15y ago
  Until proven otherwise, all code hosted on GitHub must be assumed tainted, where potential 0-day has been inserted. You should consider revealing a history of all public key changes for each project (assuming you still retained apache logs) so that people can decide for themselves how much work they have ahead of them to re-audit their past commits.
arturadib 15y ago
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.
- atambo 15y ago
  I think it's a mass assignment vulnerability: https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228
  - deleted 15y ago
    [deleted]
    - atambo 15y ago
      He did: http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.html http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.htm...
  - pyre 15y ago
    Relevant: What I want you to see in that thread I mentioned is the way the core team perceives this. You are not discovering anything unknown, we already know this stuff and we like attr protection to work the way it is. Looks like this guy got really frustrated with the Rails devs basically saying that he didn't know what he was talking about. This reminds me of all of the unsafe defaults that PHP used to have. Same justification too, "it's a config setting, so it's up to the developer/sysadmin to read the docs and set them right."
    - mokus 15y ago
      This justification seems especially odd to me since Rails did so much in the first place to popularize the idea that the default behavior should be the one most likely to be "right". Don't they (or didn't they at one time) have a mantra "convention over configuration"? I guess that doesn't apply to security.
      - vidarh 15y ago
        This also makes it all the more serious. As the PHP developers found out the hard way: When you make it really easy to get started, a lot of people won't learn the system in depth enough to understand all the issues because they don't need to in order to make it work "well enough" for most cases. By making Rails so easy to get started, they pretty much guarantee that there's going to be a ton of developers that don't pick up on, or forget, that they need to deal with issues like this. That even a site like Github was vulnerable to this demonstrates just how seriously wrong it is to pick a default like this..
zyfo 15y ago
Here's the guy's blog post about the hack: http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.html http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.htm... "Today I can pull/commit/push in any repository on github. Jack pot."
- arturadib 15y ago
  Regardless of the 'hacker's motives/personality, I think this is yet another testimony to the power of open source. When you have this many eyeballs looking at your code, the odds of a good-intentioned (however playful/immature) coder to discover a vulnerability is much greater than those of a real ill-intentioned hacker simply due to the sheer number of the former. The issue will quickly get fixed by the community, the kid will get the attention he wants, and life will go on.
  - boundlessdreamz 15y ago
    Hmm? The vulnerability is in github's code (not open source) and not in rails.
    - eddington 15y ago
      It actually looks like the vulnerability is on Rails itself, which Github is built on...
    - nknight 15y ago
      The vulnerability is that Rails is insecure by default. That used to be the case for a lot of things, then finally people noticed how the real world works, and started fixing them. Apparently the Rails developers have actively resisted the lesson everyone else already learned.
      - boundlessdreamz 15y ago
        The vulnerability is still Github's. Rails provide the tools to do this right. Whether rails should provide stricter defaults is another question altogether. I was replying to the parent, who attributed this to the power of "open source & eyeballs looking at your code" but this is not such an instance.
klodolph 15y ago
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.
- psadauskas 15y ago
  One of the first comments is a link to where he tried, but the issue was repeatedly closed.
  - phillmv 15y ago
    He submitted it to… the rails bug tracker. Three days ago. That's not disclosing the issue to GitHub at all.
    - BenjaminCoe 15y ago
      It's a pretty insane leap of logic to maliciously attack the Github website to prove your point regarding a framework hosted on that website. Politely contacting people about the vulnerability, rather than pulling that crap in an extremely public venue seems like the more mature, and less incredibly illegal way to go about things.
      - jsnell 15y ago
        The "attack" clearly wasn't malicious, though probably immature. A malicious attacker would have been doing things like gaining access to users' private repos and stealing the code, or trying to sneak in harmful commits to a repo under a false name. This was at most a prank or a demonstration. So why this instead of responsible disclosure? The problem is that it was not really a GitHub issue, it's Rails having a grossly insecure default setting. According to one of the comments in the bug tracker a lot of other high-profile sites also have the same problem. And presumably new ones would keep popping up for as long as Rails is the new hotness. So disclosing the problem to GitHub would not solve anything. They'd deploy the fix, but a lot of other sites remain vulnerable. That's probably the case even if GitHub were willing to take a PR hit and admit they'd been insecure for a long time in order to spur other Rails users to fix their code. After all, they hadn't fixed their code after the previous widely published security problems caused by the same underlying issue. Clearly the Rails core team were not willing to consider any kind of changes to improve the situation. As such, you can argue that making as big a scene as possible is the best way to improve security globally. It's of course unfortunate for GitHub that he chose to use that site as the example due to the obvious reasons. And certainly the timing is about as unfriendly as possible from the point of view of a west coast person.
apl 15y ago
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 http://homakov.blogspot.com/p/about-me.html for reference.]
- 5vforest 15y ago
  On the other hand, this makes for a hilarious perusal of his blog.
- rplnt 15y ago
  Well... > Why I do this? Since guys in rails issues ingored me But I guess I agree.
- deleted 15y ago
  [deleted]
deleted 15y ago
[deleted]
holman 15y ago
We've patched and fixed this on GitHub.
- arturadib 15y ago
  Time from exploit to fix in production: < 1h. On a Sunday morning. I'd buy Github stock if I could.
  - pyre 15y ago
    It doesn't hurt that the vulnerability was already discussed in a bug report by the 'attacker.' I'd imagine it would have taken a longer time had they needed to track down how exactly this happened, though I guess I'm making an assumption on what their logging/auditing/reporting is like.
    - jberryman 15y ago
      Right, I'm kind of baffled by the posters on the bug report calling this a "0-day" exploit.
  - sausagefeet 15y ago
    At the very least you'd probably be getting it on the cheap at that point.
- lucaspiller 15y ago
  Good work. Please could you write a post explaining what you did, so others using Rails can do the same?
  - momoro 15y ago
    They probably simply made user_id protected on whatever table controls commit permissions.
- bgentry 15y ago
  Do you plan on notifying customers whose repos he accessed? This looks like more than just a minor breach if he was able to give himself admin rights and had read/write access to any repo.
deleted 15y ago
[deleted]
wycats 15y ago
Here's my proposal for improving the situation: https://gist.github.com/1974187 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.
- bradly 15y ago
  > Merb's approach was to have mass assignment protection in the controller, and I personally think it's self-evident that it belongs there. But wasn't Merb merged into Rails? :) Sigh...
  - wycats 15y ago
    Not every idea from Merb made it into Rails. Especially when an idea would cause significant backwards-compatibility breakage (return string from action vs. implicit rendering), we stuck with the Rails approach. I always felt that "it's up to the developer to do the right thing" violates the normal Rails convention over configuration principles, but I also weigh breaking a large % of existing Rails apps in a way that is not easy to quickly fix heavily. That said, this problem is almost identical to XSS protection. We were able to find a solution that mostly "just works" for new developers, with some caveats, but it broke nearly all existing apps in a way that required significant effort to fix. Like mass assignment, previous vulnerabilities were caused by Rails defaults that caused most users to make mistakes (nearly everyone had at least a few cases where `h` was required but wasn't done). Like XSS protection, we have a solution here that will mostly just work for the happy path. The end result is a Rails default that will be only marginal harder to use than what we have now, but secure by default.
    - bradly 15y ago
nate 15y ago
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.
- nfm 15y ago
  In my uses of rails, belongs_to associations get changed frequently. Any time anything gets 'assigned' to something, this occurs: assigning something to a user, to a milestone, to a plan, to an account, maybe even to a priority. YMMV but I'd say this isn't minute. That said, there are certainly places in my apps where I don't want this to occur. And whitelisting is much better than blacklisting!
  - nate 15y ago
    And you're using mass assignment to do those assignments?
    - nfm 15y ago
      Nah, just changing the _id column's value so that the belongs_to association changes.
      - nate 15y ago
        "There is probably a minute number of cases where someone wants mass assignment changes to include the parent's id of that record" So it sounds like you aren't using mass assignment to change parent ids either. Most people don't. Most people use mass assignment to change the attributes of an object, not that objects association to a parent.
patrickaljord 15y ago
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.
- lgeek 15y ago
  In the case of Diaspora it was a less subtle and more obvious issue. They just weren't checking for authorization, as in: def destroy @album = Album.find_by_id params[:id] @album.destroy [...] end when the second line should have been something like: user.albums.find_by_id params[:id] But, well, they're both pretty bad mistakes.
  - zdouglas 15y ago
    Palpability is in the eye of the beholder. There's a lot of talk in the GH issue comments dismissing GH's "obvious" failure of not using `attr_accessible`.
  - patio11 15y ago
    Nope, Diaspora also had this exact issue, which would let you use anything where a params hash updated a user model to e.g. overwrite their credentials or encryption keys. The specific exploitable example I found would have let you do it even if they had been checking authorization to update objects properly, because the attacker could reassign his own objects as the victim's objects with arbitrary attack payloads, one of which being sufficient to compromise the victim's account. When I wrote a journal article about it my recommendation was that Rails ship with ActiveRecord::Base.attr_accessible(nil) by default, because otherwise vulnerabilities of that nature were virtually inevitable.
- bascule 15y ago
  Ensuring input is properly validated is always the developer's concern. A framework can make that easier for you, but especially in cases like this, which are basic user authentication concerns, it is ABSOLUTELY the developer's responsibility to be 100% sure how all of those abstractions work and ensure there are no leaks anywhere. You can't punt on security. Abstractions may make it easier, but you better be damn certain how they work.
bretthopper 15y ago
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.
- zwily 15y ago
  We included a spec in our rails app that inspects all AR models and errors if any don't specify any attr_accessible (with a whitelist of models to ignore). This catches anything included by plugins too, which can be helpful. https://github.com/instructure/canvas-lms/blob/9b52a51b6a37efa5164f7f636dd8e7478dd0eb77/spec/models/general_model_spec.rb#L46 https://github.com/instructure/canvas-lms/blob/9b52a51b6a37e...
- eval 15y ago
  This is definitely a first step. The thing is that with 'attr_accessible' a lot of interaction with your models gets harder (think testing or working from the console). I tried fixing this with introducing roles that have access to all attributes. Source at https://github.com/eval/sudo_attr_accessibility https://github.com/eval/sudo_attr_accessibility
shearn89 15y ago
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...
shearn89 15y ago
Also just noticed this at the bottom of his resume (http://homakov.blogspot.com/p/service.html http://homakov.blogspot.com/p/service.html): "<s>Discount for girls</s>"
comechao 15y ago
He did the same with Tower: https://github.com/rails/rails/commit/b83965785db1eec019edf1fc272b1aa393e6dc57 https://github.com/rails/rails/commit/b83965785db1eec019edf1...
teej 15y ago
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.
- ceejayoz 15y ago
  The point, though, is that this is more of a Rails issue than a Github issue. If Github gets it wrong, others are likely to.
  - rsynnott 15y ago
    Well, it's a beginner-level Rails mistake; it is not precisely an obscure issue. Googling 'attr_accessible' will show you discussion going back years, and it has been actively exploited before. I'm shocked that the mistake was made in Github, though.
    - thibaut_barrere 15y ago
      It's a beginner-level Rails mistake but it's very widely seen in codebases: it's a real, common threat.
      - acqq 15y ago
        Obviously not beginner-level if more famous sites suffer from the same problem -- Egor mentions some in his posts and he is only one person -- imagine what all the black hats can do working in parallel.
        Blaisorblade 15y ago
        By your reasoning, since tons of C programs can't use strcpy() correctly and thus contain buffer overflows, it takes a C master to use strcpy() correctly. I'd say instead that the kind of reasoning is wrong and that even experts can make beginner-level mistakes (not so often). But these are the kind of mistakes a good framework/language/etc. should try to prevent - especially if security-related. Tons of research tries to reduce error-prone programming activities.
tvon 15y ago
Nothing makes you look like more of a jackass then throwing gifs into rails commit comments.
orblivion 15y ago
> "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.
- psquid 15y ago
  Yeah, it seems to me that committing a fix with a commit message along the lines of "Oops, you left this open. Don't worry, though, I fixed it." would've been the best option.
  - muyuu 15y ago
    Technically it would be more like: "I just set it to safer defaults, since you guys won't. By the way, if your defaults are not strict enough for Github - where you trusted to host your code - gets them right, that pretty much makes them unsafe defaults."
javascriptlol 15y ago
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?
sad_panda 15y ago
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.
unseen 15y ago
This sounds like a "register_globals" feature. Are people still that obtuse to basic security?
mvasilkov 15y ago
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.
danmaz74 15y ago
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.
- masklinn 15y ago
  > I would say that homakov's angry and not very mature reaction to his warning being ignored Not just ignored but dismissed multiple times. It's not exactly surprising that he went and injected code into Rails's master.
Estragon 15y ago
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?
codesuela 15y ago
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.
deleted 15y ago
[deleted]
deanpcmad 15y ago
How has he hacked Github? He's a contributor on rails/rails see here and search for homakov - https://github.com/rails/rails/contributors https://github.com/rails/rails/contributors
- masklinn 15y ago
  Being a contributor does not mean he has push access to the rails master, it just means one of his patches/pull requests got accepted into Rails in the past. In fact, it's even better: the commit he pushed into Rails's master through the exploit would have automatically added him as a contributor even if he had not already been one.
- jroll 15y ago
  The point of his hack was to _make_ him a contributor to rails.
hundredwatt 15y ago
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 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
deleted 15y ago
[deleted]
raggi 15y ago
http://speakerdeck.com/u/holman/p/ruby-patterns-from-githubs-codebase?slide=33 http://speakerdeck.com/u/holman/p/ruby-patterns-from-githubs... nuff said.
bbeausej 15y ago
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?
teyc 15y ago
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.
wandernotlost 15y ago
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 http://en.wikipedia.org/wiki/Fail-safe
tlogan 15y ago
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.
- teyc 15y ago
  Can somebody explain why this is a Rails bug? Insecure by default. Microsoft used to be the laughing stock because the default install was vulnerable to exploits. While one might argue that Github devs should have known better, the counterargument is that if Github devs couldn't get it right, think about the thousands of people trying out rails for the first time, building their little web app.

655 points by stwe 15y ago

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

white_devil 15y ago
So you're actually proud of writing that sleazy piece of damage-control distraction bullshit? You're completely misrepresenting what happened. Someone pointed out that Rails makes all Rails applications astoundingly insecure by default since forever, and got condescendingly dismissed several times by the people in charge. He then proceeded to make a point by demonstrating the severity of the security flaw, and you made him look like some malicious hacker that got swiftly punished by the ever-vigilant GitHub team. Public Key Security Vulnerability, really? You detected the attack and expunged the unauthorized key? What a load of bullshit.
teyc 15y ago
Until proven otherwise, all code hosted on GitHub must be assumed tainted, where potential 0-day has been inserted. You should consider revealing a history of all public key changes for each project (assuming you still retained apache logs) so that people can decide for themselves how much work they have ahead of them to re-audit their past commits.

arturadib 15y ago

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.

atambo 15y ago
I think it's a mass assignment vulnerability: https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228
- deleted 15y ago
  [deleted]
  - atambo 15y ago
    He did: http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.html http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.htm...
- pyre 15y ago
  Relevant: What I want you to see in that thread I mentioned is the way the core team perceives this. You are not discovering anything unknown, we already know this stuff and we like attr protection to work the way it is. Looks like this guy got really frustrated with the Rails devs basically saying that he didn't know what he was talking about. This reminds me of all of the unsafe defaults that PHP used to have. Same justification too, "it's a config setting, so it's up to the developer/sysadmin to read the docs and set them right."
  - mokus 15y ago
    This justification seems especially odd to me since Rails did so much in the first place to popularize the idea that the default behavior should be the one most likely to be "right". Don't they (or didn't they at one time) have a mantra "convention over configuration"? I guess that doesn't apply to security.
    - vidarh 15y ago
      This also makes it all the more serious. As the PHP developers found out the hard way: When you make it really easy to get started, a lot of people won't learn the system in depth enough to understand all the issues because they don't need to in order to make it work "well enough" for most cases. By making Rails so easy to get started, they pretty much guarantee that there's going to be a ton of developers that don't pick up on, or forget, that they need to deal with issues like this. That even a site like Github was vulnerable to this demonstrates just how seriously wrong it is to pick a default like this..

zyfo 15y ago

Here's the guy's blog post about the hack: http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.html http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.htm... "Today I can pull/commit/push in any repository on github. Jack pot."

arturadib 15y ago
Regardless of the 'hacker's motives/personality, I think this is yet another testimony to the power of open source. When you have this many eyeballs looking at your code, the odds of a good-intentioned (however playful/immature) coder to discover a vulnerability is much greater than those of a real ill-intentioned hacker simply due to the sheer number of the former. The issue will quickly get fixed by the community, the kid will get the attention he wants, and life will go on.
- boundlessdreamz 15y ago
  Hmm? The vulnerability is in github's code (not open source) and not in rails.
  - eddington 15y ago
    It actually looks like the vulnerability is on Rails itself, which Github is built on...
  - nknight 15y ago
    The vulnerability is that Rails is insecure by default. That used to be the case for a lot of things, then finally people noticed how the real world works, and started fixing them. Apparently the Rails developers have actively resisted the lesson everyone else already learned.
    - boundlessdreamz 15y ago
      The vulnerability is still Github's. Rails provide the tools to do this right. Whether rails should provide stricter defaults is another question altogether. I was replying to the parent, who attributed this to the power of "open source & eyeballs looking at your code" but this is not such an instance.

klodolph 15y ago

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.

psadauskas 15y ago
One of the first comments is a link to where he tried, but the issue was repeatedly closed.
- phillmv 15y ago
  He submitted it to… the rails bug tracker. Three days ago. That's not disclosing the issue to GitHub at all.
  - BenjaminCoe 15y ago
    It's a pretty insane leap of logic to maliciously attack the Github website to prove your point regarding a framework hosted on that website. Politely contacting people about the vulnerability, rather than pulling that crap in an extremely public venue seems like the more mature, and less incredibly illegal way to go about things.
    - jsnell 15y ago
      The "attack" clearly wasn't malicious, though probably immature. A malicious attacker would have been doing things like gaining access to users' private repos and stealing the code, or trying to sneak in harmful commits to a repo under a false name. This was at most a prank or a demonstration. So why this instead of responsible disclosure? The problem is that it was not really a GitHub issue, it's Rails having a grossly insecure default setting. According to one of the comments in the bug tracker a lot of other high-profile sites also have the same problem. And presumably new ones would keep popping up for as long as Rails is the new hotness. So disclosing the problem to GitHub would not solve anything. They'd deploy the fix, but a lot of other sites remain vulnerable. That's probably the case even if GitHub were willing to take a PR hit and admit they'd been insecure for a long time in order to spur other Rails users to fix their code. After all, they hadn't fixed their code after the previous widely published security problems caused by the same underlying issue. Clearly the Rails core team were not willing to consider any kind of changes to improve the situation. As such, you can argue that making as big a scene as possible is the best way to improve security globally. It's of course unfortunate for GitHub that he chose to use that site as the example due to the obvious reasons. And certainly the timing is about as unfriendly as possible from the point of view of a west coast person.

apl 15y ago

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 http://homakov.blogspot.com/p/about-me.html for reference.]

5vforest 15y ago
On the other hand, this makes for a hilarious perusal of his blog.
rplnt 15y ago
Well... > Why I do this? Since guys in rails issues ingored me But I guess I agree.
deleted 15y ago
[deleted]

deleted 15y ago

[deleted]

holman 15y ago

We've patched and fixed this on GitHub.

arturadib 15y ago
Time from exploit to fix in production: < 1h. On a Sunday morning. I'd buy Github stock if I could.
- pyre 15y ago
  It doesn't hurt that the vulnerability was already discussed in a bug report by the 'attacker.' I'd imagine it would have taken a longer time had they needed to track down how exactly this happened, though I guess I'm making an assumption on what their logging/auditing/reporting is like.
  - jberryman 15y ago
    Right, I'm kind of baffled by the posters on the bug report calling this a "0-day" exploit.
- sausagefeet 15y ago
  At the very least you'd probably be getting it on the cheap at that point.
lucaspiller 15y ago
Good work. Please could you write a post explaining what you did, so others using Rails can do the same?
- momoro 15y ago
  They probably simply made user_id protected on whatever table controls commit permissions.
bgentry 15y ago
Do you plan on notifying customers whose repos he accessed? This looks like more than just a minor breach if he was able to give himself admin rights and had read/write access to any repo.

deleted 15y ago

[deleted]

wycats 15y ago

Here's my proposal for improving the situation: https://gist.github.com/1974187 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.

bradly 15y ago
> Merb's approach was to have mass assignment protection in the controller, and I personally think it's self-evident that it belongs there. But wasn't Merb merged into Rails? :) Sigh...
- wycats 15y ago
  Not every idea from Merb made it into Rails. Especially when an idea would cause significant backwards-compatibility breakage (return string from action vs. implicit rendering), we stuck with the Rails approach. I always felt that "it's up to the developer to do the right thing" violates the normal Rails convention over configuration principles, but I also weigh breaking a large % of existing Rails apps in a way that is not easy to quickly fix heavily. That said, this problem is almost identical to XSS protection. We were able to find a solution that mostly "just works" for new developers, with some caveats, but it broke nearly all existing apps in a way that required significant effort to fix. Like mass assignment, previous vulnerabilities were caused by Rails defaults that caused most users to make mistakes (nearly everyone had at least a few cases where `h` was required but wasn't done). Like XSS protection, we have a solution here that will mostly just work for the happy path. The end result is a Rails default that will be only marginal harder to use than what we have now, but secure by default.
  - bradly 15y ago

nate 15y ago

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.

nfm 15y ago
In my uses of rails, belongs_to associations get changed frequently. Any time anything gets 'assigned' to something, this occurs: assigning something to a user, to a milestone, to a plan, to an account, maybe even to a priority. YMMV but I'd say this isn't minute. That said, there are certainly places in my apps where I don't want this to occur. And whitelisting is much better than blacklisting!
- nate 15y ago
  And you're using mass assignment to do those assignments?
  - nfm 15y ago
    Nah, just changing the _id column's value so that the belongs_to association changes.
    - nate 15y ago
      "There is probably a minute number of cases where someone wants mass assignment changes to include the parent's id of that record" So it sounds like you aren't using mass assignment to change parent ids either. Most people don't. Most people use mass assignment to change the attributes of an object, not that objects association to a parent.

patrickaljord 15y ago

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.

lgeek 15y ago
In the case of Diaspora it was a less subtle and more obvious issue. They just weren't checking for authorization, as in: def destroy @album = Album.find_by_id params[:id] @album.destroy [...] end when the second line should have been something like: user.albums.find_by_id params[:id] But, well, they're both pretty bad mistakes.
- zdouglas 15y ago
  Palpability is in the eye of the beholder. There's a lot of talk in the GH issue comments dismissing GH's "obvious" failure of not using `attr_accessible`.
- patio11 15y ago
  Nope, Diaspora also had this exact issue, which would let you use anything where a params hash updated a user model to e.g. overwrite their credentials or encryption keys. The specific exploitable example I found would have let you do it even if they had been checking authorization to update objects properly, because the attacker could reassign his own objects as the victim's objects with arbitrary attack payloads, one of which being sufficient to compromise the victim's account. When I wrote a journal article about it my recommendation was that Rails ship with ActiveRecord::Base.attr_accessible(nil) by default, because otherwise vulnerabilities of that nature were virtually inevitable.
bascule 15y ago
Ensuring input is properly validated is always the developer's concern. A framework can make that easier for you, but especially in cases like this, which are basic user authentication concerns, it is ABSOLUTELY the developer's responsibility to be 100% sure how all of those abstractions work and ensure there are no leaks anywhere. You can't punt on security. Abstractions may make it easier, but you better be damn certain how they work.

bretthopper 15y ago

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.

zwily 15y ago
We included a spec in our rails app that inspects all AR models and errors if any don't specify any attr_accessible (with a whitelist of models to ignore). This catches anything included by plugins too, which can be helpful. https://github.com/instructure/canvas-lms/blob/9b52a51b6a37efa5164f7f636dd8e7478dd0eb77/spec/models/general_model_spec.rb#L46 https://github.com/instructure/canvas-lms/blob/9b52a51b6a37e...
eval 15y ago
This is definitely a first step. The thing is that with 'attr_accessible' a lot of interaction with your models gets harder (think testing or working from the console). I tried fixing this with introducing roles that have access to all attributes. Source at https://github.com/eval/sudo_attr_accessibility https://github.com/eval/sudo_attr_accessibility

shearn89 15y ago

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...

shearn89 15y ago

Also just noticed this at the bottom of his resume (http://homakov.blogspot.com/p/service.html http://homakov.blogspot.com/p/service.html): "<s>Discount for girls</s>"

comechao 15y ago

He did the same with Tower: https://github.com/rails/rails/commit/b83965785db1eec019edf1fc272b1aa393e6dc57 https://github.com/rails/rails/commit/b83965785db1eec019edf1...

teej 15y ago

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.

ceejayoz 15y ago
The point, though, is that this is more of a Rails issue than a Github issue. If Github gets it wrong, others are likely to.
- rsynnott 15y ago
  Well, it's a beginner-level Rails mistake; it is not precisely an obscure issue. Googling 'attr_accessible' will show you discussion going back years, and it has been actively exploited before. I'm shocked that the mistake was made in Github, though.
  - thibaut_barrere 15y ago
    It's a beginner-level Rails mistake but it's very widely seen in codebases: it's a real, common threat.
    - acqq 15y ago
      Obviously not beginner-level if more famous sites suffer from the same problem -- Egor mentions some in his posts and he is only one person -- imagine what all the black hats can do working in parallel.
      - Blaisorblade 15y ago
        By your reasoning, since tons of C programs can't use strcpy() correctly and thus contain buffer overflows, it takes a C master to use strcpy() correctly. I'd say instead that the kind of reasoning is wrong and that even experts can make beginner-level mistakes (not so often). But these are the kind of mistakes a good framework/language/etc. should try to prevent - especially if security-related. Tons of research tries to reduce error-prone programming activities.

tvon 15y ago

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

orblivion 15y ago

> "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.

psquid 15y ago
Yeah, it seems to me that committing a fix with a commit message along the lines of "Oops, you left this open. Don't worry, though, I fixed it." would've been the best option.
- muyuu 15y ago
  Technically it would be more like: "I just set it to safer defaults, since you guys won't. By the way, if your defaults are not strict enough for Github - where you trusted to host your code - gets them right, that pretty much makes them unsafe defaults."

javascriptlol 15y ago

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?

sad_panda 15y ago

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.

unseen 15y ago

This sounds like a "register_globals" feature. Are people still that obtuse to basic security?

mvasilkov 15y ago

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.

danmaz74 15y ago

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.

masklinn 15y ago
> I would say that homakov's angry and not very mature reaction to his warning being ignored Not just ignored but dismissed multiple times. It's not exactly surprising that he went and injected code into Rails's master.

Estragon 15y ago

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?

codesuela 15y ago

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.

deleted 15y ago

[deleted]

deanpcmad 15y ago

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

masklinn 15y ago
Being a contributor does not mean he has push access to the rails master, it just means one of his patches/pull requests got accepted into Rails in the past. In fact, it's even better: the commit he pushed into Rails's master through the exploit would have automatically added him as a contributor even if he had not already been one.
jroll 15y ago
The point of his hack was to _make_ him a contributor to rails.

hundredwatt 15y ago

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 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

deleted 15y ago

[deleted]

raggi 15y ago

http://speakerdeck.com/u/holman/p/ruby-patterns-from-githubs-codebase?slide=33 http://speakerdeck.com/u/holman/p/ruby-patterns-from-githubs... nuff said.

bbeausej 15y ago

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?

teyc 15y ago

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.

wandernotlost 15y ago

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 http://en.wikipedia.org/wiki/Fail-safe

tlogan 15y ago

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.

teyc 15y ago
Can somebody explain why this is a Rails bug? Insecure by default. Microsoft used to be the laughing stock because the default install was vulnerable to exploits. While one might argue that Github devs should have known better, the counterargument is that if Github devs couldn't get it right, think about the thousands of people trying out rails for the first time, building their little web app.