Skip to content
This repository

Mass assignment vulnerability - how to force dev. define attr_accesible? #5228

Closed
homakov opened this Issue March 01, 2012 · 106 comments
Egor Homakov

Those who don't know methods attr_accesible / protected - check that article out http://enlightsolutions.com/articles/whats-new-in-edge-scoped-mass-assignment-in-rails-3-1

Let's view at typical situation - middle level rails developer builds website for customer, w/o any special protections in model(Yeah! they don't write it! I have asked few my friends - they dont!)
Next, people use this website but if any of them has an idea that developer didnt specify "attr_accesible" - hacker can just add an http field in params, e.g. we have pursue's name edition. POST request at pursues#update

id = 333 (target's pursues id)
pursue['name'] = 'my purses name'
pursue['user_id'] = 412(hacker id)

if code is scaffolded than likely we got Pursue.find(params[:id]).update_attributes(params[:pursue]) in the controller. And that is what I worry about.

After execution that POST we got hacker owning target's pursue!

I don't mean that it is Rails problem, of course not. But let's get it real(Getting Real ok) - most of developers are middle/junior level and most of them don't write important but not very neccessary things: tests, role checks etc including topic - attr_accesible

how to avoid injections ? What should Rails framework do to force people to keep their rails websites safe? Making attr_accesible necessary field in model? What do you think guys.

Piotr Sarnacki
Collaborator

I was thinking about generating something like this, unless you pass some kind of option to generator (sometimes you just want to skip attr_accessible if you know what you're doing):

# You should always whitelist your accessible attributes for security
attr_accessible nil
Piotr Sarnacki
Collaborator

@lest pointed me to #4062, there is a config setting that can do something like that config.active_record.whitelist_attributes. Although, based on that discussion I think that the consensus is that it should not be the default.

Piotr Sarnacki drogus closed this March 01, 2012
Piotr Sarnacki drogus reopened this March 01, 2012
Egor Homakov

so, here is what i came up to.
My main concern - is relations.
I hate the idea that some people could rewrite object references by themselves. Nobody should be able to set task[user_id]=hacker_id or sort of
Vulnerable references are the main evil because
1) it totally ruins database and leaves no way to fix it back but backups
2) it works clearly. Hacked user won't be able to figure out "Where is my blog post" or "How come this spam post is in my timeline written by me" (if newpost[user_id]=target_user_to_hack)

I suggest to have foreign keys and primary keys protected by default(what are keys can be recognized by ActiveRecord easily).

IMO Rails should teach developers to set up so important and vulnerable keys by hands. It is not a handicap for prototyping, just +a few lines of code. But it is all about sane and safe rails apps.

@task = Task.new
@task.user = current_user
@task.update_attributes(params[:task]) #having params[:task][:user_id] should raise error

#or projects-tasks relations
@project = Project.find(params.delete(:project_id)) #we have project_id but it is protected. lets delete to avoid exception
if @project.user == current_user
  @task.project = @project
else
  #alarm!
end
@task.attributes = params[:task]

I've got a big picture in my head but the backbone is described above. I strongly believe that having keys(references, relations) will make ruby on rails a 20% more awesome. I mean robust

Egor Homakov

Oh, didn't mention. On my part - I wouldn't support setting all attrs protected. It will cause such big mess! You cannot force every body to read release notes. Stackoverflow will be down. :] + it is dirty solution.
What also we have in model but keys? E.g.
title: string - not vulnerable at all, user is able to edit his comment
rating: integer - pretty vulnerable but not dangerous
account: integer - developer who left user[account] open for mass assignment should suffer. Nevermind about him.
what else? Do we really need protected fields by default? No.

Egor Homakov

@dhh you dont like 'whitelist' idea - i dont too. But what is your opinion regards relation keys? They are obviously should be defined manually - in most cases. That is why having them protected by default is not a big deal.
And, surely, it should be opt-out-able, e.g.
config.active_record.protect_keys = true

Egor Homakov homakov referenced this issue March 01, 2012
Closed

3012 #5239

Egor Homakov

Just imagine which power this vulnerability gives in my.. and now in your hands if you get what has happened.
@dhh @josevalim Y you no care :trollface:

Prem Sichanugrist
Collaborator

There're too much trolling going on in this ticket and I'm not sure anymore if you mean anything serious. If you're going to do a bug report or feature request, just stop trolling and try not to be annoying, then the core team will come and check out by themselves.

Name calling while trolling doesn't help, trust me.

Egor Homakov

@sikachu bugs are serious.

This is not only bug report, because this problem is so wide spreaded. postereous, speakerdeck, scribd, github - and I only have started testing.

We need to introduce blacklist attributes. MOst of rails apps(from small to github) likely got mass-assignment bugs if they don't user attr_accessible.

