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


Quincey Morris
 

On Feb 24, 2022, at 19:14, Graham Cox <graham@...> wrote:



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?

I don’t know an exact scenario, but hacking a class name in an archive is a trivial way of substituting a malicious class type for any other type. It will negate whatever protections secure coding gives you.

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.

I’m not sure I understand your concern exactly. When you say `decodeObjectOfClasses`, you’re starting a hierarchy/graph of decodes from a root object, because each object has to decode its own properties (or whatever sub-objects it’s chosen to represent itself). The entire hierarchy/graph of decodes is constrained by the classes you specified at the top level, so nothing escapes the validation.

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.

What I had in mind, assuming your code is doing the unarchiving for client code that has its own classes in the object graph, is that the client code would just supply you with a list of its classes once (during startup, or via some method or delegate that’s invoked just before decoding starts). You’d add those into the set of classes that your code uses, and that’s what you would pass to `decodeObjectOfClasses:forKey:`.

Or, it could work the other way round if the client code does the unarchiving of an object graph that includes your private objects. It would ask you, or you would volunteer, a set of classes before decoding starts, and that would allow your classes to decode securely. The client doesn’t need to know *where* your objects are in the object graph.

As I said before, this set is applied once at the top level of decoding. It doesn’t matter where in the object graph the classes are actually used.

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

—G

Join cocoa@apple-dev.groups.io to automatically receive all group messages.