3 ms·

> starting in OS X 10.5 (which would also be around the launch of iOS in 2007) NSFunctionExpressions were extended to allow arbitrary method invocations with th

> starting in OS X 10.5 (which would also be around the launch of iOS in 2007) NSFunctionExpressions were extended to allow arbitrary method invocations with the FUNCTION keyword: "FUNCTION('abc', 'stringByAppendingString', 'def')" => @"abcdef" This is always a bad idea. If you want a callback, register it manually so only registered functions are available. Java serialization and Log4J made same style of security bug.

slaymaker1907 5y ago
Racket allows for macro/exec, but it strictly controls the namespace for both of these things. The former mainly via hygienic macros and the latter via requiring exec to be given a namespace to execute in. I think allowing things like NSFunctionExpressions are ok, but you need to have an explicit namespace, and the default needs to be an empty namespace, not the current/full namespace. Having a default is a good idea because if you don't add one as an API designer, other people creating wrappers around your library will, so at least try to make the default safe. It sounds like that is somewhat similar to your idea of registering a function.
saagarjha 5y ago
It's easy to just look at features and call them "obviously" bad but this feature provides an extensible interface that isn't much different from any other API in Objective-C that takes a selector, e.g. sortedArrayUsingSelector:. This is just how the language is. It is your job to restrict what can go into that function; in this case where the attacker controls the address space it doesn't matter anyways because an attacker will just create a calling primitive elsewhere. So this is really just a kneejerk reaction that doesn't meaningfully help security here, and removes a fairly useful API. Note that if you actually wanted to make the attack harder here I'd go after the NSPredicate evaluation instead, which you can see in the blog post is done without checking the sender, rather than going after what the predicate itself can do.
- deleted 5y ago
  [deleted]
- arcticbull 5y ago
  I honestly cannot understand the motivation of allowing arbitrary function execution in an NSPredicate at all. That seems like an unbounded complexity risk, especially when serialized. They've just defined a fun little scripting language in this random corner of Foundation. Testing it appropriately seems nightmarish, and the wrong inputs would just crash the app. A system that only allows a small set of permitted functions, or implements hard-coded functions like the first version (min, max, avg) just seems far more ... reasonable. It's the job of an API framework provider to be defensive against these kinds of vectors especially when they may not deprecate this for like, 10+ years. How this made it through Apple's API review is beyond me. If I saw someone try and pull something like this at the office I would immediately make sure the design was rejected.
  - zarzavat 5y ago
    NSPredicate dates from before when Objective-C had closures. Objective-C didn’t have closures because it was a thin layer on top of C, and C uses callbacks not closures. NSPredicate is serialisable because you might want to include it in e.g. a schema. These schemas would be loaded from the app bundle so there’s no security concerns unless your threat model includes modifying the app bundle (which is more of an OS level issue). The issue is that the serialization needs to distinguish between data that is serialized by the developer of the app at build-time, and data that is serialized at runtime.
    - arcticbull 5y ago
      I remember, I was a macOS engineer when it came out. I think NSPredicate, even a serializable one, makes sense. What doesn't make any sense is arbitrary selector invocation. Not just because it's a security risk, but also because it's a stringly-typed method name that isn't checked by the compiler. The name of the selector changing would just cause it to crash - without a compile-time check. A plist with a serialized selector that no longer exists would cause the app to crash. A plist with a malicious selector would cause the app to crash. And crashing seems like a best case outcome.
mwcampbell 5y ago
The fact that the extension of NSFunctionExpression in Leopard and the launch of iOS happened around the same time is interesting to me. Clearly one side of Apple was continuing to do everything they could to make their platform more attractive to high-level application developers, continuing the focus on this area that went back to the early years of NeXT [1], unaware (or unconcerned) that another part of Apple would be using this same framework in a consumer device where any way of abusing all of this power could be weaponized. [1]: See in particular the part of the original NeXT presentation right after intermission, with demos of gluing together high-level components using Interface Builder. https://www.youtube.com/watch?v=92NNyd3m79I https://www.youtube.com/watch?v=92NNyd3m79I
plorkyeran 5y ago
NSPredicate has from the very beginning been the moral equivalent of `eval()`, with all of the security implications that has. Sending arbitrary messages to arbitrary objects is just fundamentally the thing it does. The addition of FUNCTION() certainly made it easier to turn a security vulnerability into a useful attack, but if an attacker can control the format string passed to NSPredicate (or more generally the construction of a NSPredicate) you always had a security vulnerability.

