Hacked: commit to rails master on GitHub
github.com
github.com
Merb's approach was to have mass assignment protection in the controller, and I personally think it's self-evident that it belongs there. Moving it into the controller will also make it easier to solve the tension between reducing the friction of getting up and running quickly and having good security defaults.
In general, Rails' convention over configuration make a stock Rails app more secure by default (CSRF protections, XSS protection, timing attacks, session fixation, etc.). This is a case where there's a real tension, but I think that we can solve it by applying some brainpower to the question.
But wasn't Merb merged into Rails? :) Sigh...
I always felt that "it's up to the developer to do the right thing" violates the normal Rails convention over configuration principles, but I also weigh breaking a large % of existing Rails apps in a way that is not easy to quickly fix heavily.
That said, this problem is almost identical to XSS protection. We were able to find a solution that mostly "just works" for new developers, with some caveats, but it broke nearly all existing apps in a way that required significant effort to fix.
Like mass assignment, previous vulnerabilities were caused by Rails defaults that caused most users to make mistakes (nearly everyone had at least a few cases where `h` was required but wasn't done).
Like XSS protection, we have a solution here that will mostly just work for the happy path. The end result is a Rails default that will be only marginal harder to use than what we have now, but secure by default.
But let me just add, fixing those when I moved rails 3 felt good.
It can be argued that those apps were already broken. Nobody should complain against a security fix.
Currently, I like to use a hack that automatically makes all models use attr_accessible with no allowed attributes. Until you override it with attr_accessible in your model, nothing is allowed, so you have to think hard about what should and shouldn't be accessible.
However, it causes real annoyance when not dealing with web input. Applying it to an existing project and fixing everything it breaks is super-annoying.
From my experience, the main (though easily side-stepped) annoyance is when creating or updating records that have belongs_to associations (for example, user_id and repository_id for a commit ;)) programmatically.
For security purposes, you would not set those 2 attributes to be attr_accessible. To create a new record, you then would have to build the record and then set the user_id and repository_id on the record.
Or, you can set user and repository to be accessible (attr_accessible :user, :repository). This is fine because the associated methods expect ActiveRecord objects.
Your proposed controller technique is great. This technique coupled with some intelligent defaults seems like a good idea, with respect to both sides of the story (both a secure-by-default and frictionless convention). I'm excited to see this in the limelight, and your proposal is a fantastic start to this conversation. Thank you sir!
It is. The more interesting question is why it takes rails 7 years (and counting) to come to this conclusion.
<rant>
I'll take it one further and sing my song about the Rails ActiveRecord implementation here, which is tangentially related.
The promise of AR is to reflect on the database at startup and then "magically work".
The problem in rails is that nothing magically works. What you get out of the box is an insecure world-writable model, as illustrated by this bug. Then you begin scattering your truth over incremental (hand-crafted!) migrations, the model, and the controller, until nobody other than a cascade of unit-tests can even make sense of the interdependencies anymore.
In a world where there is not even a central place to look up which fields exist on a model and what their constraints are - short of runtime introspection, where database constraints live happily alongside and independently of model constraints, where opaque bits of ruby-logic buried in various gems add their own magic to the mix, in such a world it's really no surprise they chose to default to "all fields writable".
Because if they forced the user to do the sensible thing and explicitly list the allowable fields then where's the advantage over a declarative approach (like e.g. Django) anymore?
Imho it's long overdue to take a step back and revisit whether AR is still worth having, or ever was. Imho it causes way more problems than it solves, in contrast to the declarative approach.
</rant>
Why end your rant with an insult to its readers? Please leave the martyrdom out next time.
AR was born with a set of very opinionated decisions. I believe those that prefer a more declarative approach (and built-in identity map) can use DataMapper.
class PostsController attr_accessible: :title, :body, :related_links => { :href, :title } end
This would accept the attributes: post_title, post_body, post_related_links_0_href, post_related_links_0_title, posts_related_ink_1...
The names might not be right. I forget exactly how rails names fields. But you get the point, yes?
I disclosed a vulnerability to GitHub before. I dropped it into their Issues system marked private with the heading "URGENT". It was a Sunday and I got a response + a fix from Tom Preston-Wener himself within a few hours. That, in my mind, would have been a more responsible approach.
Yes, Github had vulnerable code. It's also true that Rails apparently defaults to leaving that bit of code vulnerable. A saner default seems in order, if even highly competent Rails devs can be caught by this.
"Well, it's a beginner-level PHP mistake; it is not precisely an obscure issue. Googling 'SQL Injection/register_globals/get_magic_quotes/etc' will show you discussion going back years, and it has been actively exploited before. I'm shocked that the mistake was made in ..., though."
Ten+ years of register_globals, get_magic_quotes and SQL injection attacks (etc etc etc) in PHP show that even well-known issues still bite people everyday. IMO, it's up to the framework developers to make it easy to do the right thing and damn hard to do the wrong thing.
Mass-assignment is very difficult to get right in any complex app (e.g. with multiple user roles) and the simplest way to avoid these problems would be to discourage the use of 'update_attributes' and teach people to explicitly set model properties.
(Of course, Rails folks probably hate the non-DRY aspect of this, but I prefer to look at my code and see exactly what's happening with user input.)
It's a Github issue no matter what the cause or responsible party.
If Github was run on unpatched windows XP boxes with all IPs public, you wouldn't argue that it was Microsoft's fault because "everyone knows" what a bad idea that would be. The base assumption should not to be completely reliant on your environment and frameworks to be secure, because they very likely have un-patched bugs and exploits. You do what you can to harden yourself up, but when something goes wrong - it's your problem.
And it would have ultimately been Github's fault (in the eyes of their users) in that case too -- because they are providing the service, and their choices (XP / Rails, Firewall / Application Firewall / Audit) is their fault.
Contrary to how a lot of things are handled today, fault can be shared by multiple parties. what Egor did was get exposure and reaction from a lot of involved parties, where he was unable to get any reaction from some of them (Rails team) in the past.
That might be unprofessional but it was also audacious - and easily the best way to get the word out quickly about an 'in the wild' exploit that will impact a huge number of apps.
Additionally: GitHub is a critical piece of infrastructure for a huge number of companies, contractors and start-ups. On balance, protecting GitHub from public embarrassment is far outweighed by the potentional impact of this sort of flaw. If a good old public "git@github ~ $ rake over coals" is what is needed to ensure that our IP is sufficiently protected, so be it.
https://github.com/rails/rails/issues/5228
Egor discovered that a lot of big sites which use Rails suffer from very serious security issues because the common Rails practices and defaults don't produce secure sites. It's so non-obvious that he was able to impersonate others, avoid most of the checks...
He was aware of consequences, but he got mostly ignored. So he decided he had to get enough attention and publicity. There are too many sites too vulnerable to just wait.
He submitted a security flaw to the Rails issue tracker. It was shut down by committers saying, "This isn't a real flaw, it's everyone's responsibility to secure their own apps."
At that point, a reasonable response is, "Yes it is, you dummies. Watch as I use it to pwn multiple high-profile production rails sites."
_rails_ _git_ _master_
Fred Wu
So what's a good gem (if any) for safe guarding params on the controller level? @dhh's params.slice feels too dirty.
DHH
yeah, it's too simple to be clean! I think you are using the wrong framework if you crave more complexity for its own sake.Understandably, Github would have liked much more to be one of those companies that will be able to quietly fix this vulnerability without anybody knowing, but now that the damage to their image is done I really hope they'll not add to that damage by persisting in their banning of a 18 year old that acted irresponsibly yes, but maliciously - definitely not.
From a PR perspective, I guess that having titles like "Silicon Valley company rewards Russian teenager who helps them eliminating a security risk" could even spin the episode in their favor.
Not just ignored but dismissed multiple times. It's not exactly surprising that he went and injected code into Rails's master.
If by adding a line or 2 to the code for generators can stop this, even if it includes a comment saying "Removing this line will do x y z", then I think the rails team could've treated the bug with a little more respect.
As @ericb said, if strong devs make this mistake, there's something wrong with the code.
I think it should also be noted that he didn't do anything malicious like trash repos, and even says on his blog:
"Then I could wipe any post in any project. That wasn't that funny but pretty
dangereous[sic]. It got more curious."
Finally: big props to the GitHub team for patching their vulnerability in <1hr on a Sunday...
I'd buy Github stock if I could.
Nor do I. However, there was no indication given that an explanation would be given through official channels. If that indication was made with the "fixed it" comment, trust me, I would have kept my mouth shut and waited.
(http://homakov.blogspot.com/2012/03/im-disappoint-github.htm...)
If this is true, would you please consider unsuspending him? This doesn't seem like a good way to reward this sort of behavior (i.e., helping through hacking).
It's not even being "rewarded"--it's being "not punished".
Wouldn't you like to have people spot vulnerabilities on your site and report them promptly without also breaking things? Seems a little ungrateful, yeah?
If we are all going to migrate to the cloud and assume our services (no longer under our control) are handled competently, we must place a higher premium on vetting that competency.
Yes. I'd even send him a thank-you email, and add him to our list of contributors.
I'd then of course contact the customer (probably over phone, as quickly as possible), explain the vulnerability, explain what happened, and explain how we were fixing it. Then I'd write a post about it, and put it on the front of the site.
That's how you do business.
Like that?
"Dear Grabastic,
Today we had a demonstration by a user (xxx) of a security vulnerability on our site.
${VULNERABILITY_EXPLANATION}
We believe that it is possible that your application or records are covered in the scope of the exploit, because of the fact that ${VULNERABILITY_APPLICATION}.
In order to fix this issue, we have ${VULNERABILITY_PATCH}.
We have thanked this user for their vigilance in spotting bugs and security weaknesses in our site, and they have been added to our contributors-security page here (link).
We have a stance that security is something that can only be improved by lots of inquisitive eyes, and so if you have seen any issues that concern you, please do not hesitate to inform us and/or demonstrate the vulnerability--provided, of course, you do so without breaking anything permanently. :)
Our service is better today than it was yesterday, and we hope that with the patience and openness of our users it will be still better tommorrow!
Sincerely, angersock "
See, not so hard!
+another showcase of rails apps vunlerability. 2 +Github pwned. again :( 3 +will you pay me for security audit?
It kinda sounds like you've succumbed to an extortion demand.
This isn't a 0-day vulnerability--this is an issue known to the rails devs, and one which they decided wrongly on. Sometimes you've got to take things to the next level of visibility.
Whether or not rails' defaults are sane is a completely separate question. Egor acted out against GH when he was ignored by rails dev. Two entirely different parties.
For a normal project, sure, whatever, but when the security hole is in a framework--especially one as widely deployed as rails--you eventually must cede that, yeah, the fastest way to get something serious fixed is to do a public exploit.
What if he'd submitted the fix to github and they'd quietly patched it and said nothing? What if they'd said something but nobody cared because hey, it's fixed now? What if they'd flat-out ignored it as the rails devs did (perhaps even citing that as their reason)?
No, security only reliably gets addressed when it hurts and hurts publicly.
The combination of hitting the exploit on perhaps the most visible site for the target audience and doing so in a way that didn't harm anything is impressive.
100% agree here. The Rails devs may have ignored his issue, but GitHub are the victims here. Why not email GitHub and inform them, on the provision they communicate the fix they just made?
Doing this and saying "P.S. GH sorry, I was bored" is not acceptable.
And yet, people have stopped using C for a lot of things because of the security implications...
Look, when Rails is pushed as an out-of-the-box magic web stack--and it is--, and the out-of-the-box config has these problems available--which seems to be the case--maybe there is something to be said for the framework needing fixing.
Just because I can write code that prevents various exploits does not mean I expect the rest of the world to--and I sure as hell don't do so while brandishing about how even beginners can use my tools.
5. You agree not to reproduce, duplicate, copy, sell, resell or exploit any portion of the Service, use of the Service, or access to the Service without the express written permission by GitHub.
You do something dumb like this, you should live with the consequences. In legal terms, if they don't enforce their TOS, it can be a legal issue later.
I love how he punts on the terms with "but lets get real".
Analogy: You break into a house and get caught. "Oh yeah, laws. But lets get real, I was pointing out they didn't lock their door."
More seriously, just because he violated the ToS doesn't mean that they have to be dicks.
From all appearances, GH is taking it quite seriously. Because it is a serious matter. Egor on the other hand wasn't taking it seriously.
Then, he reopened the bug to prove it was a bug, and they closed it again.
Then, he submitted a new bug, 1001 years in the future, and they closed it, saying "Good one ;)"
Then, he committed a text document to master, and they got all upset about it and got Github staff involved.
How about this Ruby devs: actually consider what you're saying. Isn't the point of Ruby to make things simple? Yet, somehow, making rational default choices only applies to everything but this. I know that you love to feel better than everyone else (see: http://37signals.com/svn/posts/3124-give-it-five-minutes), but seriously, you shouldn't get all upset about it later when it turns out you're wrong.
This is why I refuse to learn Ruby. I don't want to be associated with that community. "Learn Ruby if you want to be associated with a bunch of people who call themselves Rockstar Programmers."
Ruby is a really cool language with some fascinating features--don't limit yourself by assuming that you also have to learn the Rails stuff.
Seriously, give it a shot in one of its more palatable forms.
If you come from PHP or Java anyway.
no new genuinely new programming ideas have happened since the 70s and 80s.
Don't be obtuse.
You refuse to learn a language due to how a project acts? Have you ever read the {open,free,net}BSD or Linux mailing lists? You're not going to make it far in software development with an attitude like that.
Unless a quantum computing breakthrough alters the way we control computers, there will never be a one-language to rule them all. We'll continue having a wide diaspora and will take evolutionary steps to bind ideas together. Preemptive and comprehensive security model might be just one of those ideas.
A common thread here seems to be grouping Github and Rails.
Its interesting how bring up their Terms Of Service gets discounted and the holy war just takes over. No where did I mention Rails, Ruby, community, or anything else.
Me: He broke the rules. Retort: I like plant.
Disclaimer: The actions of individuals are representative of individuals. Drawing dramatic generalizations accomplishes nothing. Yes, there are more vocal asses in every community. For every asshat there is likely 100 quiet people. Right now, I'm being an asshat.
Gee, homakov is Russian. All Russians must want to hack your site. Really? No, its a dumb generalization.
Don't get me wrong, though. I can see being turned off by rockstars, but you've found a language unencumbered by them?
Example:
XSS is a common web security problem. In short, it means that putting user-originated data back on the page unescaped is unacceptable. Before Rails 3.0, the Rails approach to this problem was to provide a helper (h), which you could use to escape content that you knew to be vulnerable.
Unfortunately, many Rails users did not use this feature in all places they should have used it. As a result, many applications (Twitter included) suffered from whack-a-mole XSS vulnerabilities.
We were unable to solve this problem in the 2.x branch, because automatically escaping all text being put onto the page would be a massive breaking change and would break every app in existence.
Further, simply escaping all text would not really solve the problem. For example, the "<form>" tag generated by Rails itself should not be escaped, while any of its contents or attributes provided by the application should. Asking the user to take on the responsibility to mark Rails-generated content as not needing escaping would reintroduce the same problem we had before: people would "unescape" things too eagerly, and apps would tend to have vulnerabilities.
The solution was to release a plugin for Rails 2.3 users (rails_xss) and change the default in Rails 3. It isn't perfect: there are still cases where applications have to mark strings as "safe", and applications that have to do so often might have the same problem, but I think we did a good job, all things considered.
This case is quite similar. Rails provides all the tools necessary to have a secure application (attr_accessible is the equivalent of h), but many apps don't use it correctly (or at all). In short, a Rails security default seems to be wrong, insofar as "wrong" means that many people fail to use the security feature, causing their applications to be vulnerable.
As with the XSS feature, fixing this problem requires some thought. Simply changing the default would break a lot of applications in the wild. Like XSS, it's probably correct to do so anyway here. However, like XSS, we should make sure that we have done everything we possibly can to mitigate the additional cost associated with complying with the new default. If it's too painful, many people will overeagerly bypass the feature, reintroducing the very issue we were trying to protect them from.
In this case, I have proposed a solution that I think will mitigate a lot of the problem (https://gist.github.com/1974187), and we will likely ship it as a Rails 3.2 plugin. This will allow us to gather feedback about unexpected consequences and ensure that when we change the default for Rails 4 (which has not even shipped a prerelease yet), it actually mitigates the security problem in the vast majority of Rails applications.
Saying "we'll provide the tools and insist that developers use them safely and securely" is a bullshit answer, and always has been.
You're either secure by default, or not at all. In this case, not at all.
I hope this marks a turning point in the way the Rails team think about security.
ps At least Rails no longer allows web crawlers to delete resources by issuing GET requests.
I'd rather have the model and the ORM be pulled apart and the model make this distinction.
Or create another class that knows how to safely pull values out of a params list and use it to create a model.
But it's bad enough that the controllers "look" like they belong to a model in default Rails generators. This creates a certain amount of laxity in programmer's thinking. It boxes their thinking in instead of letting their thinking go free.
Thanks again for your hard work and being a public voice on the issue.
* Not actually DHH.
Models: find app/models -type f -name \*.rb | wc -l
Models with attr_accessible: grep -r -m1 "attr_accessible" app/models | wc -l
If those numbers aren't the same, and the missing model files inherit from ActiveRecord::Base, then look into adding attr_accessible.
https://github.com/instructure/canvas-lms/blob/9b52a51b6a37e...
I tried fixing this with introducing roles that have access to all attributes. Source at https://github.com/eval/sudo_attr_accessibility
You're completely misrepresenting what happened. Someone pointed out that Rails makes all Rails applications astoundingly insecure by default since forever, and got condescendingly dismissed several times by the people in charge.
He then proceeded to make a point by demonstrating the severity of the security flaw, and you made him look like some malicious hacker that got swiftly punished by the ever-vigilant GitHub team.
Public Key Security Vulnerability, really? You detected the attack and expunged the unauthorized key? What a load of bullshit.
You should consider revealing a history of all public key changes for each project (assuming you still retained apache logs) so that people can decide for themselves how much work they have ahead of them to re-audit their past commits.
Contrast this with the enormous hue and cry against FB, MS, et al. when they have comparatively minor holes in their systems.
I'm not saying that we need to tar and feather GH, but we should at least be equal-opportunity in our condemnations and realize that everybody is capable of mistakes. So, if you're OUTRAGED about Apple making a minor boo-boo, you should be equally outraged about this.
def destroy
@album = Album.find_by_id params[:id]
@album.destroy
[...]
end
user.albums.find_by_id params[:id]
When I wrote a journal article about it my recommendation was that Rails ship with
ActiveRecord::Base.attr_accessible(nil)
by default, because otherwise vulnerabilities of that nature were virtually inevitable.
You can't punt on security. Abstractions may make it easier, but you better be damn certain how they work.
You can't expect all of the developers in the world to be literate in security, especially the ones who choose batteries-included web frameworks as their go-to.
You can't blame the framework for people failing to validate input. The framework can provide you abstractions for helping you validate input, but at the end of the day, if you're being lazy or sloppy about what you do with data from the outside, that's your fault, not the framework's.
The way we do it is by not trusting the application. All authorization happens by levels further back, such as permissions granted to run stored procedures or on relations (permissions are granted to relations where there really is a clear, coterminous mapping between relational operations and procedural ones).
We then provide a framework for handling all of this. It means that as-yet-undiscovered flaws in our application are not as exploitable as they might be because the app is not trusted by the database.
Here's the file: https://gist.github.com/1975167, just add to lib/generators in your Rails 3 app, then do rails g mass_assignment_security -h
Hopefully others find this helpful
It should be obvious that you can't anticipate every potential attack vector at design time. Therefore, a well-designed system is one for which, when expected or normal conditions are not met, the resulting action is nothing or error, not an unexpected action.
This principle is also known as fail-safe: http://en.wikipedia.org/wiki/Fail-safe
What I want you to see in that thread I mentioned is the
way the core team perceives this. You are not discovering
anything unknown, we already know this stuff and we like
attr protection to work the way it is.
I guess that doesn't apply to security.
When you make it really easy to get started, a lot of people won't learn the system in depth enough to understand all the issues because they don't need to in order to make it work "well enough" for most cases.
By making Rails so easy to get started, they pretty much guarantee that there's going to be a ton of developers that don't pick up on, or forget, that they need to deal with issues like this.
That even a site like Github was vulnerable to this demonstrates just how seriously wrong it is to pick a default like this..
http://edgeguides.rubyonrails.org/security.html#mass-assignm...
Heck, I even learned this way back when I was learning Rails:
http://railscasts.com/episodes/26-hackers-love-mass-assignme...
That they like to consider it a "feature" doesn't make it any better - it just makes them look like idiots
The Security guide does describe the issue, and even describes attr_accessible as a "Countermeasure". http://guides.rubyonrails.org/security.html#mass-assignment
Without any precautions Model.new(params[:model]) allows attackers to set any database column’s value.
Unbelieveable! PHP showed long ago that allowing a web request to auto-populate arbitrary members in app objects is just a spectacularly bad idea.
Even the canonical "Hello Rails" example code neglects to specify attr_accessible. http://guides.rubyonrails.org/getting_started.html#hello-rai...
The general attitude of Rails developers towards security here is really shocking. I don't think I could recommend anyone use Rails.
Being able to change the :id or timestamps of the post isn't anywhere near the SQL injection vulnerabilities I've seen in many tutorials in other languages/frameworks.
I agree, though, I wouldn't recommend Rails to people who can't bother to read documentation.
The comments on the commit mention he just raised an issue that few people protect the attributes on their models from mass assignment, which… is one way this could happen.
Kind of a dick move, though. Responsible disclosure, doing it on a Sunday morning, etc, etc.
> Responsible disclosure
Looks like the guy did report it and the response was: "Not our problem" / "Not an issue." He got frustrated and decided to make a very public example of how this is bad.
Hence a plea to the framework to make this a default.
The approach this encourages is to code insecure (because you don't know any better), without any awareness of it being insecure, then remember to go back and secure it when you've found out what all the vulnerabilities are. Inevitably, you will waste time, and probably miss things out.
I'm getting a good laugh seeing this card played to defend terrible practice as the norm for what sells itself as an opinionated framework that champions convention over configuration.
I hope the asshole messenger doesn't obscure the importance of the message because people often push back harder against a message when it is delivered in an obnoxious manner by an obnoxious person.
Rails can certainly adopt an 'opinion' regarding this issue, but if I think if we were to take a look around at heavy web frameworks today, we would see a very similar approach of "let the developer decide" when dealing with Model security and serialization of fields.
These framework devs have no idea how people are going to use their models, so forcing them to whitelist everything by default may cause unnecessary headaches. Instead, they provide tools to prevent this exploit from happening should devs expose the model.
What everyone else is doing isn't a great justification--if decisions in Rails were based on what everyone else was doing, it would have been written in Java. In terms of Rails opinions, sensible defaults would be one that would suggest this should be rethought. In terms of rewrite-work, the Rails team didn't shy away from that with Rails 3, but I think the cross site scripting protection was worth the work. And even if the default is changed, nothing stops it from being a single line of code to turn it off.
Having the whole internet bashing him isn't good for anyone.
I'm for one very glad he kept pushing it out (but didn't do any real harm). This kind of vulnerability is just so common in Rails apps: I'd like to see safer defaults.
The vulnerability was demonstrated by adding a commit to the Rails project on GitHub, indicating that GitHub suffers from the vulnerability.
Here's the relevant issue. It might clarify things a bit better: https://github.com/rails/rails/issues/5228
It is a bug in that it's a usability issue; maybe it should be turned on by default, much like the auto escaping to prevent XSS that came in rails 3.
Is this really common practice in Rails code? Sure you can specify in the model that certain attributes can't be changed. But shouldn't this stuff be checked when validating form input? Normally I'd have a hash of filters, with the field/column name mapped to the appropriate set of rules for that field. Anything that's not specified in the filters doesn't go to the model's new method.
In this case, it's like the equivalent of PHP code where you santizie the data in $_POST and just send that whole variable to the database.
Choosing which fields are accepted shouldn't just be a model security issue, it's a form validation issue. This makes choosing whether an admin can change a field or not trivial as well. If a request is made as an admin, (maybe through a form that's only accessible by someone with an admin role) then you just apply the validation rules for an admin. Otherwise you apply the rules for a user.
That's how it's been since Rails 1. Which is cool. But it's error-prone for newbies, especially when Rails's model and controller generators make all attrs writeable by default, with nary a generated comment about how or why to lock things down. In a culture of convention over configuration, attrs should be locked down by default: "config.active_record.whitelist_attributes=true" for new apps, and throw a helpful message when I mass assign to a model that has no accessible attrs configured yet.
By default, if you have an new, create or update_attributes (and more, I imagine) call which changes various attributes based on a hash from parameters (eg params[:post], where you have params[:post][:title], params[:post][:body] etc) Rails allows mass assignment of every attribute on that model, since attr_accessible is not called.
There is a method you can call in the model called attr_accessible that restricts the columns that can be updated through mass assignment, while still allowing for manual assignment of other columns.
An example of this might be a post's user_id, which you would usually want to set to the current user while not allowing mass assignment. Without specifying attr_accessible it would mean that if a malicious user added params[:post][:user_id] to their POST/PUT, the Rails application would update the user_id as per the params value. If attr_accessible had been called, defining the columns that the developer wanted to be mass assigned (say post and title), it would mean that the user_id would not be mass assigned and Rails would log that this was the case.
attr_accessible therefore acts as a whitelist for columns that can be mass assigned. It just so happens that the Rails default is to have no whitelist and allow all columns to be mass assigned, despite the fact that the sensible option is to always have a call to attr_accessible in your models.
That said, there are certainly places in my apps where I don't want this to occur. And whitelisting is much better than blacklisting!
So it sounds like you aren't using mass assignment to change parent ids either. Most people don't. Most people use mass assignment to change the attributes of an object, not that objects association to a parent.
Meaning using mass assignment is very similar to SQL injection: you pass variables from user input directly to model without even verifying them. Duh?
Now regarding GitHub: yes there is a security hole and they fixed it.
However, hacking a site after finding its vulnerability is definitely illegal and hope there will be consequences. And he did not even report a problem to GitHub.
Can somebody explain why this is a Rails bug?
In fact, it's even better: the commit he pushed into Rails's master through the exploit would have automatically added him as a contributor even if he had not already been one.
I like it, this would be a great way to be snarky and semi-responsible at the same time.
No, I mean really, "malicious attack". I can't help but laugh, he committed 3 lines of text grand total, this is what you call malicious? Seriously, WTF.
The guy is a proper white-hat hacker, even if somewhat childish, Y U ban him.
"Today I can pull/commit/push in any repository on github. Jack pot."
When you have this many eyeballs looking at your code, the odds of a good-intentioned (however playful/immature) coder to discover a vulnerability is much greater than those of a real ill-intentioned hacker simply due to the sheer number of the former. The issue will quickly get fixed by the community, the kid will get the attention he wants, and life will go on.
I was replying to the parent, who attributed this to the power of "open source & eyeballs looking at your code" but this is not such an instance.
There's very popular idiom in Rails development of updating model data from a form POST/PUT in one line of code in a controller:
my_object.update_attributes(params[:object])
Because many users follow this approach, it made this hack widely exploitable. You can either assign parameters piecemeal in the controller or explicitly set the attr_accessible attributes in the model. There's nothing inherent in Rails that caused this vulnerability, rather it was programming practices by developers.
Many PHP apps used to rely on register_globals without proper input checking, and when those apps got hacked, it was clearly their their own fault. Just like GitHub is primarily responsible for today's exploit. But that didn't prevent people from calling PHP "insecure by default" for enabling register_globals in the first place.
"<s>Discount for girls</s>"> Why I do this? Since guys in rails issues ingored me
But I guess I agree.
The problem is that it was not really a GitHub issue, it's Rails having a grossly insecure default setting. According to one of the comments in the bug tracker a lot of other high-profile sites also have the same problem. And presumably new ones would keep popping up for as long as Rails is the new hotness.
So disclosing the problem to GitHub would not solve anything. They'd deploy the fix, but a lot of other sites remain vulnerable. That's probably the case even if GitHub were willing to take a PR hit and admit they'd been insecure for a long time in order to spur other Rails users to fix their code. After all, they hadn't fixed their code after the previous widely published security problems caused by the same underlying issue.
Clearly the Rails core team were not willing to consider any kind of changes to improve the situation. As such, you can argue that making as big a scene as possible is the best way to improve security globally. It's of course unfortunate for GitHub that he chose to use that site as the example due to the obvious reasons. And certainly the timing is about as unfriendly as possible from the point of view of a west coast person.
* it provides a how to for other individuals to repeat the attack, in a public forum.
* it was made against an innocent third-party.
* I doubt steps were taken to contact the third-party.
* it was made on a Sunday morning. making it difficult to scramble and get a fix out the door.
"Clearly the Rails core team were not willing to consider any kind of changes to improve the situation"
The ticket was opened three days ago. Are you advocating that if an issue isn't resolved in an open-source project, in under a week, the individual raising the issue should be able to publicly attack anyone using the project?
Also, I'll put this gem of a quote out there:
"not only github is vulnerable this way - I found a lots of rails apps that are waiting for my hack! Yeah, it is only start" (mwahahahaha).
I already agreed that GitHub were innocent bystanders and that the timing was unfortunate. But if getting publicity to the issue was the main point, it's also easy to see why GitHub was the perfect target. I also already explained why it could make perfect sense to demonstrate the vulnerability in a public manner rather than just disclose it to one of the many sites suffering from the problem. None of that is a sign of malice, it's at most bad judgement.
The ticket having been opened only three days ago would be a good point if it hadn't also been closed and declared to be working as intended with a pointer to a previously closed bug about the same issue.
I'm finding it pretty hard to stay mad at someone with this tattoo:
And the octocat tattoo... How can you not like a guy with an octocat tattoo!