IOS 6 injecting should_group_accessibility_children to POST requests(logicalfriday.com)
logicalfriday.com
IOS 6 injecting should_group_accessibility_children to POST requests
http://logicalfriday.com/2012/10/03/ios-6-injecting-should_group_accessibility_children-to-post-requests/
63 comments
Begin by reading up on Rails.
Mass assignment is a fairly normal thing to do, any parameters you want the model to ignore can be marked as so in the model and then mass assignment will work anywhere. Rails loves DRY.
Since the only way in which one can get wrong arguments is if somebody is hacking you (or attempting to do so) in which case just throwing an error at them is a fair thing to do.
Mass assignment is a fairly normal thing to do, any parameters you want the model to ignore can be marked as so in the model and then mass assignment will work anywhere. Rails loves DRY.
Since the only way in which one can get wrong arguments is if somebody is hacking you (or attempting to do so) in which case just throwing an error at them is a fair thing to do.
Blanket accepting POST data to insert into your database is a terrible idea.
Rails gives you attr_accessible and attr_protected to alleviate this problem. Protip: the one from those two that you suggested to use is just as terrible an idea as allowing blind POST into your database. Blacklisting is always the wrong approach to security. I don't care if you have to repeat yourself and type more characters: use the whitelisting of attr_accessible.
Rails gives you attr_accessible and attr_protected to alleviate this problem. Protip: the one from those two that you suggested to use is just as terrible an idea as allowing blind POST into your database. Blacklisting is always the wrong approach to security. I don't care if you have to repeat yourself and type more characters: use the whitelisting of attr_accessible.
But even if they're using attr_accessible to whitelist parameters, the failure mode here would have been exactly the same.
The real culprit is Apple. It's just not ok to add unexpected parameters to people's POSTs. If anything, they should have put it in an HTTP header instead.
The real culprit is Apple. It's just not ok to add unexpected parameters to people's POSTs. If anything, they should have put it in an HTTP header instead.
I'm a Python/Django guy so I'm not intimately familiar, but is that really the case? I was under the assumption that the handler wouldn't pass any POST vars not in attr_accessible to the model. In which case additional, unexpected ones would become silently dropped.
According to http://guides.rubyonrails.org/security.html#countermeasures you're entirely correct that it won't raise any error, although the keys won't be dropped: attr_accessible drops the attributes when mass-assigned to a model, they're still available in the params hash.
> Mass assignment is a fairly normal thing to do
No, it's an abnormal thing to do and a security hole.
> any parameters you want the model to ignore can be marked as so
Which is complete insanity.
> Rails loves DRY.
Yes so does PHP, hence register_globals.
That's on the "idiotic" side of "I don't want to write stuff", and even the Rails staff stopped defending this insanity back in March.
No, it's an abnormal thing to do and a security hole.
> any parameters you want the model to ignore can be marked as so
Which is complete insanity.
> Rails loves DRY.
Yes so does PHP, hence register_globals.
That's on the "idiotic" side of "I don't want to write stuff", and even the Rails staff stopped defending this insanity back in March.
If you want to argue against facts, please go elsewhere. Mass assignment is widely used in Rails.
Just because something is widely used does not mean it is not a security hole. Mass assignment is a security hole, plain and simple- in Rails and anywhere else.
> Mass assignment is widely used in Rails.
And register_globals was widely used in PHP, that does not make it anything but moronic.
And register_globals was widely used in PHP, that does not make it anything but moronic.
I don't think register_globals has been widely used for at least 10 or 12 years. It's been deprecated for a while and was removed completely in 5.4.0.
Hence "was". It was widely used at one point, if only because it was enabled by default until 2002 (disabled by default was introduced in PHP 4.2)
I'm not a Rails guy, so you'll have to forgive ignorance here. Are you telling me that Rails only lets you blacklist parameters, not whitelist?
Because that seems entirely the wrong way around.
EDIT: looked at the link posted elsewhere here[1] and ound that it is possible to whitelist using "attr_accessible". Please tell me people know about and use this.
[1] http://guides.rubyonrails.org/security.html#mass-assignment
Because that seems entirely the wrong way around.
EDIT: looked at the link posted elsewhere here[1] and ound that it is possible to whitelist using "attr_accessible". Please tell me people know about and use this.
[1] http://guides.rubyonrails.org/security.html#mass-assignment
You can do either. The default used to be blacklist, but that was changed to whitelist recently.
You can do either. Whitelist is obviously preferred.
It allows either.
Sure, throw an error, but not a 5xx error. ISE 500 indicates that something went wrong on the server, not on the client. 400 Bad Request maybe?
I, for one, don't like having my application error logs polluted by script kiddies running against my wall...
I, for one, don't like having my application error logs polluted by script kiddies running against my wall...
Isn't this default Rails behavior? (despite sounding like something people would make fun of PHP for doing)
It used to be, and was recently changed after they realized that even PHP had disabled `register_globals` years ago (also, after they realized even Github, one of the biggest Rails site out there, had left this hole wide open and a guy used it to demonstrate the issues on the rails repo/account, which was very funny drama: https://github.com/rails/rails/issues/5239 http://erratasec.blogspot.be/2012/03/rubygithub-hack-transla...)
This (mass-assignment) is fairly standard Rails-fu, so its fair to assume that this could affect a lot of apps out there.
Correct me if I'm wrong, but isn't it pretty much always a bad idea to do it blindly, and the defaults are considered a security problem by the Rails project itself?
http://guides.rubyonrails.org/security.html#mass-assignment
http://guides.rubyonrails.org/security.html#mass-assignment
Hey now, don't go bringing facts into this thread -- I've already been downvoted hard for, apparently, not knowing what I'm talking about when saying that this is a security issue. So obviously the Rails team don't know what they're talking about either.
He just linked to an article that explains how to do it safely.
update to reply because of downvotes:
1) butterfly knives are very useful tools
2) mass assignment can be used safely out of the box in rails post v3.2.3. To use it, you have to explicitly add parameters to the whitelist or disable the whitelist. The article is there to explain why disabling the whitelist is a bad idea.
update to reply because of downvotes:
1) butterfly knives are very useful tools
2) mass assignment can be used safely out of the box in rails post v3.2.3. To use it, you have to explicitly add parameters to the whitelist or disable the whitelist. The article is there to explain why disabling the whitelist is a bad idea.
Sure, just like butterfly knives are safe because you can find some safety tips online.
Edit to reply to edits: Mass assignment is still dangerous "out of the box" since you have to switch on the whitelist behavior by calling attr_accessible on your model classes. In the security guide, the older, more dangerous, attr_protected is introduced first.
I think every rails dev should be familiar with the security guide, but more than that I wish that security was the default. While anybody is free to make an app as insecure as they wish, it should be the exception rather than the default.
Edit to reply to edits: Mass assignment is still dangerous "out of the box" since you have to switch on the whitelist behavior by calling attr_accessible on your model classes. In the security guide, the older, more dangerous, attr_protected is introduced first.
I think every rails dev should be familiar with the security guide, but more than that I wish that security was the default. While anybody is free to make an app as insecure as they wish, it should be the exception rather than the default.
You're talking about the old behaviour. New rails apps have config.active_record.whitelist_attributes set. That means that models without attr_acessible statements will throw an error if mass-assignment is attempted. IOW, they've done exactly what you asked for. They should have done it years ago, but they did the right thing after it blew up in their face.
You say "standard Rails-fu", I say security hole. Let's call the whole thing off?
No because you don't know what you are talking about.
I see. So that whole big mass-assignment security issue that exposed GitHub a while back -- that just didn't happen? Writing code in this style is perfectly safe?
Writing code in this style is perfectly safe if you do it correctly. GitHub didn't, so the defaults were changed to make it harder to do it incorrectly.
UPDATE: editing to reply since hn won't let me reply directly because the thread is to deep, yet I'm getting downvotes
It's not a tautology. Some things are safe even if you do them wrong. Some things are unsafe no matter how you do them.
Rails changed the defaults so that now you have to deliberately decide to do things unsafely. Rails before 3.2.3 fails un-safe in this scenario, but later versions fail safe. Rails 4 uses a different solution that's even harder to screw up.
UPDATE: editing to reply since hn won't let me reply directly because the thread is to deep, yet I'm getting downvotes
It's not a tautology. Some things are safe even if you do them wrong. Some things are unsafe no matter how you do them.
Rails changed the defaults so that now you have to deliberately decide to do things unsafely. Rails before 3.2.3 fails un-safe in this scenario, but later versions fail safe. Rails 4 uses a different solution that's even harder to screw up.
"Writing code in this style is perfectly safe if you do it correctly."
That's a tautology.
In general you can't count on code being written "correctly", so this isn't a defense. It is better to have systems that degrade gracefully in the face of humans and their idiosyncrasies, rather than those that fail-unsafe, because you can't build your security system on the assumption that your code will be written by superhumans.
That's a tautology.
In general you can't count on code being written "correctly", so this isn't a defense. It is better to have systems that degrade gracefully in the face of humans and their idiosyncrasies, rather than those that fail-unsafe, because you can't build your security system on the assumption that your code will be written by superhumans.
Should all database use be frowned upon because injections are possible if you don't use them correctly?
I hope you realize that this is the identical argument PHP developers made whenever someone brought up how insecure the base language, libraries, and configuration were.
Users of a framework should have to go out of their way to make themselves insecure. It shouldn't be insecure by default.
Users of a framework should have to go out of their way to make themselves insecure. It shouldn't be insecure by default.
This isn't a Rails issue, it can affect any web framework. And it's a security flaw every single time mass assignment is used without whitelisting.
like you are holding iphone4 wrong? If it's an app doing RESTful API calls via HTTP POST then it should have 100% control on HTTP parameters.
Watch next time Apple alter some http headers and break your API.
Watch next time Apple alter some http headers and break your API.
Although I agree with you that Apple should not alter HTTP POST parameters in-flight, the server side application must expect the client to be untrusted. Anything can connect to the server socket, not just its RESTful app on iOS.
What does this have to do with client trust? They're expecting the client to access their API a certain way, and the client is sending what is arguably a malformed request. This is, surprise, causing an error to occur.
Hard to tell from the post, but it sounds like Rails gave an Exception because it couldn't find should_group_accessibility_children, but that suggests that it would act on something it could find.
If there was a parameter that gave a user admin access, then Rails might accept such a parameter and that might be used to take control of the app.
It's similar to what happened to Github.
https://github.com/rails/rails/issues/5228
I would think that the API would validate the POST parameters, ignore unexpected parameters and give errors for malformed expected ones. Taking it a bit further the developer should then be notified of malformed POST parameters being present and decide if it is a bug or an attack.
If there was a parameter that gave a user admin access, then Rails might accept such a parameter and that might be used to take control of the app.
It's similar to what happened to Github.
https://github.com/rails/rails/issues/5228
I would think that the API would validate the POST parameters, ignore unexpected parameters and give errors for malformed expected ones. Taking it a bit further the developer should then be notified of malformed POST parameters being present and decide if it is a bug or an attack.
You never ever ever ever blindly trust the client to behave a certain way.
They are not. They give an error. The only issue is that a middle man is fucking with them.
Except they "give an error" because the provided field doesn't exist in the database. Ignore for a second that half the reposts would break websites if an unexpected parameter yield an error instead of being ignored, if an untrusted client sent an "id" field, that would go through like hot steel through melted butter.
Well. The likeliest thing is that there is no "middle man fucking with them". The likeliest thing, since it's an iOS app posting to their API, is that they're introspecting a client-side object to get the values they care about. And that they're blacklisting values they know they don't want, rather than whitelisting values they know they do want.
Which meant that when a new property showed up their app blindly submitted it to their web API, and their web API blindly accepted it because it was doing mass assignment, and that's when the API broke.
Which really just hammers home the point people have been trying to get you to see, which is that these types of idioms -- mass assignment, blind trust of client-supplied data, blacklisting instead of whitelisting -- are really serious problems that should not be encouraged, and should not be swept under the rug.
Which meant that when a new property showed up their app blindly submitted it to their web API, and their web API blindly accepted it because it was doing mass assignment, and that's when the API broke.
Which really just hammers home the point people have been trying to get you to see, which is that these types of idioms -- mass assignment, blind trust of client-supplied data, blacklisting instead of whitelisting -- are really serious problems that should not be encouraged, and should not be swept under the rug.
Apple isn't injecting this into the request. It's an object property and the application developers are doing something in their app that is iterating over and Objective-C object and creating a POST request from it. I guarantee it.
Form fields by way of application/x-www-form-urlencoded generally shouldn't be trusted wholesale. In the Wild, Wild Internet, you'll see things like 3rd party JavaScript add hidden fields, etc.
Instead of modifying at the middleware or persistence layers, copy relevant fields out of the POST hash into an intermediate hash. Which, generally, will be more secure anyways.
Instead of modifying at the middleware or persistence layers, copy relevant fields out of the POST hash into an intermediate hash. Which, generally, will be more secure anyways.
If your API breaks because an extra unrecognized parameter is included, Apple didn't break your API, you did.
Can you elaborate on why you think invalid API parameters should be silently ignored? I can see arguments for both sides, but none of them would make the API "broken" in my mind.
"The programmer should always have complete control", if the API is broke, the programmer has lost control.
From a security and usability perspective, the programmer must always be in ultimate control of what code is being executed. If it's "parameters", an API should be explicitly requesting any or all parameters, and completely ignoring unknowns. If the API is scanning for unknown parameters or even worse scanning for parameters to invoke variable named function calls (it happens a lot), at best; your API is wide open for attack. (This was a major attack vector in some Wordpress APIs in the recent past.) In the meantime, the unknown parameter just causes the program to break, and why would any programmer allow that if it's so easily preventable.
From a security and usability perspective, the programmer must always be in ultimate control of what code is being executed. If it's "parameters", an API should be explicitly requesting any or all parameters, and completely ignoring unknowns. If the API is scanning for unknown parameters or even worse scanning for parameters to invoke variable named function calls (it happens a lot), at best; your API is wide open for attack. (This was a major attack vector in some Wordpress APIs in the recent past.) In the meantime, the unknown parameter just causes the program to break, and why would any programmer allow that if it's so easily preventable.
Most likely, their native client is blindly adding the objective c properties to the POST request, by using introspection. Apple is not adding anything to the POST request. This is similar to the problems people used to run into when the javascript object prototype was overridden and everyone's code would break because they were just doing
for (var prop in object)
for (var prop in object)
Indeed. Why in the world would Apple inject this into POST requests?
http://developer.apple.com/library/ios/documentation/uikit/r...
http://developer.apple.com/library/ios/documentation/uikit/r...
Willing to bet it's a bug.
I'm sure this is what's happening. Their serializer is perhaps filtering out properties they knew about, but shouldGroupAccessibilityChildren is a new property on the UIAccessibility Protocol which is adopted by NSObject.
A good short-term fix is to update the web service, but they should update it to only use the POST parameters it expects. When they release an update to the iOS app, they can fix their serializer to only serialize the properties they need.
Apple isn't injecting this - their code is.
A good short-term fix is to update the web service, but they should update it to only use the POST parameters it expects. When they release an update to the iOS app, they can fix their serializer to only serialize the properties they need.
Apple isn't injecting this - their code is.
This should not be an issue if you avoid mass-assignment with dumping all params, which is a security vulnerability anyway.
If you do something like:
@user = User.new(params[:user].slice(:name))
It will be safer, and avoid the problem.
If you do something like:
@user = User.new(params[:user].slice(:name))
It will be safer, and avoid the problem.
Does this parameter serve a documented purpose?
It's weird that iOS is doing that... but (at the risk of piling on) browsers do all sorts of weird stuff. Your app needs to deal with it.
It's weird that iOS is doing that... but (at the risk of piling on) browsers do all sorts of weird stuff. Your app needs to deal with it.
Yeah, the discussion here seems to be missing the forest for the trees.
For non-Rails programmers an idea such as mass assignment sounds very strange, but if you ignore that, it still sounds odd at first request than an OS would inject parameters into a HTTP post request.
For non-Rails programmers an idea such as mass assignment sounds very strange, but if you ignore that, it still sounds odd at first request than an OS would inject parameters into a HTTP post request.
These folks are claiming that this is an issue without creating a minimal program that reproduces the issue. If it were a real issue it would be trivial to make an app that POSTs to an API and inspect the traffic.
I have inspected several POST requests from iOS 6 apps and have not seen anything like this yet.
I have inspected several POST requests from iOS 6 apps and have not seen anything like this yet.
This has been solved already and will be part of Rails 4. Use strong parameters: https://github.com/rails/strong_parameters
Will this be a default in Rails 4? I feel like people should need to go through a lot of work to do something as dangerous as Rails/AR mass assignment...
> Will this be a default in Rails 4?
Yes.
Yes.
I'm amazed at how the discussion here has devolved into the merits of mass assignment. While that is a worthwhile topic in itself, specially given the recent github issues, any suggestion that the this issue would have been mitigated by the OP not using mass assignment is flawed.
Any webapp that I would consider secure MUST validate all input from clients. This includes white listing any and all parameters names, preferably in middleware, but at least in the controller. Allowing random keys in your input seems recipe for disaster, when you consider a multi layer app security policy. While this may seem like an overkill to some, this is best practice I've seen implemented in any project that deals with real money.
Hence, such a change in iOS WILL break any such application. Irrespective of your views on the sanity of mass assignment.
I would like to see a discussion on why apple thought it would be a good idea to introduce a new parameter to every request. Any ideas?
Any webapp that I would consider secure MUST validate all input from clients. This includes white listing any and all parameters names, preferably in middleware, but at least in the controller. Allowing random keys in your input seems recipe for disaster, when you consider a multi layer app security policy. While this may seem like an overkill to some, this is best practice I've seen implemented in any project that deals with real money.
Hence, such a change in iOS WILL break any such application. Irrespective of your views on the sanity of mass assignment.
I would like to see a discussion on why apple thought it would be a good idea to introduce a new parameter to every request. Any ideas?
"Validating input" can yield a number of results, and ignoring incorrect data (keys) altogether is not only a valid reaction, it's a perfectly safe one and it's how the vast majority of systems work. Indeed, a number of usual patterns would break if that didn't work.
Mass-assignment and similar patterns (of shoving all request parameters into your trusted content, see also `extract` and `register_globals`) "allow random keys into your input", ignoring said keys doesn't.
Mass-assignment and similar patterns (of shoving all request parameters into your trusted content, see also `extract` and `register_globals`) "allow random keys into your input", ignoring said keys doesn't.
1. They were blindly throwing the entire set of POST data into a Rails model.
2. This started throwing errors when they got an unexpected key in POST.
3. They "solved" the problem by writing a middleware to filter out that specific key from POST.
I don't even know where to begin.