How github was hacked

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

[sankara]

This is one of the most careless mistakes devs make especially those not so experienced with security. Not without a reason it is there in the fourth spot of OWASP top ten: https://www.owasp.org/index.php/Top_10_2010-A4

I really love Github and have been trying to get it adopted in my organization. After the recent events though I'm having second thoughts. I don't think any application is 100% fool proof. But a well known vulnerability; one that is always brought up in any audit, going unnoticed for so long? I honestly did not expect this from Github.

[yaix]

Not really laughing. I don't really call myself a programmer, but I am always amazed at these kind of simple-but-dangerous mistakes. Accepting a user_id as a lookup value for a DB update? What for! Take the session_id and look up the user_id from the session table! If required, check if the authenticated user has admin level or "change any user account" rights and only then accept a user_id as a POSTed input.

[bascule]

It's a write that shouldn't be authorized but was. It's an access control vulnerability, and they happen all the time.

[dasrecht]

that were my first toughs too... but sometimes a little hack hasn't to be complex ;)

[unicron]

That appears to be standard practice for "model bound" systems where the internal "domain model" is bound directly to the HTTP protocol. You get this issue with ASP.Net MVC as well on most of the trivially implemented projects out there.

This is not really a "framework bug" as such. It's just crap application architecture.

At the core of this issue is a simple misunderstanding on what the "model" part of MVC actually is. The model represents ONLY the request or "form model", not the data model. There should be a mapping of 1:1 between the request and the "form data model" always with no exception. The controller is responsible for translating that into something useful or doing something with it involving the domain/data model.

Unfortunately where the usual CRUD approach is required, ignorance reigns supreme and the shortest path, not the most correct path tends to appear.

I write this as someone who works on a rather large ASP.Net MVC application (100+ controllers) and has seen this many times already.

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

[sophacles]

I agree with your sentiment, there is no need to insult the guy on such a personal basis. Unfortunately all communities are full of people, some of whom will respond like this to situations. It is no excuse, and I don't condone the behavior. I do wonder tho, if it isn't because people are scared of their software being hacked. People frequently respond poorly to fear.

What is going on here? I think the fear people have is that this attack is laughably simple, but at the same time, was not noticed for a long time. It says something about complexity, all the interlocking parts may have simple problems that are hidden by the abstract models used. It reminds us that the law of unintended consequences are always in effect, and we need to really think out what we are doing, even if the tests all pass. It shows us that even good, well respected software is vulnerable. And it makes us wonder what we have done to open security holes and what is lurking in our code. No one wants to be responsible for such a thing, and it is not pleasant to think about potential consequences of them.

[mkramlich]

Keep in mind that "on the Internet, nobody knows you're a dog." In other words, some commentators on any website may be children or teenagers.

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

[smsm42]

Well, expecting popular blog with open anon access not to have trolls is rather naive. The reality is, however sad it seems, that there are thousands of people out there that enjoy this exactly type of behavior. And some of them, oh horror, may know how to program and read the same blogs normal people do. And until technology allows to find and punch somebody in the face by IP, they'll be around. They don't represent any community, no more that a bird crapping on your car represents the local community.

[muyuu]

Textbook case of inferiority complex.

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

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

[getsat]

#attributes=, #update_attributes, #assign_attributes=

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

[jneen]

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.

[brown9-2]

read http://blog.mhartl.com/2008/09/21/mass-assignment-in-rails-a...

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

[driverdan]

Agreed, this is how most frameworks work. They rely on the developer to put in proper security controls. One of the first things I do after I start a project from scaffolding is go through and filter out values I don't want the user to set such as user_id.

This isn't to say it should be this way, just that it's pretty standard behavior.

[aneth]

Interesting. I work with Rails every day. I understand and explain the source of the bug, and why it has nothing to do with some oversight by Rails. I'm downvoted.

Every other answer, often admittedly, is written by someone who doesn't know anything about Rails, but jumps on the "oh geez Rails has a terrible security hole" bandwagon.

What has happened to this place?

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

[gurkendoktor]

Learning to avoid register_globals and magic_quotes and SQL injections in PHP is what brought many people to Rails. It's sad to see similar choices being made there too.

[homakov]

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