slaymaker1907 5y ago

Racket allows for macro/exec, but it strictly controls the namespace for both of these things. The former mainly via hygienic macros and the latter via requiring exec to be given a namespace to execute in. I think allowing things like NSFunctionExpressions are ok, but you need to have an explicit namespace, and the default needs to be an empty namespace, not the current/full namespace. Having a default is a good idea because if you don't add one as an API designer, other people creating wrappers around your library will, so at least try to make the default safe. It sounds like that is somewhat similar to your idea of registering a function.

saagarjha 5y ago

It's easy to just look at features and call them "obviously" bad but this feature provides an extensible interface that isn't much different from any other API in Objective-C that takes a selector, e.g. sortedArrayUsingSelector:. This is just how the language is. It is your job to restrict what can go into that function; in this case where the attacker controls the address space it doesn't matter anyways because an attacker will just create a calling primitive elsewhere. So this is really just a kneejerk reaction that doesn't meaningfully help security here, and removes a fairly useful API. Note that if you actually wanted to make the attack harder here I'd go after the NSPredicate evaluation instead, which you can see in the blog post is done without checking the sender, rather than going after what the predicate itself can do.

deleted 5y ago
[deleted]
arcticbull 5y ago
I honestly cannot understand the motivation of allowing arbitrary function execution in an NSPredicate at all. That seems like an unbounded complexity risk, especially when serialized. They've just defined a fun little scripting language in this random corner of Foundation. Testing it appropriately seems nightmarish, and the wrong inputs would just crash the app. A system that only allows a small set of permitted functions, or implements hard-coded functions like the first version (min, max, avg) just seems far more ... reasonable. It's the job of an API framework provider to be defensive against these kinds of vectors especially when they may not deprecate this for like, 10+ years. How this made it through Apple's API review is beyond me. If I saw someone try and pull something like this at the office I would immediately make sure the design was rejected.
- zarzavat 5y ago
  NSPredicate dates from before when Objective-C had closures. Objective-C didn’t have closures because it was a thin layer on top of C, and C uses callbacks not closures. NSPredicate is serialisable because you might want to include it in e.g. a schema. These schemas would be loaded from the app bundle so there’s no security concerns unless your threat model includes modifying the app bundle (which is more of an OS level issue). The issue is that the serialization needs to distinguish between data that is serialized by the developer of the app at build-time, and data that is serialized at runtime.
  - arcticbull 5y ago
    I remember, I was a macOS engineer when it came out. I think NSPredicate, even a serializable one, makes sense. What doesn't make any sense is arbitrary selector invocation. Not just because it's a security risk, but also because it's a stringly-typed method name that isn't checked by the compiler. The name of the selector changing would just cause it to crash - without a compile-time check. A plist with a serialized selector that no longer exists would cause the app to crash. A plist with a malicious selector would cause the app to crash. And crashing seems like a best case outcome.

mwcampbell 5y ago

The fact that the extension of NSFunctionExpression in Leopard and the launch of iOS happened around the same time is interesting to me. Clearly one side of Apple was continuing to do everything they could to make their platform more attractive to high-level application developers, continuing the focus on this area that went back to the early years of NeXT [1], unaware (or unconcerned) that another part of Apple would be using this same framework in a consumer device where any way of abusing all of this power could be weaponized. [1]: See in particular the part of the original NeXT presentation right after intermission, with demos of gluing together high-level components using Interface Builder. https://www.youtube.com/watch?v=92NNyd3m79I https://www.youtube.com/watch?v=92NNyd3m79I

plorkyeran 5y ago

NSPredicate has from the very beginning been the moral equivalent of `eval()`, with all of the security implications that has. Sending arbitrary messages to arbitrary objects is just fundamentally the thing it does. The addition of FUNCTION() certainly made it easier to turn a security vulnerability into a useful attack, but if an attacker can control the format string passed to NSPredicate (or more generally the construction of a NSPredicate) you always had a security vulnerability.