

How github was hacked - bluemoon
http://homakov.blogspot.com/2012/03/how-to.html#

======
jtchang
Is anyone else laughing at how ridiculous this vulnerability is?

I just spent a few hours last week hacking through the Stripe CTF game.
Environment variables, string formatting injections, and a timing/side channel
attack to top it off.

This is just POSTing a value to an endpoint. And it gets written?! To the
database?! That's awesome and scary at the same time.

~~~
Jach
Seriously. This is easier than SQL Injection! Props to Egor for finding it and
showing how stupid it is. I cracked up at his "is it really interesting?"
line. It makes me wonder if this was a well-known vuln to less-than-classy
folk who have already done some damage elsewhere on GitHub.

~~~
einhverfr
A similar problem (in Perl) lead us to fork LedgerSMB from SQL-Ledger in part
because the author of SQL-Ledger had trouble fixing it....

The thing is that this really belongs to a class of vulnerabilities where
authentication information is inadequately tied together on the server. This
allows any user with valid credentials to fabricate credentials for any other
user. In SL it was worse because all you needed was the timestamp and not,
say, a valid password, but the same applies.

One thing I will say is that this sort of vulnerability IME suggests
inadequate thinking relative to security (and probably other things) on the
part of the application designer and therefore raises questions in my mind as
to what else may be lurking there.

------
rdl
I wonder if they pay their security firms for code audits, vs. just system
configuration level stuff. It's reasonable to think they're about as expert on
their codebase and its security as anyone else would be, so just having
different people within the same company look at it would probably be
effective, and could be done more frequently than an external audit. (I
generally advise AGAINST external code audit for a lot of early stage
companies, since it can be a lot of wasted money when the codebase is
changing; it's better to have some guiding principles internally and then try
to reduce the scope of security critical code as much as possible, and
eventually audit that. Stick to securing the infrastructure, which is pretty
standard across companies and thus cheap, and can be handled through
outsourcing to gmail/heroku/etc., at least in the beginning.)

Looks like they have two companies for audit/pentest (nGenuity and Matasano
Security), plus a consultant. Somehow I doubt tptacek is going to comment on
any of this.

~~~
tptacek
Comment about what? (I'm reading this in a vanity search, 12 days after you
posted it; what can I say, boring Saturday).

I'm not sure I even realized we were on Github's site. If you're asking why
we're there, you're right: I obviously can't discuss it here.

------
ortatherox
I'm not a fan of some of the comments about the man posting, you'd think the
developer community would be above "you look like frankenstein" and "your
english sucks."

~~~
danso
I wonder if his poor English (though not as poor as my Russian) contributed to
him not being taken seriously? Glad to see he was reinstated...the guy seems
almost fanatical in his devotion to GH.

It is almost reassuring that the. If was so easy to exploit, because it means
that a nonchalant hacker could bring it to mass attention...I'd hate to think
of what havoc a dedicated miscreant could've sown in a week. I recently taught
a group of neophytes how to use the web inspector, and used the example of
altering a form merely to demonstrate the malleability of a webpage. I told
them "this is just a parlor trick, don't think that it'll actually work..."

~~~
chrisguitarguy
> I wonder if his poor English [...] contributed to him not being taken
> seriously?

I'd say that's very likely the case. Unfortunate, but, in some ways,
understandable. If a person is not able to articulate an issue effectively
dismissing them becomes too easy. We all do it.

