Thoughts on the Github Hack

Over the weekend, a young coder demonstrated a security vulnerability in github.com - one with wide-reaching implications. An early demonstration is at: https://github.com/rails/rails/issues/5239.

Our friend went on to make several updates to github as he experimented/demonstrated the vulnerabilities, got his account suspended, reinstated and set off a firestorm of criticism every which way.

I was ready to put it all into the "someone is wrong" pile, until I ran across this pull request on the Rails core: https://github.com/rails/rails/pull/4062.  That's some mid-90's Microsoft style arrogance right there, and on the off chance that anyone is having trouble seeing it, I figured I might add my breath to the maelstrom.

First of all: it was not right to hack github.  It's not okay to ignore the intent of security, no matter how weak the enforcement.  Much better to have pointed out the vulnerability to github, although for sure the fame wouldn't have been as bright (which is why I'm pointedly not referring to him by name in this post.)  Given github's track record, I think there's a pretty good chance they would have come clean, admitted the fault, as well as crediting its reporter.  But that's complete supposition.

That said, I don't think it's legitimate to consider Github a blameless victim.  The flaw in Rails that was exploited is well known, and well reported, and easy, if irritating, to fix.

The technical aside here is pretty simple.  In a file in config/initializers add:

ActiveRecord::Base.__send__(:attr_accessible, nil)

Then you need to white-list mass-assignable attributes in your models:

attr_accessible :name, :body, :whatever

And keep an eye on your logs for

WARNING: Can't mass-assign protected attributes: :blah

Which is a sign that you might need to add an entry for :blah into the respective model.

All pretty simple.  There are a couple of other notes, like "don't allow reference fields (e.g. :person_id) to be mass assigned" but that's the meat of it.  Put the initializer in your generator (that's much harder) and you never have to think about it again.

So, Github didn't put a simple, well-reported fix into their code.  Is that so bad?  I think so.  Github not only invited the developer community to trust them with the products of their labor, pretty much ousting SourceForge from that position in the process and firming up a development environment choice for open source work (i.e. "use git for version control"), it also invites developers to trust them with secrets.  Specifically, the secret contents of client repositories.  Heck, they get you to pay for the privilege.   So, in short, Github is taking money to keep secrets.  And by not covering a known security hole in a default Rails deploy, they were failing to uphold the trust of their paying customers.

I think Github was letting us down pretty badly.  I think an overzealous coder did a bad thing to bring that to light, but you can't argue that Github should be surprised or is blameless.  Two bad things, no one is blameless.

But the last straw for me was reading the Rails core teams' replies to a pull request to set the default for whitelisting attributes in Rails 4.0 to 'true.'  (After previous discussion concluded that making the change for 3.2 would be "too disruptive.")  That having to do attr_accessible for every model was "a lot of paperwork" - the final commend is @dhh's "I don't like this. -1"  Which is to say: we would rather put unsanitized data into the database than do the bare minimum of manual review.  And that's pretty lame.