Re: Verbose warning splurged to the log with Secure Coding (dearchiving) -- can they be serious?

Graham Cox

On 25 Feb 2022, at 12:55 pm, Quincey Morris <quinceymorris@...> wrote:

I think you’re Doing It Wrong™. Your solution is effectively disabling secure coding, which solves your run-time error, but leaves you with the exact security vulnerability that secure coding is supposed to protect against.

Yes, I understand that writing the class name into the archive bypasses what secure coding is supposed to accomplish, though it adds a small extra hoop to jump through for a would-be malfeasant.

However, I’ve never quite understood how such an attack could succeed anyway — sure, you could theoretically hack an archive to substitute a different class, but that still isn't loading any code, which must be already linked to your app. Given other safeguards such as code signing, etc, I can’t see how that’s possible. If the class doesn’t exist already in your app, dearchiving fails. Does secure coding actually solve any known type of attack?

Nevertheless, I’d prefer to avoid this “solution” for the reason you state — that’s why I said it seems slightly smelly.

Obfuscating or encrypting the classname in the archive might be a possibility.

Your goal of “I want it to be as flexible as possible” without specifying classes in advance isn’t compatible with secure decoding. 

Let’s stick with your NSDictionary example. There are 3 cases:

1. You (try to) decode the dictionary and specify class NSDictionary. This is insecure, because decoding the dictionary also decodes arbitrary classes contained in the dictionary.

But dictionaries could be embedded deep inside some other objects that gets archived, even if you don’t know that. NSDictionary allows any kind of object (conforming to NSCopying) to be a key, so my thinking is that Apple must run into this exact same issue themselves for the secure archiving of NSDictionary. And yet they appear not to (I’ve not exhaustively tested this, but I can experiment further), suggesting that for their own classes they’re doing something privileged that we aren’t allowed to.

2. The dictionary contains only objects of “plist” classes such as NSNumber, NSDate and standard NS collection classes. There are a finite number of these, so you can always list them all in `decodeObjectOfClasses:forKey:`, and this kind of dictionary is useful for a lot of things without getting into custom classes.

Limiting keys to be plist types may be OK, without making it too inflexible. Most of the time strings are used. But limiting payload classes to plist types is a bigger limitation, since these are much more likely to be custom classes defined by client code.

3. The dictionary contains your custom classes and/or client code's custom classes as well. Again, you must use `decodeObjectOfClasses:forKey:` to specify *all* of the classes that might be in the dictionary, but you don’t know them all at compile time.

You can’t do #1, so you must do #2 or #3. Either way, use `decodeObjectOfClasses:forKey:` for a heterogenous collection or hierarchy.

Note that your set of classes for `decodeObjectOfClasses:forKey:` can be built at run-time, but you should avoid the temptation to encode that set into the archive, since that also compromises security. (An attacker can simply ask your code to unarchive data containing malicious classes, and naming their classes!)

OK, I might look into building such a set, but that would require that there is some use of the class before any dearchiving, so that the actual classes used for keys, etc can be seen and added to the set. (Other than common types which I can establish in advance). However, dearchiving could well be the first use of the class, if your app launches and opens a document. The set could be saved in preferences I guess, so it’s persistent over time… I dunno, seems pretty smelly again.

If you want to make things flexible for client code, I guess the way to do it would be to have the client tell you, on unarchiving, the list of its custom classes that it allows in the archive. You’d augment that set with the additional classes your own wrapper code needs, and specify that for `decodeObjectOfClasses:forKey:`.

I can’t see this being practical, for a basic container class that could live deep within another class. If the client code wants to dearchive an object graph containing this private stuff deep inside, they are just not going to know what classes are in use internally. They likely don’t even have a public interface. Certainly this is OK for where I build one class into another, and I’m already doing that in a couple of places.

Anyway, I’m still thinking about this, and I greatly appreciate the input Quincey.


Join to automatically receive all group messages.