7 ms·
How github was hacked
- jtchang 15y agoIs 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.
- dasrecht 15y agothat were my first toughs too... but sometimes a little hack hasn't to be complex ;)
- deleted 15y ago[deleted]
- Jach 15y agoSeriously. 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 15y agoA 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.
- rsynnott 15y agoIt's a common problem in Rails apps and has been for some years. It's a bit of a beginner mistake, though.
- bascule 15y agoIt's a write that shouldn't be authorized but was. It's an access control vulnerability, and they happen all the time.
- yaix 15y agoNot 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.
- sankara 15y agoThis 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 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.
- unicron 15y agoThat 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.
- peregrine 15y agoSeems strange that Active Record doesn't have an "update_fields" method with sig array of updateable fields, hash of field=>val. Similar to Sequel.
- getsat 15y ago#attributes=, #update_attributes, #assign_attributes=
- callumjones 15y agoBecause it's really the Model's job to determine what is safe and unsafe for mass assignment, not the controller's.
- skimbrel 15y agoBut 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.
- chrismsnz 15y agoI agree with you, I imagine your parent's reply was could have been sarcastic that got lost in text. It's really an authorisation issue as to who/what can update which parts of a model - generally this is handled in the controller.
- callumjones 15y agoI'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.
- sjtgraham 15y agoI 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 https://github.com/stevegraham/default_whitelist
- wtn 15y agoThis feature already exists in Rails. https://github.com/lest/rails/commit/f2fa4837a8a888ee86997be892d0aa5bbd2b5fd0 https://github.com/lest/rails/commit/f2fa4837a8a888ee86997be...
- ortatherox 15y agoI'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 15y agoI 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 15y ago> 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 15y ago"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
- barumrho 15y agoI am not familiar with Rails, so I am a bit confused about what caused this. What does it have to do with mass-assignment?
- brown9-2 15y agoread http://blog.mhartl.com/2008/09/21/mass-assignment-in-rails-applications/ http://blog.mhartl.com/2008/09/21/mass-assignment-in-rails-a...
- callumjones 15y agoRails 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.
- minikomi 15y agoQuickly formatted in a gist (only slightly tongue in cheek): https://gist.github.com/1975850 https://gist.github.com/1975850
- kalleboo 15y agoTo 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 15y agoEarly PHP is exactly what came to mind when I read the description. Nothing new under the sun.
- rdl 15y agoI 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 15y agoComment 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.
- jayferd 15y agoTotally avoidable via `Hash#slice`. Come on guys, this is common knowledge now. def update find_pk.update_attributes(params[:public_key].slice(...)) end
- ajross 15y agoMissing 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.
- mofey 15y agoLove the comment by dbounds: "Nice work. FULL DISCLOSURE"
- omgsean 15y agoI 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 15y agoIt won't help. After @pk.update_attr(params[:pk]) you drop mailicious pub key to user params[:pk][:user_id] no way
- omgsean 15y agoAh 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.
- gurkendoktor 15y agoLearning 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 15y agojust 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 15y agoхех :) молодец
- kellenfujimoto 15y agoThe comments are rather classy too. Glad to see xenophobia is alive and well.
- gphil 15y agoHere'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 http://guides.rubyonrails.org/security.html#mass-assignment
- mattlong 15y agoI'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 15y agoHere'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
- aneth 15y agoThis 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.
- driverdan 15y agoAgreed, 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 15y agoInteresting. 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?
- gurkendoktor 15y ago(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.
- instakill 15y agoThe ad hominem attacks are despicable.
- coreyspitzer 15y agoRegarding 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.
- niclupien 15y agoIt'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 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.