I just want to attract more attention to reviewing this problem from scratch and calmly decide - what should we do with M As-ment problem.

And, if some of you support my idea I could provide fixing pull request. But you ignore this bug, like nothing has happened for a long time. SOrry for not called for trolling, I overplayed :) Everyone, again, express your point please, you are welcome.

Xavier Noria fxn closed this March 02, 2012
Xavier Noria
Owner
fxn commented March 02, 2012

There was a proposal about changing that flag in #4062 and the consensus is the pros of the default configuration outweigh the pros of the alternative.

Thanks!

Egor Homakov

@fxn have you read my post? Did you see vulnerabilities i pointed out in e.g. github?
It is not about the flag in configuration, I'm pretty certain. We need to have :created_at, :updated_at, and references to be protected by default. Whitelist issue has nothing to do with my point
Please read issues carefully, because I clearly emphysized that flag is not a panacea

Xavier Noria
Owner
fxn commented March 02, 2012

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.

Egor Homakov

look. this is github, written on rails. https://github.com/homakov/ClientSit/issues/4
as you see github is vulnerable. Because gh devs are bad? Or who's in charge?
Rails are in charge, and blacklist of custom [:created_ate, :updated_at] is the least we MUST do

Really, is there any case when user should be able set created_at by himself? It is timestamp! Why I can set it using just <input name=model[created_at] value='1987...' ?
Again, let's talk about certain vulnerability-case. :created_at MUST be protected by default. Am i right?

Xavier Noria
Owner
fxn commented March 03, 2012

Rails is not in charge, it is your responsibility to secure your application. It is your responsibility to avoid XSS, to ensure that the user is editing a resource that belongs to him, etc.

Rails, however, does a lot of effort to assist you in securing your application as much as it can. That's why you have some protection measures built-in.

I don't think we need to special-case anything. The user has a flag to secure by default, I personally think that is enough. I see little benefit in special-casing the timestamps if you have the flag set to false. And I am totally -1 in doing anything fancy with foreign keys because they have of course valid use cases where users can set them.

Egor Homakov

Perfect response. Rails are not in charge - my question was like, 'why is that'. Surely nobody is fully in charge.

But only rails apps got this kind of bug. Yeah, phpists never got so fast prototyping tools so most of them cannot even dream about update_attributes {} and form generation too.

Rails has very convinient tools, I agree! attr_accessible is good and must thing for every developer. But for now we got it too unknown. Most of middle level developers forget about the meaning of this tool. Mixes with attr_accessors in ruby itself - so their model gets very unsecure.

So you only a little bit support special casing of timestamp.
For first view at the problem of timestamp-overwriting it doesn't seem fatal. But when I have issue/comment written in 3012 year I at least have it number 1 in any timeline orders. That seems ugly to me so I am keen to special case timestamps.

Regards foreign keys - I didn't get why you are "totally" -1, and if you got spare time please give me valid use cases if you know some. That will help me in understanding your point fully

another way to decide problem - populate using of accessible. Include it in scaffold generated output for model files. Im pretty sure rubyists will care much more having these lines and one day 4/5 will know+use this declaration. (now 4/5 don't either know or use, that's sad)

Piotr Sarnacki
Collaborator

@homakov I also don't like the idea of doing anything automated with foreign keys or timestamps. That way, people that does not know that they need to use some kind of params protection will be secure in those 2 cases, but it will fail with any other critical field, let's say admin.

@fxn what do you think about generating a model class with a comment describing attr_accessible and giving example, something like:

# If this model can be modified by publicly available resource using update_attributes
# method, you should protect its attributes with attr_accessible, for example:
# attr_accessible :title, :body
Egor Homakov

@drogus it is your viewpoint. Ok, let's not bother junior devs who will need to think about protection.
But I like your proposal regards model generator. I suggested it as an alternative, this fix will satisfy me. :)

Roland Moriz

It's 2012. When will people stop believing in "blacklists"?

The default has to be restrictive and the developer has to make sure she/he does acceptable exceptions. It's like with the XSS/sanitize stuff. It will never work the "blacklist"/"developer should care" way, imho.

At least throw a warning into the debug-log when a model has no attr_accessible defined at all...

Christian Höltje

@homakov Is this how you added b839657

Just curious.

@rmoriz +1

Rails is all about conventions. Broken by default is not a good convention.

Vlad Gorodetsky
bai commented March 04, 2012

@homakov Man, you look like a kid that wants some attention. Stop trolling and make GitHub a favor — file a private bug request via contact form — that's how non-douchebag people do.

I totally agree with @fxn — missing attr_accessible is not Rails' fault but developer's mistake. Generated models could have a friendly comment like @drogus said, but in practice I think we don't use generators that much, I thought that's easier just to type class Blah < ActiveRecord::Base.

