
Hacked: commit to rails master on GitHub - stwe
https://github.com/rails/rails/commit/b83965785db1eec019edf1fc272b1aa393e6dc57
======
wycats
Here's my proposal for improving the situation:
<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.

~~~
moe
_self-evident_

It is. The more interesting question is why it takes rails 7 years (and
counting) to come to this conclusion.

<rant>

I'll take it one further and sing my song about the Rails ActiveRecord
implementation here, which is tangentially related.

The promise of AR is to reflect on the database at startup and then "magically
work".

The problem in rails is that nothing magically works. What you get out of the
box is an insecure world-writable model, as illustrated by this bug. Then you
begin scattering your truth over incremental (hand-crafted!) migrations, the
model, and the controller, until nobody other than a cascade of unit-tests can
even make sense of the interdependencies anymore.

In a world where there is not even a central place to look up which fields
_exist_ on a model and what their constraints are - short of runtime
introspection, where database constraints live happily alongside and
independently of model constraints, where opaque bits of ruby-logic buried in
various gems add their own magic to the mix, in such a world it's really no
surprise they chose to default to "all fields writable".

Because if they forced the user to do the sensible thing and explicitly list
the allowable fields then where's the advantage over a declarative approach
(like e.g. Django) anymore?

Imho it's long overdue to take a step back and revisit whether AR is still
worth having, or ever was. Imho it causes _way_ more problems than it solves,
in contrast to the declarative approach.

</rant>

~~~
xnxn
> cue the downvotes

Why end your rant with an insult to its readers? Please leave the martyrdom
out next time.

~~~
moe
You are right, sorry, I've removed that now. It was not meant as an insult,
but didn't add anything meaningful either.

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

~~~
GrumpySimon
Allow me to rephrase that:

"Well, it's a beginner-level _PHP_ mistake; it is not precisely an obscure
issue. Googling 'SQL Injection/register_globals/get_magic_quotes/etc' will
show you discussion going back years, and it has been actively exploited
before. I'm shocked that the mistake was made in ..., though."

Ten+ years of register_globals, get_magic_quotes and SQL injection attacks
(etc etc etc) in PHP show that even well-known issues still bite people
everyday. IMO, it's up to the framework developers to make it easy to do the
right thing and damn hard to do the wrong thing.

~~~
rsynnott
I don't think it's on the same scale as register_globals or magic_quotes; both
of those were much stupider. It's still very stupid, granted, but not on the
same scale as register_globals.

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

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

------
holman
We've patched and fixed this on GitHub.

~~~
maratd
As a paying customer, whose sole reason in paying you is to keep his data
secure, I expect a bit more than "we fixed it" ... details please.

~~~
grabastic
I am also a paying customer, but I don't feel entitled to a thorough
explanation anywhere but through their own official channels... even if one of
their employees has chosen to comment here.

~~~
maratd
> I don't feel entitled to a thorough explanation anywhere but through their
> own official channels

Nor do I. However, there was no indication given that an explanation would be
given through official channels. If that indication was made with the "fixed
it" comment, trust me, I would have kept my mouth shut and waited.

~~~
grabastic
I read more negativity into your comment than I should have. Apologies.

------
bretthopper
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
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/9b52a51b6a37e...](https://github.com/instructure/canvas-
lms/blob/9b52a51b6a37efa5164f7f636dd8e7478dd0eb77/spec/models/general_model_spec.rb#L46)

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

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

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

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

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

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

------
hundredwatt
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>, just add to lib/generators
in your Rails 3 app, then do rails g mass_assignment_security -h

Hopefully others find this helpful

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

------
arturadib
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
I think it's a mass assignment vulnerability:

<https://github.com/rails/rails/issues/5228>

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

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

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

------
nate
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
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
And you're using mass assignment to do those assignments?

~~~
nfm
Nah, just changing the _id column's value so that the belongs_to association
changes.

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

------
raggi
[http://speakerdeck.com/u/holman/p/ruby-patterns-from-
githubs...](http://speakerdeck.com/u/holman/p/ruby-patterns-from-githubs-
codebase?slide=33)

nuff said.

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

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

        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.

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

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

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

------
comechao
He did the same with Tower:
[https://github.com/rails/rails/commit/b83965785db1eec019edf1...](https://github.com/rails/rails/commit/b83965785db1eec019edf1fc272b1aa393e6dc57)

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

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

------
zyfo
Here's the guy's blog post about the hack:
[http://homakov.blogspot.com/2012/03/egor-stop-hacking-
gh.htm...](http://homakov.blogspot.com/2012/03/egor-stop-hacking-gh.html)

 _"Today I can pull/commit/push in any repository on github. Jack pot."_

~~~
arturadib
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
Hmm? The vulnerability is in github's code (not open source) and not in rails.

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

~~~
nknight
Lots of projects provided the tools to do lots of things right in years past,
but they eventually came to recognize that if they didn't provide secure
defaults, they were ultimately harming everyone out of some twisted sense of
principle. Insecure defaults are thus now considered a vulnerability in the
original project.

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

------
shearn89
Also just noticed this at the bottom of his resume
(<http://homakov.blogspot.com/p/service.html>):

    
    
        "<s>Discount for girls</s>"

------
apl
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> for reference.]

~~~
rplnt
Well...

> Why I do this? Since guys in rails issues ingored me

But I guess I agree.

------
klodolph
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
One of the first comments is a link to where he tried, but the issue was
repeatedly closed.

~~~
phillmv
He submitted it to… the rails bug tracker. Three days ago. That's not
disclosing the issue to GitHub _at all_.

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

~~~
BenjaminCoe
This clearly was malicious:

* it provides a how to for other individuals to repeat the attack, in a public forum.

* it was made against an innocent third-party.

* I doubt steps were taken to contact the third-party.

* it was made on a Sunday morning. making it difficult to scramble and get a fix out the door.

"Clearly the Rails core team were not willing to consider any kind of changes
to improve the situation"

The ticket was opened three days ago. Are you advocating that if an issue
isn't resolved in an open-source project, in under a week, the individual
raising the issue should be able to publicly attack anyone using the project?

Also, I'll put this gem of a quote out there:

"not only github is vulnerable this way - I found a lots of rails apps that
are waiting for my hack! Yeah, it is only start" (mwahahahaha).

~~~
jsnell
Whether it was malicious depends on the motives, and it's very hard for me to
see where the malice is. To me it looks more like he just wanted to bring
attention to the issue in order to get it fixed.

I already agreed that GitHub were innocent bystanders and that the timing was
unfortunate. But if getting publicity to the issue was the main point, it's
also easy to see why GitHub was the perfect target. I also already explained
why it could make perfect sense to demonstrate the vulnerability in a public
manner rather than just disclose it to one of the many sites suffering from
the problem. None of that is a sign of malice, it's at most bad judgement.

The ticket having been opened only three days ago would be a good point if it
hadn't also been closed and declared to be working as intended with a pointer
to a previously closed bug about the same issue.

~~~
BenjaminCoe
I think you're winning me over to Egor's actions being more irresponsible and
misguided than malicious -- chalk it up to him being 18.

I'm finding it pretty hard to stay mad at someone with this tattoo:

<http://homakov.blogspot.com/2011/07/octocat-tattoo.html>

~~~
alexbell
Agreed. Immature - certainly. I don't think his actions were done with
malicious intent. Maybe insolent, but that only counts in the military. If he
wanted to he could have made his point in a far more malicious manner.

And the octocat tattoo... How can you not like a guy with an octocat tattoo!

