
How Homakov hacked GitHub & the line of code that could have prevented it - petenixey
https://gist.github.com/1978249
======
danso
I hope those who ripped into Homakov initially are feeling more mellow towards
him. They certainly were justified in thinking him being rash, but his action
raised far more awareness about this issue than the typical proper channel.

Rails devs (some, not all of them) had dismissed his complaint because "Every
Rails developer hears about attr_accessible." Well, I'll be the first to say
that I can't remember the last time that this update_attributes vulnerability
had been pointed out to me. I certainly can remember all the times that Rails
docs reminds me to use their sanitizing helpers when making ActiveRecord
queries.

To be fair, I haven't developed apps that required the use of user-facing
access to update_attributes, and maybe when I got around to using that, I
would've wisely consulted the dev guides to make sure I was following best
practice. But knowing me, I probably would've likely thought, "Well, that
seems simple enough, here goes."

It's not that the logic behind this vulnerability is hard to understand...in
retrospect, it's as clear and blatant as the processes that lead to SQL
injection.

But surgical patients die because elite surgeons sometimes forget to wash
their hands (Google "Atul Gawande checklist"). It's not an impossibility that
a skilled dev team would overlook the update_attributes issue.

The Rails team was right in arguing that this wasn't a security risk given a
half-competent dev. But they were looking at the problem from the wrong
perspective and assumed that everyone is as familiar with Rails best practices
as they were. So how else could Homakov convince them otherwise other than
pricking a high-profile dev group?

What if Homakov managed to alert the Github team, and they managed to fix it
quietly? Github would be safe but thousands of Rails sites would still be
operating in ignorance. It truly stinks for the Github group that they had to
respond to a five-alarm emergency on a Sunday...on the other hand, I think
there are going to be a lot of Rails devs who are thankful that they
(involuntarily) took one for the team. Thanks to Homakov, it was a small hit.

~~~
jarrett
While I mostly agree that the Rails default should probably be changed, I
think in fairness it's also worth pointing out that the Rails team _does_ give
fair warning about this issue. Specifically, anyone who reads the official
Rails Security Guide back-to-back will know about this:

<http://guides.rubyonrails.org/security.html>

One might counter that nobody reads these guides back-to-back. I must admit I
haven't read every word of every Rails guide. But I have read the security
guide in its entirety, and I think every developer owes it to herself and her
clients to do the same.

~~~
danso
I think that every competent dev knows that something like update_attributes
is inherently dangerous. So they probably keep it for backend
import/maintenance tasks. At some point, someone copies the code/wrapper-
function into the front-end.

The status quo of Rails is that everything is sanitized if you use the
helpers. And validators on the models only look at data integrity. So all this
protection, at least for me, kind of lulls you into feeling secure, because
SQL inject is generally the typical, awful-case scenario.

update_attributes is not really a SQL inject attack vector (since the actual
values are sanitized)...it's partially a social engineering scheme.

~~~
jarrett
Actually, I do use update_attributes for public-facing interfaces. But only
with attr_accessible. (I never use attr_protected, since blacklists are a
disaster waiting to happen.)

------
jarrett
Homakov kind of gave the impression he had discovered a previously unknown
vulnerability in Rails. This is not the case. Rather, he discovered an
instance in which one very prominent Rails app (Github) failed to implement a
standard Rails security practice.

For those not familiar with Rails, it boils down to this: You as the
programmer need to use a security feature built into Rails called _mass
assignment security._ If you fail to use this feature, you have a
vulnerability. In other words, the _default_ is insecure by design. The
alternative would be to make Rails secure by default, but that would mean
pretty much nothing would work until you explicitly granted access where
necessary. I guess the core team figured "not working by default" was worse
than "insecure by default."

Homakov obviously disagreed with this design decision. I can understand why,
and I mostly feel the same way.