Christian Höltje

@bai don't shoot the messenger, dude.

Roland Moriz

@bai what you say? Every book, every tutorial, every guide tells developers to use generators. Do you create all the test + migration files by hand?!

Vlad Gorodetsky
bai commented March 04, 2012

@docwhat I might have a bit overreacted — apologies for that. Yet I think giving one example of vulnerability is enough, no need to go through commits and issues over and over screwing things up.

Charlie Melbye

@bai is right, though. Zero-day attacks are a completely inappropriate way of raising awareness of a vulnerability.

Vlad Gorodetsky
bai commented March 04, 2012

@rmoriz I use generators on prototyping stage extensively, but then it simply gets too custom and often beyond scaffolds. It depends on a dev though.

Roland Moriz

@bai it's primarily a model issue, not a scaffolding issue. attr_accessor is defined inside a model, not in a controller. 100% of all developers I've worked since 2007 use generators to create models + migrations.

Matt Kydd
mtkd commented March 04, 2012

Two security issues concern me about Rails:

1) The defaults not being restrictive

2) Lack of any certification for Gem distribution (imagine if someone got a commit in to some of the most popular gems)

Roland Moriz

@mtkd rubygems does not even have a popular self-signing implementation as most cpan modules (perl) use. So you can't be sure the gem you've downloaded is the one the author made (not the CIA or another MITM).

http://search.cpan.org/~andk/CPAN-1.9800/lib/CPAN.pm#Cryptographically_signed_modules

(cpan 1.77 was released iirc around 2002/2003)

Vlad Gorodetsky
bai commented March 04, 2012

@rmoriz RubyGems supports gem signing, I use it for a client of mine: http://docs.rubygems.org/read/chapter/21
On the other hand, 99.999% of gems don't use it :-)

Roland Moriz

@bai iirc only when explicitly specified on the cli (what about bundler?) and as you said...."not very well" adopted.

Marco Chomut

In this thread: core rails devs get egg on their faces for ignoring a fundamental security concern, and then proceed to complain when the reporter 0days GitHub.

Vlad Gorodetsky
bai commented March 04, 2012

Rails devs are not responsible for Rails-powered sites. It's not their fault.

Yehuda Katz
Owner

See https://gist.github.com/1974187 for my proposal for how to improve the situation.

Jose Angel Cortinas

@bai I don't think that's completely correct, the fix presented in this issue is exactly what the Rails core devs could do, to assist the novice Rails devs. Convention over Configuration means that the Rails devs are actually partially responsible for Rails-powered sites because they establish the convention.

Ken Collins

@wycats Proposal for Improving Mass Assignment - https://gist.github.com/1974187

Bradly Feeley

@bai A framework's responsibility is to help programmers write better quality applications.

Vlad Gorodetsky
bai commented March 04, 2012

Not to mention that attr_accessible shouldn't be in model...

Sean Bamforth

It's a worrying issue, made all the more worrying in that I had no idea it existed. I know that the Rails Guys don't see this as a big deal, but I'd ask them to reconsider. This issue affects a lot of projects.

I've added "disable_mass_assignment" as per the following link and I'm updating my codebase. FWIW, it's a pretty quick job to search for affected fields and add them to the model.

http://railspikes.com/2008/9/22/is-your-rails-application-safe-from-mass-assignment

I'm not 100% sure what the ramifications are for models which have different rights according to a user level, but I'll be investigating.

One Quick Suggestion:
When Attr_Accessible is switched on, the error you get looks something like ...
"Can't mass-assign protected attributes: email, name"

Maybe this could be changed to :
"
Can't mass-assign protected attributes: email, name
Add following to your model (http://info_about_attr_accessible) to allow these to be edited in forms:
Attr_Accessible :email, :name
"

I believe the code to protect the fields should be in the model. I think model attributes should come with security baked in. Not 100% sure how this would work, but it makes the most sense to me.

Paul Dacus

From homakov: "@dhh @josevalim Y you no care"
Does this guy have to send a teddy bear & flowers?
If someone tells me "Hey you got a severe hole in your app", and I ignore him, and that hole is then exploited by Anyone (him included), I am to blame. Someone who cracks my app is an "a$$hole", but if they tell me first, and then only crack it to illustrate their point... no. I am wrong.

Xavier Noria
Owner
fxn commented March 04, 2012

@pauldacus his ticket has not been ignored, hasn't it. We disagree about his proposal, which is different.

Rails applications have a configuration flag that forces you to declare accessible attributes, the support to prevent these kind of issues is builtin.

ymakhno

Bravo to @homakov! What's the shame!

When 90% rails apps in the world are vulnerable, "core team" says "We disagree about his proposal". I have just one question is this framework for ruby developers or only for such "professionals" from "core team"? Open source is so open source in this project...

Piotr Sarnacki
Collaborator

@ymakhno this proposal would not automagically fix any existing app - it's up to developer to say which fields are protected, there is no easy way to guess that. This can be a problem in all frameworks that allow mass assignment.

That said, I'm sure that this needs some farther discussion, but automagically fixing some fields is not an answer and will just not work in practice.

Piotr Sarnacki
Collaborator

Also, for what it's worth, I proposed a temp solution to raise awareness by adding comments to generated models, to which @homakov responded "1 day ago":

But I like your proposal regards model generator. I suggested it as an alternative, this fix will satisfy me. :)

