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.
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.
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!
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.
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.
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.
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."
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.
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...
(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).
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."
I'd buy Github stock if I could.
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.
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
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.
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.
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.
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
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.
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.
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.
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 said, there are certainly places in my apps where I don't want this to occur. And whitelisting is much better than blacklisting!
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.
"<s>Discount for girls</s>"