

Public Key Security Vulnerability and Mitigation - yuvadam
https://github.com/blog/1068-public-key-security-vulnerability-and-mitigation

======
chrisacky
This is a shocking portrayal of the actual occurrences.

They/He make it sound like they were proactive about all of this, always one
step ahead of the attacker/@homokov

> As soon as we detected the attack we expunged the unauthorized key and
> suspended the user.

They didn't detect anything. They were informed. It also wasn't an attack. If
it was an attack, you wouldn't have seen it coming, or in the very least more
damage would have been done.

As far as attacks go, this was probably the "whitest" of white that has ever
gone down. Not only did the "attacker" not do any actual damage, but he was
continually ignored.

This is a shameful handling of the actual events.

~~~
mofey
Saw your full comment on github: [https://github.com/blog/1068-public-key-
security-vulnerabili...](https://github.com/blog/1068-public-key-security-
vulnerability-and-mitigation#comment-17266) Well put. I guess startups can act
enterprisy too when money is involved. The irony is that _had_ they been
actually proactive, they would have avoided this kind of publicity and money
(for 3rd party audits).

~~~
chrisacky
I just wrote a longer than expected summary.

<http://news.ycombinator.com/item?id=3664400>

~~~
marshray
I, too, agree with your sentiments. But I wish people would lay off of GitHub.
The were mostly just a bystander here.

The real troublemakers are the Rails developers who seem to seriously believe
that leaving such subtle security traps in their framework (and then blaming
the developers who follow the example code) is a defensible position.

~~~
newman314
I disagree. Github is very much not the bystander here. They chose to use
Rails (which is fine). But GH then has the onus to properly deploy their app.

An analogy would be a door that only locks with a special key in a certain
sequence. IF you choose not to do so, it's merely a door. Obviously, you could
argue that that's a bad default but I think that goes to the crux of the
problem.

~~~
marshray
I'm an experienced developer, but not terribly familiar with Rails.

What do you mean "properly deploy their app"?

The sample code at rubyonrails.org looks like it has the same problem to me,
i.e., it would be vulnerable if it were put into production in the right
(entirely reasonable) circumstances.

~~~
maratd
> What do you mean "properly deploy their app"?

No matter how bad the tool actually is, only a terrible craftsman blames his
tools. That's what he meant.

Both Rails and GH are at fault. Rails for not discouraging poor practices and
GH for not being more familiar with their own stack.

------
woodrow
The blog title is a significant misnomer, IMO. The vulnerability has nothing
to do with public keys, but with the incorrect configuration/use of a Rails
feature. This vulnerability was then _exploited_ to add a public key to the
rails/rails repo, but apparently could have been used for other purposes [1]
that are unrelated to pushing to a repo.

This seems like an omission that may have people worrying about the wrong
things or drawing the wrong conclusions if they don't take the time to
understand what actually happened.

[1] [http://homakov.blogspot.com/2012/03/egor-stop-hacking-
gh.htm...](http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.html)

~~~
cdcarter
The vulnerability was in github, allowing a public key to be added to any
project.

The vulnerability was introduced to github by misusing a rails feature.

~~~
woodrow
From the linked blog post by homakov:

    
    
      Since guys in rails issues ingored me and my issue I got spare time to test
      it on the first website i had in mind. github.
      That was pretty funny. Firstly, I could write post from 1234 year or 4321.
      Then, I could make a post pretending i am DHH. That was funny too.
     
      Then I could wipe any post in any project. That wasn't that funny but pretty
      dangereous. It got more curious.
      Today I can pull/commit/push in any repository on github. Jack pot.
    

Hence my implicit assertion that this is broader than commit access (via
public keys) and has nothing to do with cryptography/public keys itself.

To be fair, in the first sentence of the github blog post, mojombo goes on to
more precisely state "a security vulnerability in the public key update form",
but it appears that this may not be the full extent of things. For an example
of the other vulnerabilities homakov mentioned in his blog post, see [2] where
the contents of an issue was changed.

[2] [https://github.com/blog/1068-public-key-security-
vulnerabili...](https://github.com/blog/1068-public-key-security-
vulnerability-and-mitigation#comment-17306)

(Edited for formatting.)

------
nestlequ1k
Mass assignment is one of the worst practices in typical rails apps. And their
fix is to move everything to the models. All so they can have "skinny
controllers" with one liners like @user.update_attributes(params[:user]).

Seems crazy to me. I've never used attr_protected, or attr_accessible because
controllers should actually parse out the params and then explicitly define
what's needed to update the models.

It's more glue code, but you're making rails controllers do what they're
supposed to do... parse http form input and return results.

~~~
fryguy
As someone that's never worked in rails before, the fact that rails allows you
to take parameters from the url, and directly update a database object with
them is shocking to me. That's like the first thing you learn about securing
websites.

~~~
eagsalazar
Here's another shocker, cars can be driven off cliffs!! Sue Ford!

~~~
hythloday
As someone who's never driven a Ford (or, indeed, a car), I would not dream of
driving one off a cliff, nor would the Github team. It's entirely possible
that I would forget to use attr_protected. Github certainly did.

~~~
jiggy2011
Indeed , seems more akin to buying a second hand car and forgetting to ask if
the brakes are actually connected before driving off.

------
Estragon
It says they've cleared out homakov's modification, but I gather this
vulnerability has been around for a long time. Is it paranoid to worry about
malicious commits to other github repositories? (By other more surreptitious
parties?)

------
rdl
I'm curious if he reported it in any other way (www.github.org/security/).
IMO, if you report something, and no action is taken, it's understandable if
someone does an exploit like this, although not strictly ethical or right.

A global source code repository is one of the more terrifying targets on the
Internet; imagine subtle compromise of various libraries...

------
postit
This guy could have stolen several codebases, but he decided to prank rails
team.

Imagine things like instagram, path, evernote and so being seeded in
thepiratebay, this is a F _CK_ NG serious and insane issue. I have my entire
copyrighted research and codebase into github.

Im really pissed about this issue and considering to move away from github =(

------
drucken
Not sure why so many are getting worked up about the suspension.

Would _you_ not at least temporarily suspend a user account that had
compromised your stack for audit and temporary prevention of further new
developments?

If they meant to remove the account or target that specific human user, would
they not have made a statement to the fact and done something a lot more
irrevocable and unambiguous? ...

~~~
erichocean
Here's what bothers me about GitHub's response.

This 18 year old kid's actions made an enormous difference for good in the
security of _every single user of GitHub_.

And for that, he gets banned. WTF.

GitHub needs to man up, say thank you, publicly apologize for banning the kid,
and politely ask people in the future to send Rails exploits on GitHub to
some.email@account they've set up to ensure they're dealt with promptly.

Claiming the "letter of the law" here is making GitHub look seriously petty
and lame.

~~~
FuzzyDunlop
I would go as far as to say _every single [beginner] user of Rails_ too. If it
wasn't for all this, I'd never have known there was a gaping exploit I'd need
to patch up in my own app in development.

This, in and of itself, is a major flaw in their documentation. It's like
PHP's mysql functions not mentioning escaping strings on their manual pages,
but instead burying it all entirely elsewhere, where you'd never think of
looking.

"You should have read every single page of the documentation, then you'd know
we briefly mentioned mysql_real_escape_string() and prepared statements that
prevents this issue."

I mean, the least you can do if you won't provide a fix, or sensible defaults,
is to make it abundantly clear you have to correct it yourself. (Bit of a
broken record with this schtick, sorry!)

------
Jebus
I have been using PHP for 4 years, and Ruby and RoR for a month or so.

And I have noticed this pattern:

In PHP frameworks, creating an object with raw $_POST is as common as in Rails
(params), because it's faster for the developer. Of course, it's a known bad
practice, and is discouraged.

The difference is that when a php developer makes this mistake, people say
it's because php developers are all noobs and php is crap.

Now, when a Rails developer makes the very same mistake, it gets fancy names
like 'mass assignment vulnerability', and by what I read in the comments on
github, they are fully convinced that no other framework has this
'vulnerability', because no other framework has this feature of 'mass
asignment'.

~~~
polemic
Yep, this bothers me too. attr_accesible is to Rail's what magic_quotes_gpc
was to PHP: a terribly misconceived 'helper' feature that is almost designed
to trip up beginners in the worst way.

Of course, that _GitHub_ should get caught by it is alarming.

~~~
javascriptlol
Even with good knowledge of a system people forget things. The blame for this
incident falls 100% on rails and its awful design. It's like blaming the
driver for crashing your car whose brakes can silently and unpredictably fail
unless you press some button when you first get into the car. Critical failure
points should be isolated aggressively. Many software "engineers" have not yet
grasped this.