Peter Bright

Dear Rails people,

Have you learned nothing?

"Insecure-by-default" means "insecure". Trusting the programmer to fix things up and make them secure has never worked.

You guys have reinvented strcpy(). Way to go.

ymakhno

@drogus in my opinion, if you don't know how can you handle this automatically, force developer to choose correct option. With current defaults, developer sees "ok, everything works fine", and he won't read next chapter about security, he doesn't understand how dangerous can it be! Every wi-fi router after you entered predefined password says "it's dangeous to use predefined password", when you login to some admin console with predefined password it says: "You are using default password. Please change it.". You aren't warn that it's dangerous.

I believe the problem is the bug was closed after your comment and no new one is created. I think you would like to bury it under show, but thanksfully @homakov done the thing that won't be forgotten for a long time ;)

Kevin Postal

+1 Django

Piotr Sarnacki
Collaborator

@ymakhno seriously? please don't create any conspiracy theories. The first closed/open thing was done by accident, I reopened ticket seconds later.

Yehuda Katz
Owner

For what it's worth, I have commented about this ticket at http://news.ycombinator.com/item?id=3664334

Piotr Sarnacki
Collaborator

@ymakhno ok, sorry, I didn't notice that you're reffering to the other issue, not this one, frankly I missed the magical reopening. I passed it to github guys and closed as was not adding anything into discussion - @homakov proved that github is vulnerable and it's referenced here.

Daniel Fischer

@homakov wasn't trolling. It's a language barrier filled with relating to memes. His intentions were pure and good. Maybe didn't solve it the best way, but he got the attention and now a proper response.

@wycats solution is a great one.

Karl Baron

Nobody here sees the irony in Rails redoing what PHP was ridiculed for for so long?

Never.
inject.
user.
input.
by.
default.

Corin Langosch
gucki commented March 04, 2012

Imo this whole (mass) assignment protectecion does not belong to the model at all, it's a controller thing. I already created an issue for this months ago: #3157

Devrim Yasar

github owes @homakov an apology. a proper one. and the ones who tried to dismiss him because he doesn't write proper english, should do it in russian.

Pedro Machado Santa

Nice one @homakov Well pointed. GitHub jeez, unblock the man's account!

Peter Marklund
peter commented March 05, 2012

We should actually be grateful to @homakov for bringing this known but not sufficiently addressed problem to attention. Rails should of course be secure by default. Analogous to the XSS situationwe should not allow an insecure (tainted) params hash to be passed straight into our db API, thats almost as bad as being open to SQL injection.

The whitelisting/filtering belongs in the controller. Since both @dhh and @wycats seem to share this view there is a good chance we will get a proper fix in Rails soon.

Kevin Geurts

The entire idea of Rails is to make an application extremely hard to hack even when the programmer has little to none security expertise. Expecting people to fix this themselves is plain silly and goes against the rails way in my opinion.

IMO, the entire mass assignment idea should not exist - its way to dangerous and and has no room for error.

Steven Hancock