~~~
etcet
"In class - in my English class - you will have to master and write in
Standard Written English, which we might just as well call "Standard White
English" because it was developed by white people and is used by white people,
especially educated, powerful white people. [RESPONSES at this point vary too
widely to standardize.] I'm respecting you enough here to give you what I
believe is the straight truth. In this country, SWE is perceived as the
dialect of education and intelligence and power and prestige, and anybody of
any race, ethnicity, religion, or gender who wants to succeed in American
culture has got to be able to use SWE. This is just How It Is. You can be glad
about it or sad about it or deeply pissed off. You can believe it's racist and
unfair and decide right here and now to spend every waking minute of your
adult life arguing against it, and maybe you should, but I'll tell you
something - if you ever want those arguments to get listened to and taken
seriously, you're going to have to communicate them in SWE, because SWE is the
dialect our nation uses to talk to itself... And [STUDENT'S NAME], you're
going to learn to use it... because I am going to make you."

\- David Foster Wallace, Authority and American Usage

~~~
gaius
I think he is trying to say
<http://en.wikipedia.org/wiki/Received_Pronunciation>

~~~
rdl
Aside from being about the US, he's talking about written English. Standard
Written English is fairly similar even across regional accents in the US, and
I'd say written English is more similar across the majority of the English
speaking world than spoken English. (there are the gratuitous spelling changes
introduced in the UK in the 1800s/early 1900s to differentiate from US
convention, and some different words and conventions for group noun plurality,
but aside from that very similar, whereas I have a harder time understanding
some Scottish or East London speech than I do French or German.)

(there's also the accent/dialect/creyole/pidgin distinction, which often
revolves around who has an army)

------
kalleboo
To be honest, mass assignment sounds like Rails' own "register_globals". The
default should be conservative and disallow setting any fields, instead of
allowing anything to be changed.

~~~
bradleyjg
Early PHP is exactly what came to mind when I read the description.

Nothing new under the sun.

------
sjtgraham
I just threw together a quick gem that will ensure active_record model
attributes are protected from mass-assignment unless explicitly declared as
mass-assignable.

Granted, this as default will break an app that does not have the correct
attributes declared as mass-assignable, but the alternative is a vulnerable
app.

<https://github.com/stevegraham/default_whitelist>

~~~
wtn
This feature already exists in Rails.

[https://github.com/lest/rails/commit/f2fa4837a8a888ee86997be...](https://github.com/lest/rails/commit/f2fa4837a8a888ee86997be892d0aa5bbd2b5fd0)

~~~
sjtgraham
Only for relatively new versions of Rails. The oldest version of Rails
including this patch was released January 20, 2012.

~~~
spicyj
Note that that commit is only a comment -- the addition of
"whitelist_attributes" presumably is older.

~~~
sirn
It was added in Rails 3.1.

------
mattlong
I'm not too familiar with RoR, but does the mass assignment vulnerability
basically boil down to not doing any input validation for the convenience of
updating several model fields in one line of code?

~~~
cubicle67
Here's what happens, loosely

We have a form with the fields firstname, surname and date_of_birth. cool.
user submits the form. Rails takes the post data and puts in in a data
structure called _params_

Now, at the backend we need to update our database with this new info. Rails
allows us to write

    
    
        user.update(params)
        user.save
    

and the database will then be updated with all three fields from the form.
nice. except... (you can see where this is going)

I alter the form and add another field, say _is_admin_ and set the value to
true. Now, if the database has a corresponding field, that also gets updated
with the value I've posted. uh oh.

Rails does have the ability to say

    
    
        attr_protected :is_admin
    

which will stop this. It also has a config option to effectively disable mass
assignment. Unfortunately it's one of those things that everyone knows about
but often seems to be overlooked/forgotten

------
homakov
just wonder why you all discuss that kind of shit: 'who's in charge', 'who
should be punished', 'funny bug' etc in other topics. This topic is better
because it is about reality. protect your attrs blah blah

~~~
muyuu
хех :) молодец

------
gphil
Here's the relevant section of the Rails manual describing this exploit, along
with some ways to prevent it.

<http://guides.rubyonrails.org/security.html#mass-assignment>

------
coreyspitzer
Regarding the argument between whether this is a Rails framework concern or a
Rails developer's concern, the fact that many tutorials and screencasts don't
bring this vulnerability up has left the impression to a whole slew of
developers that scaffolding and other step-by-step out-of-the-box ways to
build things that Rails affords you are complete solutions that don't require
any other modifications besides what's needed for your own business logic,
etc. I think this is how we all missed something so simple. I think it's
partly because of this convention/idiom groupthink.

------
kellenfujimoto
The comments are rather classy too. Glad to see xenophobia is alive and well.

------
peregrine
Seems strange that Active Record doesn't have an "update_fields" method with
sig array of updateable fields, hash of field=>val. Similar to Sequel.

~~~
callumjones
Because it's really the Model's job to determine what is safe and unsafe for
mass assignment, not the controller's.

~~~
skimbrel
But when the model is accessed from four different controllers representing
different privilege levels (e.g. public profile page, user settings page,
internal admin page) and user experiences, is it really still the model's job
to figure out which incoming updates are allowed to update which fields?

This is "thin controllers" gone too far -- the model shouldn't have to figure
out where it's being updated from and what to allow.

~~~
callumjones
I'm somewhat of two minds about this one.

1) The model could be used everywhere so it may be best to negate the issue by
locking down the attributes at one source.

2) As you pointed out, the model is in different contexts depending on the
privilege of the current user session. It's almost as if I want a thin layer
between my model and controller that takes into account session info and
informs the model about what can and can't be done.

~~~
virmundi
Another option is to have the model take more a DDD approach. Part of the
input params could be a user object. That same user object would know its
current privileges and that could be done within the large model that is
actually trying to do something.

What I don't know is if RoR allows for this sort of modeling. I have no
experience with the framework. It might want something that is similar to
getters/setters in Java. If this is the case, such a modeling is problem not
going to work since the multiple params will break the spec.

~~~
petercooper
As an integral convention of Rails, it does not (to my best knowledge). As
something you could add yourself or package up into a plugin, sure. It just
wouldn't be the _conventional_ approach (which, as we're discovering, is
stupidly insecure and patching things over while targeting the wrong problem).

------
niclupien
It's definitely the responsibility of the developer to know the security
vulnerabilities and the guidelines of the tools he uses. Every serious
frameworks have documentation on them and it is generally easy to find.

