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/

by philnash • 14 years ago

23 63 14 years ago

logicalfriday.com

ubernostrum5• 14 years ago

So... the story here is

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.

est• 14 years ago

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.

mpyne• 14 years ago

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.

BrandonSmith• 14 years ago

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.

headShrinker2• 14 years ago

If your API breaks because an extra unrecognized parameter is included, Apple didn't break your API, you did.

jcromartie6• 14 years ago

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.

jmcnevin2• 14 years ago

This (mass-assignment) is fairly standard Rails-fu, so its fair to assume that this could affect a lot of apps out there.

ubernostrum5• 14 years ago

You say "standard Rails-fu", I say security hole. Let's call the whole thing off?

jcromartie6• 14 years ago

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

kalleboo• 14 years ago

Isn't this default Rails behavior? (despite sounding like something people would make fun of PHP for doing)

masklinn9• 14 years ago

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...)

tomjen36• 14 years ago

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.

untog4• 14 years ago

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

Pewpewarrows3• 14 years ago

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.

piquadrat• 14 years ago

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...

masklinn9• 14 years ago

> 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.

ejs• 14 years ago

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.

eli2• 14 years ago

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.

brown9-2• 14 years ago

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.

tilsammans• 14 years ago

This has been solved already and will be part of Rails 4. Use strong parameters: https://github.com/rails/strong_parameters

jcromartie6• 14 years ago

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...

Legion• 14 years ago

> Will this be a default in Rails 4?

Yes.

mobwill• 14 years ago

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)

jcromartie6• 14 years ago

Indeed. Why in the world would Apple inject this into POST requests?

http://developer.apple.com/library/ios/documentation/uikit/r...

coob• 14 years ago

Willing to bet it's a bug.

lancefisher• 14 years ago

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.

asg• 14 years ago

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?

masklinn9• 14 years ago

"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.

jcromartie6• 14 years ago

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.

IOS 6 injecting should_group_accessibility_children to POST requests

Related Stories