@kevincodux I thought the entire idea of rails was to make it easier to build applications, to focus on what your application actually does without having to worry so much about how it does it (at the lowest levels, at least). There's an entire guide explaining how to secure that application once you've built it (or even while you're building it). I agree that certain things (like mass-assignment security) should be enabled by default, but I wouldn't go as far as to say that's the "entire" idea of Rails.

Kevin Geurts

@stevenh512 True - but with one comes the other. Applications get easier to build when you do not have to invest so much time into securing your applications. Rails is also forcing people to think with a certain mindset (which makes you a better programmer in the end). Junior/mid-class developers see that something works with Rails and they think that's it.

When you force mass-assignment security - they realize their code is not working and must assign the attributes - you force them to think about security. Starting rails developers should not learn about these issues by having their entire project exploited IMO. Perhaps its not the entire idea of Rails - but Rails already got so much out of the box security build in its certainly a very big part of it.

Avery
Avery commented March 05, 2012

this is an interesting ticket, it's like "close the security hole" "no, we want developers to close it themselves" and then they continue calling it a philosophical disagreement after the bug tracker itself is hacked

Steven Hancock

@kevincodux Oh, I agree, I just wouldn't go as far as saying that's the entire idea of Rails.

Personally, I think the best solution would be to immediately release an update that takes one of the approaches given in the first two comments by @drogus on this ticket. Either set attr_accessible to nil on ActiveRecord::Base or set config.active_record.whitelist_attributes to true by default. Either of these forces safe and sane mass-assignment by "breaking" the app if your models aren't safe. I put "breaking" in quotes because, realistically, if this one change causes any kind of breakage in your app then it was already broken anyway.

Michel Pigassou

Hmm can someone summarize the discussion? Did anybody agree on something?

Damian Nowak

When I first saw Model.new params, I already knew it's vulnerable and nobody should write like that. However, this approach is widely used in all Rails guides, and so insecure. Mass-assignment should be disabled by default.

Bryan Drewery

:-1: to response from rails team.

Jason Rogers

@relgames Java-based frameworks are not the answer. They can be pwned too. Take any Jersey app with Spring integration and do the same thing. The only difference is you may have to work a little harder at guessing some of the attributes.

I think it's funny that junior/mid-level programmers are being called out so much in this thread. Surely the engineers at GH are not all junior/mid-level guys. Maybe they are and I am just naïve.

Gabor Garami

I do not think mass-assignment should be disabled by default, but must be disabled on production environment by default. I think this is a good compromissum between developer needs and production security.

About filtering what @homakov suggested, I think some similar. First, there is need a way to instruct the framework to filter it by default then it needed to be enabled in production mode. I understand every developer is responsible for its stuff but I do not agree why Rails does not help for make its application more secure.

Rails is designed with some security solutions, like CSRF stuff and basic protections against XSS. I do not think why we cannot do more step on that way, and make models secure by default. If it is planned well and configured well, then it should not be a pain in the ass. Who needs the original function, they can disable this feature.

Jason Rogers

@relgames my comment above is not fully accurate. I should have said, "Take any Jersey app with Spring integration where the services interact directly with the entities..." I have seen many Java programmers do it.

Mohamed Mansour

It was forced! 6dd6816

pera
pera commented March 05, 2012

Nobody here sees the irony in Rails redoing what PHP was ridiculed for for so long?

THIS

Elliott Mason
Eleo commented March 05, 2012

Protecting attributes from within controllers sounds like a setup to have multiple points of fault. If I have more than one controller making saves/updates on the same model (ignoring a debate over if that is good design), then I have to remember to protect attributes in multiple controllers. If I handle mass assignment from the model, there's a single, definitive barrier between submitted params and persistence, irrespective of where that data originated.

Egor Homakov

@Eleo exactly. That's what I tried to say dhh in my gist. Controller has nothing to do with unintended params. It doesnt filter. It handles, manages, controls business logic.
IMO You should write your controllers way described below:
1) have all critical fields protected/updateable - accessible
2) control saving new record in controller using role checks, dependencies and abilities. Not just by filtering :user_id
3) Never simplify business logic for "update". People got role checks in "create" but trying to manage just "update_attributes" in edit and that's why this has happened. If you check role - check it on create and on update.

Don't be lazy. sic!

Roozbeh K

@homakov I see your price-list updated on your site :D. With you are skills and rails knowledge, you are asking for a junior dev salary in us.

Jonathan Nevelson

@Eleo, what if you protect those attributes in ActionController::Base, which all other controller inherit from? This would allow you to still set all attributes in your test cases, and you would protect against having multiple points of failure.

Rodrigo Dominguez
rorra commented March 05, 2012

Probably you can make a branch like "rails_for_developers_that_doesnt_read_documentation" and in that branch you can take off update_attributes, and there you will have a "secure" rails framework for people complaining about this issue :)

egroeper

@kalleboo +1
Exactly what I thought. This is a new register_globals.
Unbelievable.

Elliott Mason
Eleo commented March 05, 2012

FWIW, this behavior isn't unique to Rails. Or, to be more specific, it isn't unique to ActiveRecord. If you were to look at other ORMs/ODMs, such as DataMapper, Sequel, MongoMapper, or Mongoid, you could see that mass assignment was allowed by default on any given model, and can be adjusted using a whitelist (or sometimes a blacklist) class method.

I don't see anything wrong with this per se. To disallow mass assignment by default would be a safer default, but it also makes the assumption on what you're doing in your application, and what does and doesn't need to be secured from user tampering. But it also requires you to be aware of the potential security concern to begin with and be thoughtful of it when designing your models.

Gabor Garami

@Eleo If somebody develops an application what will be available from internet for a wide publicity, then it must be protected on all way. I wouldn't like point to GitHub issue what happens if application is not protected. You cannot trust in the internet. If you are a beginner, then read the f_ing manual to yourself, and again, and again, until you understood: the security is not an option, but a requirement for your application.

