Public Key Security Vulnerability and Mitigation(github.com)
github.com
Public Key Security Vulnerability and Mitigation
https://github.com/blog/1068-public-key-security-vulnerability-and-mitigation
34 comments
Saw your full comment on github: https://github.com/blog/1068-public-key-security-vulnerabili...
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).
I just wrote a longer than expected summary.
http://news.ycombinator.com/item?id=3664400
http://news.ycombinator.com/item?id=3664400
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.
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.
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.
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.
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.
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.
> 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.
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.
I agree with your sentiments. What Homokov did was childish, but he made his point in a non-malicious way that got the message out better than silently writing a letter to the admins. It is evident from the events that occurred that this is an issue in Rails that needs to be fixed. What does it say about a feature of your framework when, by default, one of the largest code repository hosting sites in the world is vulnerable? The phrase "meaningful defaults" has never been more relevant.
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...
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...
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.
The vulnerability was introduced to github by misusing a rails feature.
From the linked blog post by homakov:
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...
(Edited for formatting.)
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.
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...
(Edited for formatting.)
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.
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.
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.
Here's another shocker, cars can be driven off cliffs!! Sue Ford!
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.
Indeed , seems more akin to buying a second hand car and forgetting to ask if the brakes are actually connected before driving off.
Not a rails developer here, but shouldn't basic validation inside the model class on persist be able to prevent this from happening?
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?)
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...
A global source code repository is one of the more terrifying targets on the Internet; imagine subtle compromise of various libraries...
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 FCKNG 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 =(
Imagine things like instagram, path, evernote and so being seeded in thepiratebay, this is a FCKNG 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 =(
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? ...
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? ...
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.
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.
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!)
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!)
Github was right to ban the guy. He did hack them roundly after all.
On the other hand, he did it in a largely non-malicious way and even managed to expose those Rails knuckleheads for the greater good.
Github should find a way to let him back in, intentionally. Perhaps they could offer him to create a new user and they could give him some of his repositories back (if any were important).
Edit: It appears Github has in fact reinstated him.
On the other hand, he did it in a largely non-malicious way and even managed to expose those Rails knuckleheads for the greater good.
Github should find a way to let him back in, intentionally. Perhaps they could offer him to create a new user and they could give him some of his repositories back (if any were important).
Edit: It appears Github has in fact reinstated him.
GH is playing to two audiences here. The hackers agree with you, but the corporate types (those who play by the rules) don't. GH stands to lose more by alienating the naive mob who think banning the account makes an iota of difference than it has to gain by trying to please the rest of us. GH is acting risk-averse with its PR strategy.
My prediction: Egor gets a job offer from GH within 2 months.
My prediction: Egor gets a job offer from GH within 2 months.
They should hire the kid, not bitch about it.
He wasn't upfront about everything what he was doing, so we had to investigate.
They need to be seen to do something. Imagine if something had been stolen and this became high profile, the press would be asking why this kid was able to carry on using the site after he was known to have exploited this.
Do you really believe that suspending his account prevents him from creating another one for further exploits? (should he actually want to, which I doubt)
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'.
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'.
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.
Of course, that GitHub should get caught by it is alarming.
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.
I haven't heard a single kind thing about GitHub's code from people who've deployed their FI, or more recently, enterprise product.
They've got some talented people, but I'm guessing from their posts speaking of how they work that they don't have much in the way of code review.
They've got some talented people, but I'm guessing from their posts speaking of how they work that they don't have much in the way of code review.
People do not learn from the past. People will keep reinventing these kinds of bad designs, and we will have security holes in web apps until it stops.
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.