Responsible Disclosure Policy

Responsible Disclosure Policy(github.com)

317 points by mnilsson 14 years ago | 78 comments

georgemcbay 14 years ago |

Speaking as someone who isn't a Rails developer (but does use GitHub Enterprise for work projects), when this first broke I was on the side of github and thought homakov was acting irresponsibly.

Now that more background is coming out, I think he probably did the Rails community at large a huge favor here. Had this just been fixed quietly on GitHub, that would certainly be better for GitHub's PR but the wider community might never have realized the lurking horror that the Rails team appears to have been unlikely to do anything about other than point people to the existing docs.

This situation shows that pointing people to those docs was clearly an inadequate solution. If GitHub (arguably the poster child for Rails apps outside of 37signal's own apps) could fuck this up, anyone using Rails could. All of this exposure to the problem is net positive for everyone using Rails other than GitHub and the core Rails team, IMO.

prophetjohn 14 years ago | |

It's not like this is a little known pitfall with Rails. Anyone who has read Hartl's Rails tutorial knows about it[1]. It's very commonly mentioned in basics for Rails security.

And I say all this someone who has never professionally developed for Rails. My experience with Rails consists of a couple half-done toy projects. I find it pretty surprising that Github makes this mistake. But I don't think they should be burned at the stake for this. The bigger problem was how they were initially handling the issue, which they're trying to rectify now.

[1] However, Hartl recommends using attr_accessible at the model level and DHH says this preventative measure should be implemented in the controller, ie:

    class PostsController < ActionController::Base
        def create
            Post.create(post_params)
        end
  
        def update
            Post.find(params[:id]).update_attributes!(post_params)
        end

    private
        def post_params
          params[:post].slice(:title, :content)
        end
    end

nknight 14 years ago | | |

I'm frankly amazed at how optimistic HN seems to be about "professional" coding practices. To this day I find "professional" developers writing fresh SQL injection vulnerabilities with some frequency.

javascriptlol 14 years ago | | |

I don't find it in any way surprising that this has happened any more than I would find it surprising that if you put a big hole in a footpath that someone would fall into it.

antris 14 years ago | |

I see this whole story as "Irresponsible kid accidentally teaches the whole town a lesson"

acqq 14 years ago | | |

More "an innocent kid was the only one not afraid to say that emperor is actually naked."

Please read this post written by meric, then give the counterarguments if you have them:

http://news.ycombinator.com/item?id=3665083

"I don't believe it is the stunt that costed github five to six figures. The loss of wealth was already there from day 1 when Github developers did not read Rails documentation and/or when Rails decided to make attributes publicly accessible by default. Today it is merely a "correction" where instead of Github's customer losing confidential company information without knowing it is now Github bearing the costs upfront, as it should be. In the "emperor has no clothes" story would you say it was the kid who pointed out the emperor had no clothes caused the emperor's embarrassment?"

You can also compare the whole context with the misfeature of PHP:

http://www.php.net/manual/en/security.globals.php

dandelany 14 years ago | | |

Irresponsible, perhaps, but somehow I doubt it was accidental :)

reledi 14 years ago | |

I think everyone won in the end. Homakov got the credit he deserved, Rails' security flaw got a lot of attention, and GitHub had the chance to prove themselves, which in my opinion they did.

davesims 14 years ago | |

It's an easy mistake to make, but arguably no easier than, for instance, not escaping input strings to guard against SQL injection. IMO it falls to the developer to set protected on vulnerable attributes. This is pretty basic Rails security practice.

EDIT: not 'escaping', but using hashes or formatted strings, etc., you get the idea.

einhverfr 14 years ago | | |

I am glad you fixed this in your post btw, but I still think the initial thought of using escaping to solve issues like SQL injection shows the way we think about security is flawed.

In general we tend to see security flaws as programming flaws. In other words, the programmer makes a mistake and therefore there is a security hole. The problem with this approach is that programmers make mistakes all the time.

Certainly it is impossible to take all the weight off the programmer's shoulders. Mistakes will always allow a program to be misused, misdirected, and so forth. However, most security issues are best solved as architecture problems, not as developer problems.

For example input sanitation is generally a bad idea* beyond certain things we should never see in inputs. It's far better to find ways of making the input safe to the database and to the web interface that doesn't depend on it being sanitized on input.