I understand some ORMs does not enforce this security setting by default but this page is not an issue page for Mongoid, Sequel, or DataMapper, but it is an issue page for Rails, or to be more specific, for ActiveRecord.. If other ORMs decide make a possible security hole on a non-careful developer's code, then this is their own decision, not the rails developers' one.
Because somebody decides he jumping from the rock without a rope, we do not need to follow that example. We can attach ourself to a rope, and we currently doing it.

Gabor Garami

There was one problem with this option: it wasn't promoted enough. I never saw this option in the generated default config files, and if this (and GitHub) issue does not points me to it, I never set it, even if I use attr_accessible in my applications.

Promoting the security is a very high priority communication job, if there is one channel where we can do it, we need to do it.

Elliott Mason
Eleo commented March 05, 2012

@hron84 You can't protect the user from everything.

Having all attributes blacklisted is arguably a safer default, as I said. It nonetheless makes assumptions about the application. There might be no need for certain applications (which might not even necessarily be "web apps" per se) to have all attributes blacklisted by default. Some users might even find such a default more burdensome than helpful, depending on what their use case is.

The irony of your "read the f_ing manual" statement is that the lack thereof is part of the problem here. Users aren't protecting their attributes either out of ignorance or oversight. And to be fair, maybe the vulnerability isn't stressed enough in the documentation. (I haven't looked.) I'm not arguing against trying to mitigate potential disaster -- I'm all for it -- but I would also argue that if one doesn't truly understand why mass assignment is disabled by default, then he or she is liable to make their models insecure no matter what.

And for the users who are truly oblivious, here's what's going to happen anyway:

"I can't set user_id from this form. Let me just add user_id to the attr_accessible array, because that's what will make this action work the way I want it to."

That user doesn't understand what attr_accessible is for, why they'd want to whitelist attributes, why they need to whitelist attributes, and what can happen if they whitelist the wrong attributes. They might not even understand that they are whitelisting attributes -- they might think attr_accessible is some kind of riff on attr_accessor that adds getters and setters based on their schema.

At best, we're really just going to make it a little more difficult for them to screw themselves. Defaulting to a blacklist is an improvement to security, but no panacea. And I guess that's what I was getting at in my previous posts. It seems like some people are mindblown by all this, like allowing you to protect your attributes but not doing it by default is just nuts. We can go ahead and improve it, but there's still a dozen other gotchas awaiting the unprepared developer.

Michael van Rooijen

@Eleo

Having all attributes blacklisted is arguably a safer default, as I said. It nonetheless makes assumptions about the application.

To be fair, this sorcery also made assumptions for all our Rails apps (for security reasons): 9415935

And for the users who are truly oblivious, here's what's going to happen anyway:

"I can't set user_id from this form. Let me just add user_id to the attr_accessible array, because that's what will make this action work the way I want it to."

If you're going to do this (setting attr_accessible) without knowing what it does, and you can't be arsed to look it up because you're too lazy, then you deserve to get "hacked". At this point, the average developer would look up what this does unless they already know, and understand what it's for.

At best, we're really just going to make it a little more difficult for them to screw themselves. Defaulting to a blacklist is an improvement to security, but no panacea. And I guess that's what I was getting at in my previous posts. It seems like some people are mindblown by all this, like allowing you to protect your attributes but not doing it by default is just nuts. We can go ahead and improve it, but there's still a dozen other gotchas awaiting the unprepared developer.

The first thing I pop in to the model when I create one is the attr_accessible method, just to ensure I don't forget, because I likely will since I'm busy implementing other things. Even if you know it exists, like GitHub, apparently even skilled programmers get screwed by Rails' defaults. Thankfully @homakov isn't an asshole, but assume he was, he could've done A LOT of damage with automated scripts to GitHub. Also, if this unprepared developer runs in to a lot of gotchas because of having to whitelist stuff, that's good, he or she will learn from them, best way to learn is trial and error. But at least he/she (unless he/she is a total noob, and cba to read docs to clarify what certain things actually do when in doubt and actually uses it wrong) won't be screwed the way GitHub was. Obviously GitHub is at fault here though, not Rails. When I heard the news about the mass-assignment issue + GitHub I thought people were trolling, before I actually read about it.

I don't really care whether attr_accessible blacklists everything by default or not. But from my point of view, by not having it as the default, it has the potential to cause a lot of damage to even experienced developers. Perhaps it's possible to have it on by default, with a flag in config/application.rb to disable it if it isn't necessary, or the other
way around, since everyone does stuff in the config/application.rb anyway and will see the option and be like: "oh yeah, *flags it*".

Might just go ahead and do this to be safe:

ActiveRecord::Base.send(:attr_accessible, nil)

Just my two cents.

Adrien Lamothe

@homakov I like what you say about controllers handling, managing and controlling business logic. Code runs faster in controllers than in models. The TDD fanatics like to put all business logic in the models because it is easier to test, but they apparently don't care about how fast it runs (the client can pay more for that later.) But I disagree about not filtering data in controllers. When you are working with complex data relationships, it may be necessary to place some of the filtering logic in the controller; but that logic is the app developer's responsibility, no special rails constructs are required. People need to realize the examples, even in the books, are simplistic. But sure, why not blacklist all mass assignment and require the dev to use attr_accessible, nothing wrong with that.

jpoa
jpoa commented March 06, 2012

@homakov Well done :)

