5 ms·

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 unexpec

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 14y 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 14y 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.
  - jmcnevin 14y ago
    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.
    - BigZaphod 14y ago
      You never ever ever ever blindly trust the client to behave a certain way.
      - tomjen3 14y ago
        They are not. They give an error. The only issue is that a middle man is fucking with them.
        masklinn 14y ago
        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.
        tomjen3 14y ago
        Actually Rails have a feature to mark certain parameters as not mass assignable.
        masklinn 14y ago
        Which is also broken.
        ubernostrum 14y ago
        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.
    - huxley 14y ago
      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 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.
- BrandonSmith 14y 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.
- headShrinker 14y ago
  If your API breaks because an extra unrecognized parameter is included, Apple didn't break your API, you did.
  - phaylon 14y ago
    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.
    - headShrinker 14y ago
      "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.
- jcromartie 14y 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.
jmcnevin 14y ago
This (mass-assignment) is fairly standard Rails-fu, so its fair to assume that this could affect a lot of apps out there.
- ubernostrum 14y ago
  You say "standard Rails-fu", I say security hole. Let's call the whole thing off?
  - tomjen3 14y ago
    No because you don't know what you are talking about.
    - ubernostrum 14y ago
      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?
      - bryanlarsen 14y ago
        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.
        jerf 14y ago
        "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.
kalleboo 14y ago
Isn't this default Rails behavior? (despite sounding like something people would make fun of PHP for doing)
- masklinn 14y 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 https://github.com/rails/rails/issues/5239 http://erratasec.blogspot.be/2012/03/rubygithub-hack-translated.html http://erratasec.blogspot.be/2012/03/rubygithub-hack-transla...)
tomjen3 14y 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.
- untog 14y 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 http://guides.rubyonrails.org/security.html#mass-assignment
  - bryanlarsen 14y ago
    You can do either. The default used to be blacklist, but that was changed to whitelist recently.
  - bob_p 14y ago
    You can do either. Whitelist is obviously preferred.
  - alanpca 14y ago
    It allows either.
- Pewpewarrows 14y 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.

est 14y 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 14y 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.
- jmcnevin 14y ago
  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.
  - BigZaphod 14y ago
    You never ever ever ever blindly trust the client to behave a certain way.
    - tomjen3 14y ago
      They are not. They give an error. The only issue is that a middle man is fucking with them.
      - masklinn 14y ago
        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.
        tomjen3 14y ago
        Actually Rails have a feature to mark certain parameters as not mass assignable.
        masklinn 14y ago
        Which is also broken.
      - ubernostrum 14y ago
        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.
  - huxley 14y ago
    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 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.
BrandonSmith 14y 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.
headShrinker 14y ago
If your API breaks because an extra unrecognized parameter is included, Apple didn't break your API, you did.
- phaylon 14y ago
  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.
  - headShrinker 14y ago
    "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.
jcromartie 14y 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.

jmcnevin 14y ago

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

ubernostrum 14y ago
You say "standard Rails-fu", I say security hole. Let's call the whole thing off?
- tomjen3 14y ago
  No because you don't know what you are talking about.
  - ubernostrum 14y ago
    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?
    - bryanlarsen 14y ago
      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.
      - jerf 14y ago
        "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.

kalleboo 14y ago

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

masklinn 14y 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 https://github.com/rails/rails/issues/5239 http://erratasec.blogspot.be/2012/03/rubygithub-hack-translated.html http://erratasec.blogspot.be/2012/03/rubygithub-hack-transla...)

tomjen3 14y 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.

untog 14y 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 http://guides.rubyonrails.org/security.html#mass-assignment
- bryanlarsen 14y ago
  You can do either. The default used to be blacklist, but that was changed to whitelist recently.
- bob_p 14y ago
  You can do either. Whitelist is obviously preferred.
- alanpca 14y ago
  It allows either.
Pewpewarrows 14y 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.