* This is because you can only sanitize based on how you want things to go on output, whether you are outputting from your program to the db or to a user interface of some sort. If you sanitize for HTML, you can't use the same info reasonably well for LaTeX, etc.....

So I am of the considered opinion that data should be checked for basic sanity on input (no termination sequences in the middle of input strings, etc), and escaped on use or output. If you have a framework to do this, then you centralize that logic so you don't have to think about it every time. This drastically cuts down on things like XSS, SQL Injection, and the like.

This sort of thing again strikes me as something the framework should prevent. That's not necessarily a flaw of Rails if you use Rails as a toolkit for your own application frameworks. However, it is an architectural flaw, not a programming mistake.

zzzeek 14 years ago | | |

I'm not a Rails/Ruby user but any decent database abstraction layer or ORM should be using bound parameters for all literal values. "Escaping" of SQL strings is best left to the database driver.

jarin 14 years ago | | |

This kind of sums up the point of contention.

Rails, by default, does things like escaping input and output strings, CSRF protection, masking password fields in the logs, etc. So why doesn't it do the same with attribute assignment?

JumpCrisscross 14 years ago |

Given that:

(1) the nature of the suspension was not communicated to Egor at the onset of the situation, nor,

(2) noted in the blog post [1] describing how Github "detected the attack",

I am inclined to believe that this is a response to the furious reaction to their suspension decision and was not, as this post implies, the game plan from the beginning.

It's healthy that they've reversed their suspension but the lack of transparency (not to mention potential dissembling) on the decision process regarding the revocation is still troubling.

[1] https://github.com/blog/1068-public-key-security-vulnerabili...

kragen 14 years ago |

The problem I see with this blog post is something I haven't seen mentioned in the comments. It's not GitHub's place to set policy on what kind of disclosure is or isn't "responsible". Egor Homakov's responsibility is not to GitHub; his responsibility is to other users. His moral duty upon finding a security vulnerability is to act in such a way that other users will be minimally hurt. It appears that he has fulfilled that responsibility spectacularly in this case.

GitHub has no business demanding his, or your, agreement to a legal contract that prohibits you from exercising your best judgment in such a case.

Furthermore, "responsible disclosure" is a propaganda euphemism for "allowing irresponsible vendors to cover their asses, possibly at the expense of their users". Terms like "responsible disclosure" have no place in a serious discussion. Please see the blog post by the Google security team at http://googleonlinesecurity.blogspot.com/2010/07/rebooting-r... for further details.

chalst 14 years ago | |

This is beautifully put.

dthunt 14 years ago |

Honestly, I am less likely to want to use github in light of this announcement. You handled this incident badly, and then didn't acknowledge it, nor offer the much-needed props to Egor for exposing an issue you guys didn't think was serious.

If this is how you react to someone who WANTS to tell you about a serious problem, how what percentage of the people who don't love you enough to put a tattoo on themselves are likely to report an issue versus sell this to one of the many buyers of 'sploits who exist out there?

The reality is that these folks generally don't want to hurt you, they just want you to understand the thing you won't admit. When it happens, and you've got egg on your face, grow a pair and cop up to the fact that you/the system failed, and GIVE PROPS. Fix the issue, move on, and award the guy who did you a solid by finding an issue his 15 minutes of fame.

Volpe 14 years ago | |

He didn't WANT to tell github, he wanted Rails Core to pay attention to him, and github was the sacrificial lamb.

He doesn't deserve props from github, he just exploited their app to make a point (to rails core) he never disclosed anything to github, from what I can tell.

gbrindisi 14 years ago |

I hate to be the one pointing out this but it's a shame that a company like GitHub will reward responsible disclosures just with a thank you and the promise to not pursue a legal action.

http://help.github.com/responsible-disclosure/

"white hat researchers are always appreciated"

rflrob 14 years ago | |

If what you're implying here is that they should be offering a bounty for discovery of bugs, I'm not necessarily disagreeing with you, but to expect them to get a policy about that and to allocate funding for those bounties on a Sunday, within 24 hours of a major, public breach seems a little unreasonable.

gbrindisi 14 years ago | | |

I'm with you but still it's silly they didn't have a responsible disclosure program until today in the first place.

joedev 14 years ago |

Another in the long line of Silicon Valley companies screwing up and then getting kudos from the community for handling the screw up. It's getting tiresome to see people get accolades for basically doing their job, after failing to do their job.