It's github's admins fault, the man did all he could. It's time people start listening when things like these come by.

Again, well done, fully applaud.

Ryan

Agreed. Sorry, but all this nonsense about blacklisting by default being annoying is ridiculous. If attr_accessible is the "smart" way to go, then devs should be forced (at the very least encouraged) to use it.

On the off chance that they're creating something that requires absolutely no security (seriously?!?), they can deactivate the blacklist for themselves.

wrzasa

Rails is about reasonable conventions over configuration. Reasonable being the key word. In this case @homakov proved your conventions are not reasonable. And you ignored him. Lesson learned: don't trust rails conventions, they are not reasonable, so if what you just coded works, now sit with the documentation, read it all and check all possible options to find out what do the rails dev's screwed and fix it; if you fail to do it and trust their conventions, they will tell you it's your fault. It was foolish to turn @homakov away. Just don't goon wit it.

BTW: I use the attr_accessible and attr_protected in my apps from the very beginning. Anyway I support what @homakov proposed.

Gabor Garami

@Eleo @meskyanichi was said all argument what I would like, so I would like to reply to just this:

You can't protect the user from everything.

This is absolute true. But, as a framework developer, I can protect my users from security problems caused by framework if it is possible. Developers are not a secretaries or nurses to I shouldn't assume they can't solve their problems but if I can help them to write better and more secure application then I must do it.

Faraz Yashar
fny commented March 06, 2012

Take a breath, and think naturally...

In life, people consciously and loudly choose to tote themselves in the nude. Even simple fashion is considered a statement:

attr_accesible birthday_suit # read: wildcard/regexp
attr_accesible keg, midriff, sixpack, love_handles
attr_protected sun_dont_shine, knockers, lingere, knicker_bockers
attr_protected bunny_suit # read: put the hazmat on the bunny

This is an issue of avoiding the dreaded nipslip or more poignantly protecting baby developers from rapists and pedophiles. Seriously, how much can you expect from a five-month-old?

Consider another analogy between escaped strings (which Rails quite aptly handles) and consumables. Cereal and arsenic shouldn't mix; medicines are proscribed by experts; and the LSD's for the adventurous. In the end, you choose, but you can't just allow anyone to shove anything into any orifice.

Sensible conventions reflect how we think, and thus make languages like Ruby and fameworks like Rails so wonderfully appealing and empowering. Flexibility shouldn't be compromised, of course: I'd be pissed if I had to fight to dress myself. But keep the infant in his diapers and sew the long-forgotten crotch holes in your buddy's pants (coughgithubcough.) After all, mistakes are only human. ;D

azinazadi

i think instead of fighting, rails people should find a nice solution for this problem. please

Gabor Garami

Solution is in the repo, forcing security is enabled by default.

Chris

Couldn't rails automatically use the input fields declared within the form_for block to determine what fields are allowed to be posted? This could be implemented by automatically inserting a special key at the end of the form, then the controller could only allow the declared parameters through to the actions.

Chris

Would it be possible to encode the ids of every element in the form? Using sha1 or something like that. The form could also contain something that would assist in the decoding of the ids. The helper methods (form_for , text_field) would handle the encoding and the controller would automatically decode the params, so it would be transparent to the user.

Gabor Garami

@evilgeenius No, it is not possible, because the name attribute what is really matters. And it is needed for compute params array. And from SHA1 you cannot decode anything, because you do not needed to name your form elements as same as attribute names - it is just a help for you. But, the form-decoding logic is cannot guess what do you want to get in your form, and if the choosen encoding is decodeable, then hacker can decode it relative easily - so it is not a protection, but security through obscurity - not needed, unusable, and does not matter.

And form_for is just generates a HTML form. It is not related to handling incoming parameters. And,you cannot restrict incoming parameters on the level of framework, because it is an application-dependent thing.

wrzasa
Norv
norv commented March 07, 2012

Can someone please point out to me a reasonable use case, for a web application, when you don't need to make sure your POSTed data is exactly what you're expecting?

I seem to see people think there are cases where not being forced to be explicit about what you allow, would be... needed, apparently. I'd appreciate a case.

