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.
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.
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.
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.
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.
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.
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.
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."
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..."
> 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.
"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
"4. If you don't have functional English, learn it.
"As an American and native English-speaker myself, I have previously been reluctant to suggest this, lest it be taken as a sort of cultural imperialism. But several native speakers of other languages have urged me to point out that English is the working language of the hacker culture and the Internet, and that you will need to know it to function in the hacker community.
"Back around 1991 I learned that many hackers who have English as a second language use it in technical discussions even when they share a birth tongue; it was reported to me at the time that English has a richer technical vocabulary than any other language and is therefore simply a better tool for the job. For similar reasons, translations of technical books written in English are often unsatisfactory (when they get done at all).
"Linus Torvalds, a Finn, comments his code in English (it apparently never occurred to him to do otherwise). His fluency in English has been an important factor in his ability to recruit a worldwide community of developers for Linux. It's an example worth following.
"Being a native English-speaker does not guarantee that you have language skills good enough to function as a hacker. If your writing is semi-literate, ungrammatical, and riddled with misspellings, many hackers (including myself) will tend to ignore you. While sloppy writing does not invariably mean sloppy thinking, we've generally found the correlation to be strong--and we have no use for sloppy thinkers. If you can't yet write competently, learn to."
Not that being a "Rails Rockstar" necessarily implies being a Hacker--what I like about your quote is that it shows the phenomenon is everywhere. One nice thing for programmers though is that with online text communications you can hide a thick accent that would otherwise be held against you even if what you speak is grammatically perfect.
Some people are simply not good at languages, while at the same time they are extremely skilled coders. I know many people like this, in particular Japanese, Chinese and Russian-speaking coders. It's apparently quite a task to learn proper English when your native language is one of these and you weren't exposed to English soon enough.
Dismissing this guy because of his poor English is plain and simply xenophobic prejudice. In this case it came back to the Rails/Github guys and bit them in the ass.
Teaching standardized languages helps to reduce discrimination. Discrimination against African Americans (in general) is rare now, but discrimination against people who speak Ebonics is high. So is discrimination against people who sound Hispanic, and to a lessor extent many regional accents.
If SWE is taught well, it helps people from poor neighborhoods get taken more seriously. If it wasn't taught, only people from richer neighborhoods would know it, and would be used by rich people to exclude upstarts.
That said, using SWE to discriminate is a bad thing. It's just a little less unfair than discriminating based on the fluency of a dialect that poor kids never had a chance to learn.
I agree with all you're saying here, but I just want to point out that it doesn't attack the source of the problem.
A 19 year old Russian male who wasn't taught English properly will need A LOT of effort to learn proper SWE. For some people, it will simply not be feasible. Ability to learn human languages - as opposed to computer languages, much easier to an introvert/math person - is a very distinct ability and you'd be surprised how little correlation it has to intelligence and other skills.
Help reducing discrimination by teaching SWE is akin to help fight discrimination against other races by cosmetics and plastic surgery. Sure, it works on an individual level, but it doesn't help the people who truly have a problem: the ones discriminating.
You're right - it's a band-aid (or maybe a bandage, given that it can be effective) - it's fixing a problem that shouldn't be there. But if it allows some well-spoken members of minorities (i.e. Obama) to fly under the radar, succeed, and set a good example; so it does help fight discrimination.
Yeah, this was quite a revelation to me when I realized it. I have a bias towards this dialect and an instinctive bias against, say, AAVE since it sounds "uneducated". I've been trying to correct this, thinking about how, say, British English sounds different and has different grammar but I don't think any less of it or its speakers.
Does anyone have links to great videos of educated, intelligent discussion in typically looked down-upon accents (e.g., AAVE, deep south, etc)? For instance, I read once that the Queens accent was stigmatized until Feynman became so popular. When I hear videos of him, I notice his distinct speech, but it sounds "smart" because Feynman speaks it!
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)
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.
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.
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.
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.
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?
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
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
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.
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.
It's naive and it's not exactly an ACL or anything but it's a way to indicate the context at a basic level.
Sadly, people seem to be running around like headless chickens trying to scotch tape trash bags over a broken window instead of learning how to replace the glass.
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.
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.
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).
Precisely because there can be so many places (controllers) that can access the model, usually you use declarative access control rules on the model to control access in one place. It can be done with role. Then whoever user having the role can access the model.
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.
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.
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.
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:
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.
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.
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.
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.
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
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.
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.
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.