[olliej]

The core problem is deserialization formats where the serialized content specifies that class that should be instantiated. See the yaml, pickle, Java, etc serialization bugs over the years.

The real kicker here is that someone was clearly trying to do the correct thing (see mentions of secure coding), but the way secure coding works meant that arbitrary subclasses of any type that declared itself as supporting secure coding could be instantiated. Because the subclasses don't necessarily actually support secure coding you get much sadness.

There are things that could be done to make deserialization safer, but the core problem will remain that the untrusted content gets to specify the classes that will be instantiated.

[vesinisa]

What's so wrong with MessagePack, BSON etc.? Apart from de-serializing arbitrary classes from untrusted inputs being obviously insane, it's almost always an overkill as well. Why not simply stick to a binary format that doesn't even have a way of expressing data outside of a few well-defined primitive types?

[olliej]

iMessage has a large base of existing users that need to be able to continue to use messages, and presumably it was thought that by restricting with the NSSecureCoding API it would be safe, but as you say any API where the remote specifies the class to instantiate is going to be something of a footgun.

iMessage also predates MessagePack, BSON, and many of the other now "common" and "obvious" serialization formats, so you're (kind of) saying "why didn't it use a format that didn't exist".

The reality of course is that in addition to everything else NSCoding is the language supported serialization system, and not using it would have been an example of "reinventing the wheel". The reality is that the engineers using NSCoding + NSSecureCoding quite reasonably expected it to actually be secure (it's right in the title).

All those things aside it irks me that any of the message processing happens in the springboard process rather than a separate sandboxed process that doesn't have essentially complete access to everything that exists.

[tlb]

MessagePack only deserializes into special classes created by their IDL. The problem usually comes from calling the constructor on a user-written class, which doesn't carefully check constructor arguments.

[saagarjha]

I think the specific issue was that subclasses didn't support the semantics of the classes they inherited from correctly.

[olliej]

Yes, but the reason for that is because the subclasses didn't recognize that they were expected to conform to secure coding. Because subclasses inherit the value of the class flag supportsSecureCoding without any obvious indication of that.

The problem is that the core design of NSCodable (and NSSEcureCoding) is that the serialized data get to specify the type to instantiate, and the way supportsSecureCoding is exposed means that the claim to secure coding is inherited.

The former is a fairly common problem in serialization systems, and the security flaws in that approach resulted in the subsequent NSSecureCoding APIs.

The latter is a byproduct of how secure coding is implemented, and it results in any user of NSSecureCoding needing to be aware of all classes that may be transitively pulled into their address space. The subclassing semantics means that even the explicit list of allowed classes is not sufficiently robust - e.g '@interface ArbitraryInsecureClass : NSData' means ArbitraryInsecureClass can be instantiated if you allow NSData in the secure coding list, and ArbitraryInsecureClass could be declared in some transitively included framework, and is generally leaked implementation details.

These are all "fixable" for some value of fixable constrained by binary compatibility (though some of it could be mitigated by Darwin's linked-on-or-after mechanisms). There are a variety of steps that Apple could take that would make the entire NSCoding mechanism much more safe/secure without (afaict) necessarily breaking anything, though obviously I don't have access to the compatibility information they probably have.

Things I would do (if I were king?):

* Compiler warning when you subclass a class that declared supportsSecureCoding

* Make the allowed classes restrict to explicit classes rather than just conforming classes (e.g. if you say NSData you can't instantiate a subclass of NSData) - I would give good odds of this breaking things

* Rather than just checking the value of TargetClass.supportsSecureCoding I'd query the runtime to require supportsSecureCoding being a direct class member of TargetClass

* I would consider adding a "+property secureClass: Class" (or whatever the syntax is) that is instantiated, so even if you aren't using secure coding you end up instantiating a sane type - though it would be "optional" due to existing code and objects not having the property. But then at least Apple could go through the core types in Foundation (Array, Data, String, etc) and make it so you would only ever instantiate understood classes.

This would make the API slightly less footgun heavy.

[saagarjha]

Yeah, I'm not sure how far explicitly restricting the list of classes you can instantiate will go because of class clusters, etc which make it basically impossible to actually create "just" an NSString or NSData. However, it's still possible to have "secure" decoding if all subclasses preserves the semantics of their superclass, since applications will not be able to tell the difference. The issue here is that some classes did things that were outright incorrect with regards to their subclassing contract, and NSCoding made it possible for these poorly-written classes to be instantiated in unexpected contexts. (Oh, and thanks for fixing your formatting.)

[olliej]

XPC requires a specific set of classes to be enumerated outside the core datatypes, and that is something bounded. There is no part of your software that should ever be enumerating anything other than a deliberate concrete type. That's how we got into this mess in the first place :D

[re formatting: my desire to indent bullet points is matched only by HNs desire to then make each bullet point a single line \o/]