sneak 14 years ago |

Their claim that his exploiting the vulnerability (in a completely benevolent fashion) was not "responsible disclosure" is bogus.

They need to stop trying to cover their ass and just apologize for suspending the guy.

skeletonjelly 14 years ago |

"and we worked with him to fix it in a timely fashion"

Did this take place in private? I can't see any evidence of this from his issue on the rails repo.

mojombo 14 years ago | |

Yes, this happened via our support system which all happens through private emails.

technoweenie 14 years ago | |

Yup.

dhconnelly 14 years ago |

This is a great response to this clusterfuck. These guys have a responsibility to react slowly and deliberately. I'm impressed they got the problem solved and two public statements out on a Sunday. This is why I'm happy to be a paying customer.

ashamedlion 14 years ago |

Impressed by how Github handled this. It's easy to get red-faced and smack down with a hammer, and it's hard to remain level-headed and come to the best decision. This seems like it was, in fact, the best decision.

For those of you who didn't see, they were quick to update people and be on the ball: https://github.com/blog/1068-public-key-security-vulnerabili...

spullara 14 years ago |

The thing I haven't heard a straight answer on is 1) how long has the bug existed, 2) have they proved that it wasn't previously exploited.

luriel 14 years ago | |

1) Given the nature of the bug "for ever" seems like a good guess 2) I doubt they can prove any such thing.

gnu8 14 years ago |

(Ir)responsible disclosure always does more harm than good. Making these issues public is the only reasonable thing to do.

mmj48 14 years ago | |

Disclosure does mean making things public...

ktizo 14 years ago |

At least, after this, there should be a few people picking through github with a pin to look for other vulnerabilities.

ig1 14 years ago |

What the guy did was not only morally irresponsible but also criminal.

The security community has long has an accepted standard of responsible disclosure, which involves informing the vulnerable party beforehand and allowing them time to fix the problem before publicly disclosing it.

Publishing a vulnerability before giving those vulnerable a chance to fix it is irresponsible, using it to compromise a system is criminal. He was getting off light from getting his account suspended, GitHub could push for a criminal prosecution resulting in deportation and serious jail-time for his actions.

It doesn't matter what he did after the compromise (whether it was benevolent or not), the compromise of an account not held by him puts him clearly into the "black-hat" category.

obtu 14 years ago | |

The vulnerability was public, known for years, and no doubt already exploited. Making a splash about it on GitHub, popular as it is with rails hackers, is the best thing that could happen to the security of the rails ecosystem.

ig1 14 years ago | | |

The class of vulnerabilities was known but not this particular case. That's like saying people should be publish 0-day buffer overflows because it's a known vulnerability class.

Karunamon 14 years ago | |

>the compromise of an account not held by him puts him clearly into the "black-hat" category.

That's not what "black-hat" means.

ig1 14 years ago | | |

Yes it does.

Gain unauthorized access to an account and using it falls under pretty much any standard definition of "black-hat" and in practical terms breaks computer security laws in pretty much all legal jurisdictions which have them.

JulianMorrison 14 years ago |

Honestly I have no idea why they didn't just ban him and say "don't hack us". It really should be that simple.

gavingmiller 14 years ago | |

The straight up ban is a great short term solution, but in the long run being easy to work with on security issues and not alienating your users is a better road. As they stated, this user wasn't malicious so banning him only causes grief and could turn him from an ally into an enemy.

There was a great post a couple of days back that in effect said: It's not a matter of _if_ your security will be compromised but _when_. By being open to your users disclosing this information you're helping to keep your product secure. IMHO 37signals does a good job of this by linking and giving credit to those that have discovered security flaws in their apps (http://37signals.com/security-response).

tommorris 14 years ago | |

There's a very good reason not to do that. I'm a Wikipedia admin, and we have a policy on blocking and banning that sort of makes sense. A block is done to "prevent damage or disruption". That might be short term or indefinitely, but not permanently. You put in a block when there's an issue. But a ban is a formal statement that you are no longer allowed in. You only do that after some thought and consultation.

Github is actually pretty similar given that the commercial side of Github is fuelled by the free, open-source side. There is a feeling of community ownership. Going straight for the ban without some thought and soul-searching is confusing fixing an issue with making a more detailed judgment about whether the user ought to be on the site at all.

IMHO, Github played it about right.