How github was hacked(homakov.blogspot.com)
homakov.blogspot.com
How github was hacked
http://homakov.blogspot.com/2012/03/how-to.html#
72 comments
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.
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.
It's a common problem in Rails apps and has been for some years. It's a bit of a beginner mistake, though.
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.
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.
It's a write that shouldn't be authorized but was. It's an access control vulnerability, and they happen all the time.
[deleted]
that were my first toughs too... but sometimes a little hack hasn't to be complex ;)
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.
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.
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.
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.
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.
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 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.
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.
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.
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..."
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.
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
- 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."
--Eric S. Raymond, How To Become A Hacker http://www.catb.org/~esr/faqs/hacker-howto.html
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.
"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."
--Eric S. Raymond, How To Become A Hacker http://www.catb.org/~esr/faqs/hacker-howto.html
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.
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.
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.
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!
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!
I think he is trying to say http://en.wikipedia.org/wiki/Received_Pronunciation
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)
(there's also the accent/dialect/creyole/pidgin distinction, which often revolves around who has an army)
He's talking about the United States, and RP is distinctly British, so probably not.
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.
Textbook case of inferiority complex.
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
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
This feature already exists in Rails.
https://github.com/lest/rails/commit/f2fa4837a8a888ee86997be...
https://github.com/lest/rails/commit/f2fa4837a8a888ee86997be...
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?
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
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
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
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
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
http://guides.rubyonrails.org/security.html#mass-assignment
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.
The comments are rather classy too. Glad to see xenophobia is alive and well.
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.
Because it's really the Model's job to determine what is safe and unsafe for mass assignment, not the controller's.
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.
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.
Yes it is. But even ignoring that, Rails 3.2 added a feature to do something similar: roles. You can do things like this in the model:
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.
attr_accessible :user_id, :on => :admin
@something.update_attributes(params[:something], :as => :admin)
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.
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.
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.
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).
With regarding your second point, django middleware would be beneficial and does exactly that.
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.
I 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.
It's really an authorisation issue as to who/what can update which parts of a model - generally this is handled in the controller.
#attributes=, #update_attributes, #assign_attributes=
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.
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.
Totally avoidable via `Hash#slice`. Come on guys, this is common knowledge now.
def update
find_pk.update_attributes(params[:public_key].slice(...))
endMissing 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.
This API sucks rocks.
Quickly formatted in a gist (only slightly tongue in cheek): https://gist.github.com/1975850
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?
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.
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.
Love the comment by dbounds: "Nice work. FULL DISCLOSURE"
The ad hominem attacks are despicable.
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.
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.
(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.
> 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.
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.
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.
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.
What has happened to this place?
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?
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
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.
It won't help.
After @pk.update_attr(params[:pk]) you drop mailicious pub key to user params[:pk][:user_id]
no way
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.