If the developer choose to ignore it, is the framework responsible of his
action? I don't think so. Like other commenters have said, this is a
beginner's mistake and they happen all the time. I don't understand how Rails
can be blamed for this. They have done their duty by documenting the issue and
it's easy to find (tell me who develops in Rails and is unaware of these
guides?)

By the way, this feature is also known in Spring MVC
(<http://www.springsource.com/security/spring-mvc>) and affect frameworks
based on it too (ex: Grails). They state this is a "usage issue" and not a bug
in the framework.

------
jayferd
Totally avoidable via `Hash#slice`. Come on guys, this is common knowledge
now.

    
    
        def update
          find_pk.update_attributes(params[:public_key].slice(...))
        end

~~~
ajross
Missing the point. You have a code idiom that is insecure by default. Leaving
out the slice produces _working code_ with a critical vulnerability. Mistakes
of omission are routine; people forget stuff all the time. The goal of a
secure framework is to be tolerant of that kind of goof.

This API sucks rocks.

------
minikomi
Quickly formatted in a gist (only slightly tongue in cheek):
<https://gist.github.com/1975850>

------
barumrho
I am not familiar with Rails, so I am a bit confused about what caused this.
What does it have to do with mass-assignment?

~~~
callumjones
Rails has the ability to protect certain fields from mass-assignment; fields
where you don't want the user setting values during POST because they may be
able to alter the security of that model.

Assuming this guy is right; the pub key class was allowing any old user to
modify the owner_id of the pub key object and change who it belongs to. The
pub key class wasn't configured to protect against mass owner_is assignment.

------
mofey
Love the comment by dbounds: "Nice work. FULL DISCLOSURE"

------
instakill
The ad hominem attacks are despicable.

------
aneth
This has been Rails behavior from day 1. Rails seems to assume that people
will make some level of effort to secure their application before deploying to
production. There are many ways and places to protect models, and mass
assignment protection is a blunt tool that would not have worked for github,
so the default behavior is not the issue.

This bug could occur in any framework where someone assumed all attributes
submitted are writable by the current user. Rails has no internal concept of
users or roles, so building that by default into a model makes no sense.

This is a github bug, not a Rails issue. One could argue it's a questionable,
but defensible, decision in the Rails framework, to have such an easy way to
take every submitted field and apply it to a model. I'd argue that using such
a feature in a production app is a fault of the developer for failing to read
their own code, because it's rather obvious and clear what the code does:

@product.update_attributes(params[:product])

Does exactly what you'd expect it to do.

~~~
gurkendoktor
(I didn't downvote you)

> Does exactly what you'd expect it to do.

Given the existence of attr_accessible, attr_protected, a global on/off switch
for the default behavior _and_ nested attributes, you cannot tell what that
line does without knowing the contents of at least two files.

And Rails 3.1 _does_ have the concept of roles (not necessarily of users),
baked into the same mass assignment logic into the model via the :on argument.

I'm not saying that this is _all_ bad or a bug even, but I don't see how this
is trivial to the reader either.

~~~
aneth
I agree there are a number of complex issues here, however that line of code
still obviously takes ALL content from the outside world and directly updates
a model with it. Any developer who does not spot that as a security issue is
probably creating many others as well. Rails can not protect against
developers not understanding that form submissions can contain any content and
should not be trusted or applied directly to models without understanding
what's happening. A cautious developer would slice up the submission to update
the model with only the expected or allowed fields.

I just learned of the new role feature for attr_accessible because of this
controversy. This seems to solve one of the major issues with attr_accessible
- that different controllers and users need to update different attributes, so
any somewhat complex app would end up widening it's attr_accessible attributes
beyond what they should be.

These are still blunt tools though - what if only superadmin users can update
a role column to superadmin, but admins can update it to admin or guest. This
requires more extensive logic in the controller than simple attribute
filtering, demonstrating why this filtering really belongs outside the model.
Despite that, I think the new "role" based attr_accessible probably covers
most cases and seems quite useful.

For all we know, GitHub may have been using attr_accessible but have expanded
it to include columns updatable by admins.

Thank you for the thoughtful comment - perhaps there is hope that HN hasn't
been entirely taken over by people talking out of their asses.

------
omgsean
I find it really surprising that Rails is taking heat for this. "Protect your
attributes" is something you learn really early on, and most of my models will
have a spec along the lines of "as a user, I can't steal another user's _____"

On top of that, public facing code should be written like

    
    
      def update
        @pk = current_user.public_keys.find(params[:id])
        # do the update if you find the key
      end
    

Simple stuff.

~~~
homakov
It won't help. After @pk.update_attr(params[:pk]) you drop mailicious pub key
to user params[:pk][:user_id] no way

~~~
omgsean
Ah that's right, in this case he's trying to make something that belongs to
him belong to someone else. Regardless, something like user_id should be
protected and really if you're setting up a website whose primary audience is
made up of hackers you should be whitelisting on every model.