So Homakov posted an issue to the Rails repo Github
(<https://github.com/rails/rails/issues/5228>) suggesting the default be
changed. He made a good case and was initially polite. A few days passed, and
nobody else had posted to his thread.

So, presumably to draw attention to this issue, he exploited the fact that
Github had failed to use mass assignment protection. Specifically, he posted a
comment with a far-future timestamp, which obviously should be impossible. (I
think that's what he did, although Github seems to to have fixed the timestamp
now.) He then said this should be proof enough that the Rails defaults need to
be changed.

The problem with Homakov's argument, as pointed out in subsequent comments in
the thread, is that Homakov's hack only demonstrated a mistake on _Github's_
part, not a bug in Rails. It didn't prove anything about Rails that we didn't
already know. The only thing surprising he demonstrated was that Github had
left open a rather serious vulnerability.

TL;DR: Rails has some less-than-secure defaults which all Rails developers are
expected to understand and deal with. Homakov found out that Github failed to
do so in at least one instance, and he wanted to use that as proof the Rails
defaults should be changed.

~~~
pbreit
I don't like the gist of your post. You are giving Rails way too much of a
pass and assigning too much blame to GitHub. A framework should not be
"insecure by design". Period. GitHub engineers are likely about as good as
they get and still missed it. That's less an indictment of GitHub engineering
and more so poor decision-making on the Rails side.

~~~
jarrett
I agree with you that the default should be changed. And note that any
application developer _can_ in fact require attr_accessible globally, across
all models, with one line of configuration.

The point of my post was not to give Rails a pass, and I apologize for
misleading if it came off that way. Rather, I was trying to clarify the
situation for those who are less familiar with Rails. The discussions
surrounding this issue (including Homakov's own words) seem to erroneously
suggest that Homakov discovered a previously unknown vulnerability in Rails. I
was merely clarifying that he instead found a vulnerability in _a specific
Rails app._

Now, it's a matter of opinion as to whether the Rails default should be called
a "vulnerability." I say yes, but reasonable people can disagree. What's
clear, though, is that no _previously unknown_ vulnerabilities in the
framework have been revealed.

~~~
javascriptlol
No. A property of a system that leads to compromises is a vulnerability. Let's
not let give in to neo-essentialism. Just because the word "vulnerability" has
been assigned some narrow meaning in the past is no reason to enforce that
usage. This is a problem, and "vulnerability" is plainly as the sun shines the
right word for it.

~~~
einhverfr
Put another way, security problems are as much architectural problems as
programming mistakes. If the architecture is secure, the programming mistakes
will be less severe. Start with a good architecture and the rewards, security-
wise, will be substantial.

This is the problem. We think of a security problem as "the developer made a
mistake." Often it's the software architect who made the mistake, and if we
insist that frameworks weed out the bad architects we are all better off.

------
jazzychad
Is it really frowned upon to do:

    
    
        user.name = params[:user][‘name’]
    

?? Call me old fashioned, but this is called 'defensive coding' and should (in
my opinion) be the norm when dealing with client-generated input. It might be
more verbose and not 'The Rails Way', but update_attributes seems like too
much magic for my paranoid taste.

~~~
danso
It's not frowned upon, but it would require tighter coupling between the
controller and the view...?

~~~
jazzychad
Yes, that makes sense. If your view's form should only update the user's name,
then the logic that handles that form submission should only update the user's
name. Allowing the client to inject other attributes (even allowed/whitelisted
ones) could potentially break other app logic.

------
jgrahamc
It's surprising to me (not a very experienced Rails developer) that the
default behavior should be so open to abuse.

Why isn't the default the opposite?

~~~
Peaker
Because convenience is deemed more important than safety.

Same reason that "enum" and "int" are pretty much interchangeable in C, that
arithmetic conversions and truncations are implicit -- it's not very safe, but
it is more convenient.

~~~
baq
you know, i've heard that from php. it was damn convenient in version 3.

we all know how it ended up.

------
LargeWu
Just a heads up for those who only read the TL;DR: Please be aware that if you
just put that single line of code in your initializers for your existing app,
your app will break anywhere you are using update_attributes(). They do call
it out later in the article, but you have to set attr_accessible on all your
models.

~~~
petenixey
Good flag - I've made the warning even clearer in the Gist

------
SpoonMeiser
Is it just me, or doesn't this sound very similar to SQL injection, only as
applied to an ORM instead?

That is, if my understanding is correct, they're taking user posted data and
trivially turning it into a command to update data.

This doesn't sound like a problem with Rails, in the same way that if I turn
data I receive from the user straight into an SQL statement, the fact that
people can abuse it isn't a problem with SQL.

~~~
tedunangst
It's a problem with Rails because Rails provided the update facility that
takes user data and told people "if you use this, you can build a blog in 15
minutes." If you're turning user data into SQL, at least you're the one who
wrote the code. Your database didn't come with a parse POST and update table
function.

------
yumraj
I must say that this episode is the best example of how to handle such cases.
Homakov found the issue and made his point without any sign of maliciousness.
Github, also handled is extremely professionally by accepting it and fixing
the problem and then publishing a full report on it, rather than get into a
pissing match with Homakov and getting law enforcement and lawyers involved.

BigCo's should take a note.

~~~
rickchristie
Read his blog: <http://homakov.blogspot.com/> \- GitHub suspended his account.

~~~
technoweenie
It was a temporary suspension while we investigated the scope of the damage.
There was some other weird activity on his account that he wasn't up front
about.

------
adriand
I agree that the default for Rails should be more secure, but this was a
really basic mistake by the GitHub team. Yes, mistakes do happen, but there's
a very simple way to avoid the exploit that is postulated in this article.

The 'schematic' of what the public key update looks like from the original
post:

    
    
        class PublicKeyController < ApplicationController
          before_filter :authorize_user
          ...
          def update
            @current_key = PublicKey.find_by_id params[:key]['id']
            @current_key.update_attributes(params[:key])
          end
        end
    

The correct way to code this is as follows:

    
    
        class PublicKeyController < ApplicationController
          before_filter :authorize_user
          ...
          def update
            @current_key = current_user.public_keys.find params[:key]['id']
            @current_key.update_attributes(params[:key])
          end
        end
    

This has two updates to it that protect against this exploit:

1\. Rather than calling "find_by_id" on the PublicKey model, which searches
all public keys, you call it on the current user's list of public keys. This
scopes the search down to the public keys that they own. Thus, if you pass in
the id of a key they do not own, it will not be found, leading us to:

2\. Using "find" instead of "find_by_id" will trigger an
ActiveRecord::RecordNotFound error (404) if the resource is not found. Of
course, find_by_id will just return nil in this instance, so the
update_attributes part would still fail, but triggering a 404 is an easier,
cleaner way of dealing with this, I think.

It's really very simple: you don't let people access stuff they don't own.

Now, this does not protect you against faked timestamps, or against privilege
escalation by passing in a faked "role" parameter, and so on. You still need
to use attr_accessible to protect yourself from that stuff, but scoping
resources down to the user who owns them is a simple technique that should be
standard practice for applications with authentication.

~~~
phamilton
That wouldn't fix it.

I own the key. I set the owner of the key to you. You now own the key that I
have on my machine. I commit to your repo.

I'm sure the github team has authorization at a model level, preventing access
to other user's resources. This wasn't an instance of accessing another user's
resources via rails, it was assigning your own resources to another user, then
abusing that fact via git.

~~~
adriand
Actually, that's true. I stand corrected. I think I was distracted by the
rather obvious issue in the example code that the OP posted, which did not
include this basic protection.

------
gphil
I posted a link to the relevant part of it
(<http://news.ycombinator.com/item?id=3665429>) on another thread regarding
this exploit already, but the official Rails Security Guide covers this and
other common security pitfalls really well. It is worth reading over
thoughtfully if you are building a Rails app:

<http://guides.rubyonrails.org/security.html>

~~~
davidblondeau
Right, the Rails security guide and the Netscape secure coding guide
(<https://wiki.mozilla.org/WebAppSec/Secure_Coding_Guidelines>) are highly
recommended reading for every developer at my company. They are very well
written and cover a lot of ground when developing web applications on RoR.

------
feralchimp
I'm not a Ruby or Rails guy, so I can't comment on how likely a typical (or
atypically good) Rails dev might be to code this sort of bug into their app.

But as a security guy, given the power of Public Key assignment in the context
of a system for managing access to Git repositories, I can't help but be a
little surprised that model objects that touch Public Keys weren't more
thoroughly reviewed.

If nothing else, folks everywhere will be thinking a little harder about
authorization logic this week.

------
rmc
I'm a Python/Django developer and don't really know much Ruby or Ruby on
Rails. Does anyone have an outsiders/non-rubyist explaination of how this hack
was carried out? Did he modify HTTP headers? POST parameters?

~~~
carlio
From what I can tell, the 'rails way' of dealing with form data is something
akin to user.__dict__.update(request.POST). So you can just add some
additional parameters to your POST data and set arbitrary fields on the model.

~~~
danso
I don't think this is the best way to describe this. It is not the "Rails Way"
to NOT validate form input. The Github team used a method that allows quick
and easy assignment of bulk values. This method is used often in non-public-
facing tasks but when it is used in connection with a public facing POST
request, then validation logic __should __be implemented.

It was not, in Github's case. It seems like a glaring error in retrospect but
it's easy to see how this code (or the pattern) would move over from the
private to public-facing interface and not trigger any errors or notice.

------
kristofferR
I wonder how many Rails apps there are out there that is still vulnerable to
this sort of flaw. Both GitHub and Posterous has fixed it, but there's
probably thousands of smaller less known Rails sites/apps that still haven't
been patched.

~~~
danso
I know I immediately panicked because I know this was not one of the many
issues that I had thought to be aware of. Luckily, I remembered that I've just
out of habit/circumstance not used update_attributes...so I avoided the bullet
out of dumb luck.

I imagine there are many, many sites that are vulnerable to this. I hope the
high profile hack (at least on HN) quickly spreads the kind of panic that gets
other devs to check their repos today.

~~~
dean
Although the OP talks about update_attributes, the problem is not limited to
that method. The problem is with "mass assignment". So, if you are passing a
hash of attributes and values to the new or create method of a model, then you
are doing mass assignment, and you are exposed to this issue.

For example, if you are doing this in your controller:

    
    
      @user = User.new(params[:user])
    

then you are doing mass assignment.

------
InclinedPlane
I find it hilarious that just in the last few days they've removed
register_globals from PHP for good. It seems every language/framework ends up
retracing the same set of security/convenience tradeoffs over and over again.

------
Estragon
Since this vulnerability has been known about for years and experienced
crackers have presumably checked for it before, is there any reason to be
concerned about more malicious corruption of other repositories on github?

~~~
phamilton
The problems with update_attributes have been known, and messing with things
like timestamps probably has been done before, but Rails itself didn't violate
any acls. All users still can only access their own resources via Rails.

This attack was a combination of Rails and git/ssh keys. There's a bit of a
clever aspect to it, one that isn't implied merely by understanding the
vulnerabilities of update_attributes. While I think it is likely someone else
has thought of it before, it is a little more exotic than something your
average cracker is going to try.

~~~
danso
Really? How is this exotic? Simple inspection of the rails form will reveal
attribute and model names. And as Homarov proves, you don't need to even leave
the friendly interface of the inspector to POST what you want.

The only reason why a cracker hasn't tried this is because it seems too simple
to work.

~~~
javascriptlol
That is, assuming it hasn't been tried before. We don't really know.

------
jimworm
DHH does it in half a line: <https://gist.github.com/1975644>

`attr_accessible` should only be used to protect the attributes that are NEVER
modified by users. Access to the rest of the attributes may differ by user
role, and should be handled by the controller. Trying to use `attr_accessible`
to protect everything leads to enough frustration to make one eventually give
up on security.

~~~
marshray
_`attr_accessible` should only be used to protect the attributes that are
NEVER modified by users_

But web app developer may not be able to know in advance what new columns
might be added on the database, possibly by some other team. If I understand
this right, in the absence of attr_accessible, any new columns are completely
writable by the HTTP request.

So having a default-deny whitelist approach is the only sane strategy.

 _Trying to use `attr_accessible` to protect everything leads to enough
frustration to make one eventually give up on security._

Or give up on Rails. Usually the basic security of the database is not
negotiable.

------
madarco
It's sad that this same bug was resolved in CakePhp _7_ years* ago:
[http://groups.google.com/group/cake-
php/browse_thread/thread...](http://groups.google.com/group/cake-
php/browse_thread/thread/03ce2bc624d335b9/eeb5f847369a74d1)

------
o1iver
"Homakov PUT an update to his own existing public key which included a new
user_id. The user_id he used was that of a member of the Rails repository
members."

But wouldn't that mean, that the commit would display the username of the user
with the `user_id' that he used?

~~~
gcampbell
Github displays the username/avatar for a commit based on that commit's Author
field, not the user that pushed the changes to the repository.

~~~
ajross
It's more that they display both. Unlike git core, github actually tracks
"push" events to branches (git doesn't care, it only sees commits) as distinct
from the commits they contain.

So presumably it would have shown the rails developer pushing a change
authored by Homakov.

~~~
technoweenie
Git has no real way to track that, only commits. We do track it internally,
but AFAIK it's not exposed except through post-receive hooks.

------
pdufour
I don't find this a good solution. A much better solution is to generate a
list of form inputs that are present on a page, then when the form is
submitted, check if any of the form inputs were changed / any were added.

~~~
obtu
Agreed, whitelisting attributes regardless of context is much too coarse. Here
is a form library that implements what you describe:
<https://news.ycombinator.com/item?id=3666907>

------
forza
GitHub really needs to clearify if access to private repos was compromised,
for how long and if such access would be traceable.

~~~
why-el
Between the time Homakov made his work public and the time it took them to fix
it? I think it was close to an hour and on a Sunday. I doubt there are any
other cases.

~~~
cbs
And we're confident that this guy is the only guy to ever exploit that
weakness on github in the last 4 years?

~~~
why-el
No, we are not. My bad, I misunderstood the statement; I thought he was
referencing yesterday only, which I thought might not be possible, given the
sort amount of time. Obviously if other people knew that's a whole different
story.

------
Shanewho
Is the solution given in the article any different than using this?

config.active_record.whitelist_attributes = true

Also, this isn't the first time someone's been bit by this:
[http://www.kalzumeus.com/2010/09/22/security-lessons-
learned...](http://www.kalzumeus.com/2010/09/22/security-lessons-learned-from-
the-diaspora-launch/)

~~~
petenixey
I didn't know about the solution you mention at the time I wrote the post. I'm
reading up on it now and I think it's actually going to end up being the
official solution so it's a good flag:

[https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac...](https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac53c57a302b162fd736)

------
lattepiu
The real culprit here is HashWithIndifferentAccess, a crutch that throws away
a difference that Ruby has for a reason.

The sensible way to do updates, in my opinion, is

    
    
        User.update(:name => params['user']['name'])
    

But there's no way in Rails to keep that syntax while disabling

    
    
        User.update(params['user'])

~~~
bluesnowmonkey
clutch -> crutch

------
unclegene


------
unclegene
Am I the only one who does not understand what is this about? Oh, no, looks
like rails team does neither. Stupid code can be written in any
framework/language. How much experience does one need to understand a simple
rule - _never_ use user input directly. If you have an urge to trust your
users - I'd suggest better way: `params[:command]`