wrzasa
Elliott Mason
Eleo commented March 07, 2012

@norv In the real world, there aren't a lot. One exception is where the data is coming from a "trusted" user, e.g. an admin. Sure, maybe there's fields you want to secure even from an admin, or maybe not, or maybe you just figure it doesn't matter if they want to hack their own app, if they want to go to those lengths.

However, not every model deals with POSTed data, and some do but only indirectly.

For example, a project I work on imports XML data from a "trusted" source, so in these kinds of models, there's no need for security. There are numerous models in the project like this. I'd essentially have to attr_accessible every last column, if the default were for all attributes to be blacklisted by default. This is somewhat arduous, and is somewhat of a maintainability headache, because if the schema changes later on, I have to add those new fields to the attr_accessible list.

If we're going to go with a blacklisted-by-default approach, is there some way for me to auto-whitelist every attribute without having to be so explicit as to type out each one?

Argel Arias

Probably a simple configuration should be enough blacklisted-by-default , a config to change it to whitelisted-by-default and the option to change it directly in the model at run time.

I don't see the problem really and rails has just maked a fool of itself...

PD: If you advertise you framework as "even an idiot can make a page in 30 minutes" you will end up with a lot of idiots writing pages in 30 minutes...

Rangel Reale

To me it seems obvious from the start that mass assignment should be disabled by default.

Otherwise, you add a field on a table, and you have to remember to block it on the model? If the field would be user-assignable, then of course a form would have to be modified to add an input, and at THIS time, you would modify the model, because it is related to the same change.

Yii framework in PHP which is very similar to Rails, blocks alls fields by default since the beginning.

If this is common practice in the Rails community, I would never trust a Rails application. This is as bad as SQL injection.

Adrien Lamothe

Just received an email from GitHub, perhaps related to this issue:

"A security vulnerability was recently discovered that made it possible for an attacker to add new SSH keys to arbitrary GitHub user accounts. This would have provided an attacker with clone/pull access to repositories with read permissions, and clone/pull/push access to repositories with write permissions. As of 5:53 PM UTC on Sunday, March 4th the vulnerability no longer exists."

Norv
norv commented March 08, 2012

Thank you!

@Eleo I was going to point out the commit above - cjcsuhta already mentioned it.
Reversing the switch is possible, for one's particular application.

The possibility of different sources for the data getting to the model, than http form input, and needing different treatment for them, tells me again that: authorization, filtering of the incoming data, and even validation needed for the data, are not really model issues. They may apply on the model, or be aware of it, but they cannot and should not be handled all at model level, otherwise you can't really have flexibility nor security as needed.
The handlers of those actions (i.e. controllers, forms, actions classes) should be responsible, directly or indirectly.

On the other hand, since in the case of web form data, there is an always-present need for request filtering, authorization, and validation, I find it more than reasonable to address these concerns at the level of a web framework.
If it's at the expense of convenience, then at the expense of convenience.
Security >> convenience.

At most, validation, from this list, may be disputable as to where it belongs, it may be the choice of a particular framework to handle it more tied to the model itself. I just don't see how could the rest be model matters, and in the same time have a both flexible and secure framework for development.
(Nevertheless, the commit pointed out above is IMO a step in the right direction, at least it makes sure the defaults will be saner).

Side note: I'd say admin is not really an exception in the sense of my question, since checking if the current user is an admin means an authorization check.

David Burry

We developers using Rails should not have to write Merb or 0day the repo in order to get core Rails devs to stop saying "The emperor DOES SO have clothes!" Core devs, please change your attitude. We want newly-generated rails apps to have a setting in the application.rb that by-default turns off mass assignment, unless we reverse that setting in the application.rb or on a model by model basis. This way it's backwards compatible with existing apps too. Most of us want this. Stop making us resort to such tactics for anyone on the core team to listen.

Gabor Garami

@dburry you knocking on the open door. Please read back this issue, the future Rails versions will come with enabled enforcement.

David Burry

@hron84 indeed, for this issue... but will our attitudes change enough from this experience so that such tactics won't be necessary with other issues? I see the attitudes as the root cause that made this issue go so long ignored. And I realize this is more a social thing than a technical thing, so maybe this isn't the right forum for me to keep talking about it, sorry :P

sampablokuper

Isn't the question of whether mass assignment is enabled or disabled by default a bit of a red herring? I've just posted a question to this effect at SO, if anyone's interested.

Egor Homakov

@sampablokuper Answer is No. User is able to update HIS public key record AND he updates it with new user_id. code:

pk=current_user.public_keys.find(params[:id])
pk.update_attributes! params[:public_key]
Federico Nusymowicz fedenusy referenced this issue from a commit February 14, 2014
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.